config: remove Args field from Flags

This field was populated for one reason, to test that it was empty.
Of all the callers, only a single one used this functionality. The rest
constructed a `Flags{}` struct which did not set Args.

I think this shows that the logic was in the wrong place. Only the agent
command needs to care about validating the args.

This commit removes the field, and moves the logic to the one caller
that cares.

Also fix some comments.

agent/config/builder.go

@@ -84,12 +84,6 @@ type Builder struct {
 // NewBuilder returns a new configuration builder based on the given command
 // line flags.
 func NewBuilder(flags Flags) (*Builder, error) {
-	// We expect all flags to be parsed and flags.Args to be empty.
-	// Therefore, we bail if we find unparsed args.
-	if len(flags.Args) > 0 {
-		return nil, fmt.Errorf("config: Unknown extra arguments: %v", flags.Args)
-	}
-
 	newSource := func(name string, v interface{}) Source {
 		b, err := json.MarshalIndent(v, "", "    ")
 		if err != nil {

agent/config/flags.go

@@ -20,15 +20,12 @@ type Flags struct {
 	// format independent of their extension.
 	ConfigFormat *string
 
-	// HCL contains an arbitrary config in hcl format.
 	// DevMode indicates whether the agent should be started in development
 	// mode. This cannot be configured in a config file.
 	DevMode *bool
 
+	// HCL contains an arbitrary config in hcl format.
 	HCL []string
-
-	// Args contains the remaining unparsed flags.
-	Args []string
 }
 
 // AddFlags adds the command line flags for the agent.

agent/config/flags_test.go

@@ -2,91 +2,91 @@ package config
 
 import (
 	"flag"
-	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
-// TestParseFlags tests whether command line flags are properly parsed
+// TestAddFlags_WithParse tests whether command line flags are properly parsed
 // into the Flags/File structure. It contains an example for every type
 // that is parsed. It does not test the conversion into the final
 // runtime configuration. See TestConfig for that.
-func TestParseFlags(t *testing.T) {
+func TestAddFlags_WithParse(t *testing.T) {
 	tests := []struct {
 		args     []string
-		flags Flags
-		err   error
+		expected Flags
+		extra    []string
 	}{
 		{},
 		{
 			args:     []string{`-bind`, `a`},
-			flags: Flags{Config: Config{BindAddr: pString("a")}},
+			expected: Flags{Config: Config{BindAddr: pString("a")}},
 		},
 		{
 			args:     []string{`-bootstrap`},
-			flags: Flags{Config: Config{Bootstrap: pBool(true)}},
+			expected: Flags{Config: Config{Bootstrap: pBool(true)}},
 		},
 		{
 			args:     []string{`-bootstrap=true`},
-			flags: Flags{Config: Config{Bootstrap: pBool(true)}},
+			expected: Flags{Config: Config{Bootstrap: pBool(true)}},
 		},
 		{
 			args:     []string{`-bootstrap=false`},
-			flags: Flags{Config: Config{Bootstrap: pBool(false)}},
+			expected: Flags{Config: Config{Bootstrap: pBool(false)}},
 		},
 		{
 			args:     []string{`-config-file`, `a`, `-config-dir`, `b`, `-config-file`, `c`, `-config-dir`, `d`},
-			flags: Flags{ConfigFiles: []string{"a", "b", "c", "d"}},
+			expected: Flags{ConfigFiles: []string{"a", "b", "c", "d"}},
 		},
 		{
 			args:     []string{`-datacenter`, `a`},
-			flags: Flags{Config: Config{Datacenter: pString("a")}},
+			expected: Flags{Config: Config{Datacenter: pString("a")}},
 		},
 		{
 			args:     []string{`-dns-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{DNS: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{DNS: pInt(1)}}},
 		},
 		{
 			args:     []string{`-grpc-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{GRPC: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{GRPC: pInt(1)}}},
 		},
 		{
 			args:     []string{`-http-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{HTTP: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{HTTP: pInt(1)}}},
 		},
 		{
 			args:     []string{`-https-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{HTTPS: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{HTTPS: pInt(1)}}},
 		},
 		{
 			args:     []string{`-serf-lan-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{SerfLAN: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{SerfLAN: pInt(1)}}},
 		},
 		{
 			args:     []string{`-serf-wan-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{SerfWAN: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{SerfWAN: pInt(1)}}},
 		},
 		{
 			args:     []string{`-server-port`, `1`},
-			flags: Flags{Config: Config{Ports: Ports{Server: pInt(1)}}},
+			expected: Flags{Config: Config{Ports: Ports{Server: pInt(1)}}},
 		},
 		{
 			args:     []string{`-join`, `a`, `-join`, `b`},
-			flags: Flags{Config: Config{StartJoinAddrsLAN: []string{"a", "b"}}},
+			expected: Flags{Config: Config{StartJoinAddrsLAN: []string{"a", "b"}}},
 		},
 		{
 			args:     []string{`-node-meta`, `a:b`, `-node-meta`, `c:d`},
-			flags: Flags{Config: Config{NodeMeta: map[string]string{"a": "b", "c": "d"}}},
+			expected: Flags{Config: Config{NodeMeta: map[string]string{"a": "b", "c": "d"}}},
 		},
 		{
 			args:     []string{`-bootstrap`, `true`},
-			flags: Flags{Config: Config{Bootstrap: pBool(true)}, Args: []string{"true"}},
+			expected: Flags{Config: Config{Bootstrap: pBool(true)}},
+			extra:    []string{"true"},
 		},
 		{
 			args: []string{`-primary-gateway`, `foo.local`, `-primary-gateway`, `bar.local`},
-			flags: Flags{Config: Config{PrimaryGateways: []string{
+			expected: Flags{Config: Config{PrimaryGateways: []string{
 				"foo.local", "bar.local",
 			}}},
 		},
@@ -97,22 +97,20 @@ func TestParseFlags(t *testing.T) {
 			flags := Flags{}
 			fs := flag.NewFlagSet("", flag.ContinueOnError)
 			AddFlags(fs, &flags)
+
 			err := fs.Parse(tt.args)
-			if got, want := err, tt.err; !reflect.DeepEqual(got, want) {
-				t.Fatalf("got error %v want %v", got, want)
-			}
-			flags.Args = fs.Args()
+			require.NoError(t, err)
 
 			// Normalize the expected value because require.Equal considers
 			// empty slices/maps and nil slices/maps to be different.
-			if len(tt.flags.Args) == 0 && flags.Args != nil {
-				tt.flags.Args = []string{}
+			if tt.extra == nil && fs.Args() != nil {
+				tt.extra = []string{}
 			}
-			if len(tt.flags.Config.NodeMeta) == 0 {
-				tt.flags.Config.NodeMeta = map[string]string{}
+			if len(tt.expected.Config.NodeMeta) == 0 {
+				tt.expected.Config.NodeMeta = map[string]string{}
 			}
-
-			require.Equal(t, tt.flags, flags)
+			require.Equal(t, tt.extra, fs.Args())
+			require.Equal(t, tt.expected, flags)
 		})
 	}
 }

agent/config/runtime_test.go

@@ -3851,7 +3851,7 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) {
 				if err != nil {
 					t.Fatalf("ParseFlags failed: %s", err)
 				}
-				flags.Args = fs.Args()
+				require.Len(t, fs.Args(), 0)
 
 				if tt.pre != nil {
 					tt.pre()

command/agent/agent.go

@@ -174,14 +174,17 @@ func (c *cmd) startupJoinWan(agent *agent.Agent, cfg *config.RuntimeConfig) erro
 }
 
 func (c *cmd) run(args []string) int {
-	// Parse our configs
 	if err := c.flags.Parse(args); err != nil {
 		if !strings.Contains(err.Error(), "help requested") {
 			c.UI.Error(fmt.Sprintf("error parsing flags: %v", err))
 		}
 		return 1
 	}
-	c.flagArgs.Args = c.flags.Args()
+	if len(c.flags.Args()) > 0 {
+		c.UI.Error(fmt.Sprintf("Unexpected extra arguments: %v", c.flags.Args()))
+		return 1
+	}
+
 	config := c.readConfig()
 	if config == nil {
 		return 1

command/agent/agent_test.go

@@ -34,7 +34,7 @@ func TestConfigFail(t *testing.T) {
 		},
 		{
 			args: []string{"agent", "-server", "-bind=10.0.0.1", "-datacenter=foo", "some-other-arg"},
-			out:  "==> config: Unknown extra arguments: [some-other-arg]\n",
+			out:  "==> Unexpected extra arguments: [some-other-arg]\n",
 		},
 		{
 			args: []string{"agent", "-server", "-bind=10.0.0.1"},