Simplify the flux flags parsing

We can save the clean-up phase, which reset things to `nil` if they
weren't passed as flags, by checking with the flag parser
directly. This makes it trickier to construct a config (since it needs
the flags parser state), but that's what `ParseFluxArgs` is for.

The code that uses ParseFluxArgs is sensitive to whether it gets a
`nil`, a value, or an error, so that behaviour must be preserved.

agent/flux.go

@@ -13,19 +13,19 @@ import (
 // FluxConfig stores existing flux arguments which will be used when updating WC agents
 type FluxConfig struct {
 	// git parameters
-	GitLabel *string
-	GitURL   *string
+	GitLabel string
+	GitURL   string
 	// This arg is multi-valued, or can be passed as comma-separated
 	// values. This accounts for either form.
 	GitPath         []string
-	GitBranch       *string
-	GitTimeout      *time.Duration
-	GitPollInterval *time.Duration
-	GitSetAuthor    *bool
-	GitCISkip       *bool
+	GitBranch       string
+	GitTimeout      time.Duration
+	GitPollInterval time.Duration
+	GitSetAuthor    bool
+	GitCISkip       bool
 
 	// sync behaviour
-	GarbageCollection *bool
+	GarbageCollection bool
 
 	// For specifying ECR region from outside AWS (fluxd detects it when inside AWS)
 	RegistryECRRegion []string
@@ -38,11 +38,13 @@ type FluxConfig struct {
 	// This is now hard-wired to empty in launch-generator, to tell flux
 	// _not_ to use service discovery. But: maybe someone needs to use
 	// service discovery.
-	MemcachedService *string
+	MemcachedService string
 
 	// Just in case we more explicitly support restricting Weave
 	// Cloud, or just Flux to particular namespaces
 	AllowNamespace []string
+
+	fs *pflag.FlagSet
 }
 
 // AsQueryParams returns the configuration as a fragment of query
@@ -51,19 +53,15 @@ func (c *FluxConfig) AsQueryParams() string {
 	// Nothing clever here.
 	vals := url.Values{}
 
-	if c == nil {
-		return ""
-	}
-
 	// String-valued arguments
-	for arg, val := range map[string]*string{
+	for arg, val := range map[string]string{
 		"git-label":         c.GitLabel,
 		"git-url":           c.GitURL,
 		"git-branch":        c.GitBranch,
 		"memcached-service": c.MemcachedService,
 	} {
-		if val != nil {
-			vals.Add(arg, *val)
+		if c.fs.Changed(arg) {
+			vals.Add(arg, val)
 		}
 	}
 
@@ -81,59 +79,48 @@ func (c *FluxConfig) AsQueryParams() string {
 	}
 
 	// duration-valued arguments
-	for arg, dur := range map[string]*time.Duration{
+	for arg, dur := range map[string]time.Duration{
 		"git-timeout":       c.GitTimeout,
 		"git-poll-interval": c.GitPollInterval,
 	} {
-		if dur != nil {
+		if c.fs.Changed(arg) {
 			vals.Add(arg, dur.String())
 		}
 	}
 
-	for arg, flag := range map[string]*bool{
+	for arg, flag := range map[string]bool{
 		"sync-garbage-collection": c.GarbageCollection,
 		"git-set-author":          c.GitSetAuthor,
 		"git-ci-skip":             c.GitCISkip,
 	} {
-		if flag != nil {
-			vals.Add(arg, strconv.FormatBool(*flag))
+		if c.fs.Changed(arg) {
+			vals.Add(arg, strconv.FormatBool(flag))
 		}
 	}
 
 	return vals.Encode()
 }
 
-func isFlagPassed(fs *pflag.FlagSet, name string) bool {
-	found := false
-	fs.Visit(func(f *pflag.Flag) {
-		if f.Name == name {
-			found = true
-		}
-	})
-	return found
-}
-
 // ParseFluxArgs parses a string of args into a nice FluxConfig
 func ParseFluxArgs(argString string) (*FluxConfig, error) {
-	cfg := &FluxConfig{}
-
 	fs := pflag.NewFlagSet("default", pflag.ContinueOnError)
 	fs.ParseErrorsWhitelist.UnknownFlags = true
+	cfg := &FluxConfig{fs: fs}
 
 	// strings
-	cfg.GitLabel = fs.String("git-label", "", "")
-	cfg.GitURL = fs.String("git-url", "", "")
-	cfg.GitBranch = fs.String("git-branch", "", "")
-	cfg.MemcachedService = fs.String("memcached-service", "", "")
+	fs.StringVar(&cfg.GitLabel, "git-label", "", "")
+	fs.StringVar(&cfg.GitURL, "git-url", "", "")
+	fs.StringVar(&cfg.GitBranch, "git-branch", "", "")
+	fs.StringVar(&cfg.MemcachedService, "memcached-service", "", "")
 
 	// durations
-	cfg.GitTimeout = fs.Duration("git-timeout", time.Second, "")
-	cfg.GitPollInterval = fs.Duration("git-poll-interval", time.Minute, "")
+	fs.DurationVar(&cfg.GitTimeout, "git-timeout", time.Second, "")
+	fs.DurationVar(&cfg.GitPollInterval, "git-poll-interval", time.Minute, "")
 
 	// bools
-	cfg.GitSetAuthor = fs.Bool("git-set-author", false, "")
-	cfg.GitCISkip = fs.Bool("git-ci-skip", false, "")
-	cfg.GarbageCollection = fs.Bool("sync-garbage-collection", false, "")
+	fs.BoolVar(&cfg.GitSetAuthor, "git-set-author", false, "")
+	fs.BoolVar(&cfg.GitCISkip, "git-ci-skip", false, "")
+	fs.BoolVar(&cfg.GarbageCollection, "sync-garbage-collection", false, "")
 
 	// string slices
 	fs.StringSliceVar(&cfg.GitPath, "git-path", nil, "")
@@ -145,41 +132,10 @@ func ParseFluxArgs(argString string) (*FluxConfig, error) {
 	// Parse it all
 	fs.Parse(strings.Split(argString, " "))
 
-	// Cleanup anything that wasn't explicitly set
-	for arg, val := range map[string]**string{
-		"git-label":         &cfg.GitLabel,
-		"git-url":           &cfg.GitURL,
-		"git-branch":        &cfg.GitBranch,
-		"memcached-service": &cfg.MemcachedService,
-	} {
-		if !isFlagPassed(fs, arg) {
-			*val = nil
-		}
+	if fs.NFlag() > 0 {
+		return cfg, nil
 	}
-	for arg, val := range map[string]**time.Duration{
-		"git-timeout":       &cfg.GitTimeout,
-		"git-poll-interval": &cfg.GitPollInterval,
-	} {
-		if !isFlagPassed(fs, arg) {
-			*val = nil
-		}
-	}
-	for arg, val := range map[string]**bool{
-		"sync-garbage-collection": &cfg.GarbageCollection,
-		"git-set-author":          &cfg.GitSetAuthor,
-		"git-ci-skip":             &cfg.GitCISkip,
-	} {
-		if !isFlagPassed(fs, arg) {
-			*val = nil
-		}
-	}
-
-	// Did we find anything?
-	if cfg.AsQueryParams() == "" {
-		return nil, nil
-	}
-
-	return cfg, nil
+	return nil, nil
 }
 
 func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) {

agent/main.go

@@ -39,9 +39,7 @@ const (
 	defaultWCHostname        = "cloud.weave.works"
 	defaultWCPollURL         = "https://{{.WCHostname}}/k8s.yaml" +
 		"?k8s-version={{.KubernetesVersion}}&t={{.Token}}&omit-support-info=true" +
-		"{{if .FluxConfig}}" +
-		"&{{.FluxConfig.AsQueryParams}}" +
-		"{{end}}" +
+		"{{if .FluxConfig.AsQueryParams}}&{{.FluxConfig.AsQueryParams}}{{end}}" +
 		"{{if .CRIEndpoint}}" +
 		"&cri-endpoint={{.CRIEndpoint}}" +
 		"{{end}}" +

agent/main_test.go

@@ -2,10 +2,11 @@ package main
 
 import (
 	"errors"
+	"fmt"
 	"net/url"
 	"reflect"
+	"strings"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -59,16 +60,11 @@ func TestAgentManifestURL(t *testing.T) {
 func TestAgentFluxURL(t *testing.T) {
 	// Not an exhaustive test; just representative
 	gitPath := []string{"config/helloworld", "config/hej-world"}
-	memcachedService := ""
-	gitCISkip := false
-	gitTimeout := 40 * time.Second
-
-	fluxCfg := &FluxConfig{
-		GitPath:          gitPath,
-		MemcachedService: &memcachedService,
-		GitCISkip:        &gitCISkip,
-		GitTimeout:       &gitTimeout,
-	}
+
+	argsStr := fmt.Sprintf(`--git-path=%s --memcached-service= --git-ci-skip=false --git-timeout=40s`, strings.Join(gitPath, ","))
+
+	fluxCfg, err := ParseFluxArgs(argsStr)
+	assert.NoError(t, err)
 
 	cfg := &agentConfig{
 		AgentPollURLTemplate: defaultWCPollURL,
@@ -98,24 +94,24 @@ func TestParseFluxArgs(t *testing.T) {
 	argString = "--git-ci-skip"
 	fluxCfg, err = ParseFluxArgs(argString)
 	assert.Equal(t, nil, err)
-	assert.Equal(t, true, *fluxCfg.GitCISkip)
+	assert.Equal(t, true, fluxCfg.GitCISkip)
 
 	// Test handling boolean flags w `=true|false`
 	argString = "--git-ci-skip=true"
 	fluxCfg, err = ParseFluxArgs(argString)
 	assert.Equal(t, nil, err)
-	assert.Equal(t, true, *fluxCfg.GitCISkip)
+	assert.Equal(t, true, fluxCfg.GitCISkip)
 
 	argString = "--git-ci-skip=false"
 	fluxCfg, err = ParseFluxArgs(argString)
 	assert.Equal(t, nil, err)
-	assert.Equal(t, false, *fluxCfg.GitCISkip)
+	assert.Equal(t, false, fluxCfg.GitCISkip)
 
 	// Test we only serialize props that we provided
 	argString = "--git-label=foo --git-path=derp"
 	fluxCfg, err = ParseFluxArgs(argString)
 	assert.Equal(t, nil, err)
-	assert.Equal(t, "foo", *fluxCfg.GitLabel)
+	assert.Equal(t, "foo", fluxCfg.GitLabel)
 	assert.Equal(t, "git-label=foo&git-path=derp", fluxCfg.AsQueryParams())
 
 	// string[]

integration-tests/tests/kube-system-migration.sh

@@ -35,6 +35,6 @@ while [ $(kubectl get pods -n kube-system -l 'app in (weave-flux, weave-cortex,
 echo "• Check old flux arguments have been applied to the new agent"
 args=$(kubectl get pod -n weave -l name=weave-flux-agent -o jsonpath='{.items[?(@.metadata.labels.name=="weave-flux-agent")].spec.containers[?(@.name=="flux-agent")].args[*]}')
 if [[ $args != *"--git-url=git@github.com:weaveworks/example --git-path=k8s/example --git-branch=master --git-label=example"* ]]; then
-    echo "Missing existing flux args: $args"
+    echo "Missing existing flux args: got $args"
     exit 1
 fi