Fix options parsing - ensuring that TOKEN_PATH and TOKEN_XXX aren't mixed (#367)

Author: davidcollom

cmd/app/options.go

@@ -64,19 +64,25 @@ var (
 
 // Options is a struct to hold options for the version-checker.
 type Options struct {
+	kubeConfigFlags *genericclioptions.ConfigFlags
+
+	Client                client.Options
 	MetricsServingAddress string
-	DefaultTestAll        bool
-	CacheTimeout          time.Duration
 	LogLevel              string
 
-	PprofBindAddress        string
+	PprofBindAddress string
+	selfhosted       selfhosted.Options
+
+	CacheTimeout            time.Duration
 	GracefulShutdownTimeout time.Duration
 	CacheSyncPeriod         time.Duration
 
-	kubeConfigFlags *genericclioptions.ConfigFlags
-	selfhosted      selfhosted.Options
+	DefaultTestAll bool
+}
 
-	Client client.Options
+type envMatcher struct {
+	re     *regexp.Regexp
+	action func(matches []string, value string)
 }
 
 func (o *Options) addFlags(cmd *cobra.Command) {
@@ -363,59 +369,83 @@ func (o *Options) assignSelfhosted(envs []string) {
 		}
 	}
 
-	regexActions := map[*regexp.Regexp]func(matches []string, value string){
-		selfhostedHostReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Host = value
+	// Go maps iterate in random order - Using a slice to consistency
+	regexActions := []envMatcher{
+		{
+			re: selfhostedTokenPath,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].TokenPath = value
+			},
 		},
-		selfhostedUsernameReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Username = value
+		{
+			re: selfhostedTokenReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Bearer = value
+			},
 		},
-		selfhostedPasswordReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Password = value
+		// All your other patterns (host, username, password, insecure, capath...)
+		{
+			re: selfhostedHostReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Host = value
+			},
 		},
-		selfhostedTokenPath: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].TokenPath = value
+		{
+			re: selfhostedUsernameReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Username = value
+			},
 		},
-		selfhostedTokenReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].Bearer = value
+		{
+			re: selfhostedPasswordReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].Password = value
+			},
 		},
-		selfhostedInsecureReg: func(matches []string, value string) {
-			initOptions(matches[1])
-			if val, err := strconv.ParseBool(value); err == nil {
-				o.Client.Selfhosted[matches[1]].Insecure = val
-			}
+		{
+			re: selfhostedInsecureReg,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				if b, err := strconv.ParseBool(value); err == nil {
+					o.Client.Selfhosted[matches[1]].Insecure = b
+				}
+			},
 		},
-		selfhostedCAPath: func(matches []string, value string) {
-			initOptions(matches[1])
-			o.Client.Selfhosted[matches[1]].CAPath = value
+		{
+			re: selfhostedCAPath,
+			action: func(matches []string, value string) {
+				initOptions(matches[1])
+				o.Client.Selfhosted[matches[1]].CAPath = value
+			},
 		},
 	}
 
 	for _, env := range envs {
-		pair := strings.SplitN(env, "=", 2)
-		if len(pair) != 2 || len(pair[1]) == 0 {
+		parts := strings.SplitN(env, "=", 2)
+		if len(parts) != 2 || parts[1] == "" {
 			continue
 		}
+		key := strings.ToUpper(parts[0])
+		val := parts[1]
 
-		key := strings.ToUpper(pair[0])
-		value := pair[1]
-
-		for regex, action := range regexActions {
-			if matches := regex.FindStringSubmatch(key); len(matches) == 2 {
-				action(matches, value)
+		for _, p := range regexActions {
+			if match := p.re.FindStringSubmatch(key); len(match) == 2 {
+				p.action(match, val)
 				break
 			}
 		}
 	}
 
+	// If we have some selfhosted flags, lets set them here...
 	if len(o.selfhosted.Host) > 0 {
 		o.Client.Selfhosted[o.selfhosted.Host] = &o.selfhosted
 	}
+
 	if !validSelfHostedOpts(o) {
 		panic(fmt.Errorf("invalid self hosted configuration"))
 	}

cmd/app/options_test.go

@@ -2,9 +2,10 @@ package app
 
 import (
 	"os"
-	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	"github.com/jetstack/version-checker/pkg/client"
 	"github.com/jetstack/version-checker/pkg/client/acr"
 	"github.com/jetstack/version-checker/pkg/client/docker"
@@ -181,10 +182,7 @@ func TestComplete(t *testing.T) {
 			o := new(Options)
 			o.complete()
 
-			if !reflect.DeepEqual(o.Client, test.expOptions) {
-				t.Errorf("unexpected client options, exp=%#+v got=%#+v",
-					test.expOptions, o.Client)
-			}
+			assert.Exactly(t, test.expOptions, o.Client)
 		})
 	}
 }
@@ -201,7 +199,7 @@ func TestInvalidSelfhostedPanic(t *testing.T) {
 	}
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			defer func() { recover() }()
+			defer func() { _ = recover() }()
 
 			o := new(Options)
 			o.assignSelfhosted(test.envs)
@@ -236,10 +234,7 @@ func TestInvalidSelfhostedOpts(t *testing.T) {
 
 			valid := validSelfHostedOpts(&test.opts)
 
-			if !reflect.DeepEqual(test.valid, valid) {
-				t.Errorf("unexpected selfhosted valid options, exp=%#+v got=%#+v",
-					test.valid, valid)
-			}
+			assert.Equal(t, test.valid, valid)
 		})
 	}
 }
@@ -360,10 +355,7 @@ func TestAssignSelfhosted(t *testing.T) {
 			o := new(Options)
 			o.assignSelfhosted(test.envs)
 
-			if !reflect.DeepEqual(o.Client.Selfhosted, test.expOptions.Selfhosted) {
-				t.Errorf("unexpected client selfhosted options, exp=%#+v got=%#+v",
-					test.expOptions.Selfhosted, o.Client.Selfhosted)
-			}
+			assert.Exactly(t, test.expOptions.Selfhosted, o.Client.Selfhosted)
 		})
 	}
 }