From 85e0338136ecc64d765b983da54334a6b4bdf87e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 1 May 2020 18:17:28 -0400 Subject: [PATCH] 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 | 6 --- agent/config/flags.go | 5 +- agent/config/flags_test.go | 96 ++++++++++++++++++------------------ agent/config/runtime_test.go | 2 +- command/agent/agent.go | 7 ++- command/agent/agent_test.go | 2 +- 6 files changed, 55 insertions(+), 63 deletions(-) diff --git a/agent/config/builder.go b/agent/config/builder.go index 026fa35673..0185a66634 100644 --- a/agent/config/builder.go +++ b/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 { diff --git a/agent/config/flags.go b/agent/config/flags.go index fccee4c1d0..8d1fc8a018 100644 --- a/agent/config/flags.go +++ b/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. diff --git a/agent/config/flags_test.go b/agent/config/flags_test.go index 63563b298f..91f097a5e1 100644 --- a/agent/config/flags_test.go +++ b/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 + args []string + expected Flags + extra []string }{ {}, { - args: []string{`-bind`, `a`}, - flags: Flags{Config: Config{BindAddr: pString("a")}}, + args: []string{`-bind`, `a`}, + expected: Flags{Config: Config{BindAddr: pString("a")}}, }, { - args: []string{`-bootstrap`}, - flags: Flags{Config: Config{Bootstrap: pBool(true)}}, + args: []string{`-bootstrap`}, + expected: Flags{Config: Config{Bootstrap: pBool(true)}}, }, { - args: []string{`-bootstrap=true`}, - flags: Flags{Config: Config{Bootstrap: pBool(true)}}, + args: []string{`-bootstrap=true`}, + expected: Flags{Config: Config{Bootstrap: pBool(true)}}, }, { - args: []string{`-bootstrap=false`}, - flags: Flags{Config: Config{Bootstrap: pBool(false)}}, + args: []string{`-bootstrap=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"}}, + args: []string{`-config-file`, `a`, `-config-dir`, `b`, `-config-file`, `c`, `-config-dir`, `d`}, + expected: Flags{ConfigFiles: []string{"a", "b", "c", "d"}}, }, { - args: []string{`-datacenter`, `a`}, - flags: Flags{Config: Config{Datacenter: pString("a")}}, + args: []string{`-datacenter`, `a`}, + expected: Flags{Config: Config{Datacenter: pString("a")}}, }, { - args: []string{`-dns-port`, `1`}, - flags: Flags{Config: Config{Ports: Ports{DNS: pInt(1)}}}, + args: []string{`-dns-port`, `1`}, + expected: Flags{Config: Config{Ports: Ports{DNS: pInt(1)}}}, }, { - args: []string{`-grpc-port`, `1`}, - flags: Flags{Config: Config{Ports: Ports{GRPC: pInt(1)}}}, + args: []string{`-grpc-port`, `1`}, + expected: Flags{Config: Config{Ports: Ports{GRPC: pInt(1)}}}, }, { - args: []string{`-http-port`, `1`}, - flags: Flags{Config: Config{Ports: Ports{HTTP: pInt(1)}}}, + args: []string{`-http-port`, `1`}, + expected: Flags{Config: Config{Ports: Ports{HTTP: pInt(1)}}}, }, { - args: []string{`-https-port`, `1`}, - flags: Flags{Config: Config{Ports: Ports{HTTPS: pInt(1)}}}, + args: []string{`-https-port`, `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)}}}, + args: []string{`-serf-lan-port`, `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)}}}, + args: []string{`-serf-wan-port`, `1`}, + expected: Flags{Config: Config{Ports: Ports{SerfWAN: pInt(1)}}}, }, { - args: []string{`-server-port`, `1`}, - flags: Flags{Config: Config{Ports: Ports{Server: pInt(1)}}}, + args: []string{`-server-port`, `1`}, + expected: Flags{Config: Config{Ports: Ports{Server: pInt(1)}}}, }, { - args: []string{`-join`, `a`, `-join`, `b`}, - flags: Flags{Config: Config{StartJoinAddrsLAN: []string{"a", "b"}}}, + args: []string{`-join`, `a`, `-join`, `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"}}}, + args: []string{`-node-meta`, `a:b`, `-node-meta`, `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"}}, + args: []string{`-bootstrap`, `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) }) } } diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 20d9e70be7..d8e137a596 100644 --- a/agent/config/runtime_test.go +++ b/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() diff --git a/command/agent/agent.go b/command/agent/agent.go index f1df46ad22..ee088dce8b 100644 --- a/command/agent/agent.go +++ b/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 diff --git a/command/agent/agent_test.go b/command/agent/agent_test.go index 2f37a52d0a..835d98850c 100644 --- a/command/agent/agent_test.go +++ b/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"},