luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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.
This commit is contained in:
Daniel Nephin 2020-05-01 18:17:28 -04:00
parent 599551496f
commit 5ac012dddf
6 changed files with 55 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
agent/config/builder.go
View File

@ -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 {

5
agent/config/flags.go
View File

@ -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.

96
agent/config/flags_test.go
View File

@ -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)
})
}
}

2
agent/config/runtime_test.go
View File

@ -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()

7
command/agent/agent.go
View File

@ -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

2
command/agent/agent_test.go
View File

@ -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"},