Flag defaults should support pflag SliceValue (#5632)

* use non-reflection in flags, and set defaults

* Add support for setting default values for pflag SliceValues

* shift setting of defaults

* oops restore %v

* log fatal error for unhandled default value type

cmd/skaffold/app/cmd/flags.go

@@ -21,6 +21,7 @@ import (
 	"reflect"
 	"time"
 
+	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
@@ -517,8 +518,16 @@ func (fl *Flag) flag(cmdName string) *pflag.Flag {
 	if methodName == "" {
 		methodName = methodNameByType(reflect.ValueOf(fl.Value))
 	}
+	isVar := methodName == "Var"
+	// pflags' Var*() methods do not take a default value but instead
+	// assume the value is already set to its default value.  So we
+	// explicitly set the default value here to ensure help text is correct.
+	if isVar {
+		setDefaultValues(fl.Value, fl, cmdName)
+	}
+
 	inputs := []interface{}{fl.Value, fl.Name}
-	if methodName != "Var" {
+	if !isVar {
 		if d, found := fl.DefValuePerCommand[cmdName]; found {
 			inputs = append(inputs, d)
 		} else {
@@ -528,8 +537,8 @@ func (fl *Flag) flag(cmdName string) *pflag.Flag {
 	inputs = append(inputs, fl.Usage)
 
 	fs := pflag.NewFlagSet(fl.Name, pflag.ContinueOnError)
-
 	reflect.ValueOf(fs).MethodByName(methodName).Call(reflectValueOf(inputs))
+
 	f := fs.Lookup(fl.Name)
 	if len(fl.NoOptDefVal) > 0 {
 		// f.NoOptDefVal may be set depending on value type
@@ -540,37 +549,35 @@ func (fl *Flag) flag(cmdName string) *pflag.Flag {
 	return f
 }
 
-func reflectValueOf(values []interface{}) []reflect.Value {
-	var results []reflect.Value
-	for _, v := range values {
-		results = append(results, reflect.ValueOf(v))
-	}
-	return results
-}
-
 func ResetFlagDefaults(cmd *cobra.Command, flags []*Flag) {
 	// Update default values.
 	for _, fl := range flags {
 		flag := cmd.Flag(fl.Name)
 		if !flag.Changed {
-			defValue := fl.DefValue
-			if fl.DefValuePerCommand != nil {
-				if d, present := fl.DefValuePerCommand[cmd.Use]; present {
-					defValue = d
-				}
-			}
-			if sv, ok := flag.Value.(pflag.SliceValue); ok {
-				reflect.ValueOf(sv).MethodByName("Replace").Call(reflectValueOf([]interface{}{defValue}))
-			} else {
-				flag.Value.Set(fmt.Sprintf("%v", defValue))
-			}
+			setDefaultValues(flag.Value, fl, cmd.Name())
 		}
 		if fl.IsEnum {
 			instrumentation.AddFlag(flag)
 		}
 	}
 }
 
+// setDefaultValues sets the default value (or values) for the given flag definition.
+// This function handles pflag's SliceValue and Value interfaces.
+func setDefaultValues(v interface{}, fl *Flag, cmdName string) {
+	d, found := fl.DefValuePerCommand[cmdName]
+	if !found {
+		d = fl.DefValue
+	}
+	if sv, ok := v.(pflag.SliceValue); ok {
+		sv.Replace(asStringSlice(d))
+	} else if val, ok := v.(pflag.Value); ok {
+		val.Set(fmt.Sprintf("%v", d))
+	} else {
+		logrus.Fatalf("%s --%s: unhandled value type: %v (%T)", cmdName, fl.Name, v, v)
+	}
+}
+
 // AddFlags adds to the command the common flags that are annotated with the command name.
 func AddFlags(cmd *cobra.Command) {
 	var flagsForCommand []*Flag
@@ -613,3 +620,29 @@ func hasCmdAnnotation(cmdName string, annotations []string) bool {
 	}
 	return false
 }
+
+func reflectValueOf(values []interface{}) []reflect.Value {
+	var results []reflect.Value
+	for _, v := range values {
+		results = append(results, reflect.ValueOf(v))
+	}
+	return results
+}
+
+func asStringSlice(v interface{}) []string {
+	vt := reflect.TypeOf(v)
+	if vt == reflect.TypeOf([]string{}) {
+		return v.([]string)
+	}
+	switch vt.Kind() {
+	case reflect.Array, reflect.Slice:
+		value := reflect.ValueOf(v)
+		var slice []string
+		for i := 0; i < value.Len(); i++ {
+			slice = append(slice, fmt.Sprintf("%v", value.Index(i)))
+		}
+		return slice
+	default:
+		return []string{fmt.Sprintf("%v", v)}
+	}
+}

cmd/skaffold/app/cmd/flags_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package cmd
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -174,3 +175,22 @@ func TestResetFlagDefaults(t *testing.T) {
 		})
 	}
 }
+
+func TestAsStringSlice(t *testing.T) {
+	tests := []struct {
+		input    interface{}
+		expected []string
+	}{
+		{"string", []string{"string"}},
+		{0, []string{"0"}},
+		{[]string{"a", "b"}, []string{"a", "b"}},
+		{[]int{0, 1}, []string{"0", "1"}},
+	}
+	for _, test := range tests {
+		testutil.Run(t, fmt.Sprintf("%v", test.expected), func(t *testutil.T) {
+			result := asStringSlice(test.input)
+
+			t.CheckDeepEqual(test.expected, result)
+		})
+	}
+}