Add support for nillable flag values to fix --default-repo (#5654)

cmd/skaffold/app/cmd/flags.go

@@ -34,6 +34,13 @@ var (
 	fromBuildOutputFile flags.BuildOutputFileFlag
 )
 
+// Nillable is used to reset objects that implement pflag's `Value` and `SliceValue`.
+// Some flags, like `--default-repo`, use nil to indicate that they are unset, which
+// is different from the empty string.
+type Nillable interface {
+	SetNil() error
+}
+
 // Flag defines a Skaffold CLI flag which contains a list of
 // subcommands the flag belongs to in `DefinedOn` field.
 // See https://pkg.go.dev/github.com/spf13/pflag#Flag
@@ -100,7 +107,7 @@ var flagRegistry = []Flag{
 		Shorthand:     "d",
 		Usage:         "Default repository value (overrides global config)",
 		Value:         &opts.DefaultRepo,
-		DefValue:      "",
+		DefValue:      nil,
 		FlagAddMethod: "Var",
 		DefinedOn:     []string{"dev", "run", "debug", "deploy", "render", "build", "delete"},
 	},
@@ -574,7 +581,9 @@ func setDefaultValues(v interface{}, fl *Flag, cmdName string) {
 	if !found {
 		d = fl.DefValue
 	}
-	if sv, ok := v.(pflag.SliceValue); ok {
+	if nv, ok := v.(Nillable); ok && d == nil {
+		nv.SetNil()
+	} else 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))

cmd/skaffold/app/cmd/flags_test.go

@@ -22,6 +22,7 @@ import (
 
 	"github.com/spf13/cobra"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -124,6 +125,7 @@ func TestMakeFlag(t *testing.T) {
 func TestResetFlagDefaults(t *testing.T) {
 	var v string
 	var sl []string
+	var soru config.StringOrUndefined
 
 	valueFlag := Flag{
 		Name:          "value",
@@ -147,16 +149,28 @@ func TestResetFlagDefaults(t *testing.T) {
 		},
 		DefinedOn: []string{"build", "debug", "test"},
 	}
-	flagRegistry := []*Flag{&valueFlag, &sliceFlag}
+	varFlag := Flag{
+		Name:          "var",
+		Value:         &soru,
+		FlagAddMethod: "Var",
+		DefValue:      nil,
+		DefValuePerCommand: map[string]interface{}{
+			"debug": "dbg",
+			"build": "bld",
+		},
+		DefinedOn: []string{"build", "debug", "test"},
+	}
+	flagRegistry := []*Flag{&valueFlag, &sliceFlag, &varFlag}
 
 	tests := []struct {
 		command       string
 		expectedValue string
 		expectedSlice []string
+		expectedVar   interface{}
 	}{
-		{"test", "default", []string{"default"}},
-		{"debug", "dbg", []string{"dbg", "other"}},
-		{"build", "bld", []string{"bld"}},
+		{"debug", "dbg", []string{"dbg", "other"}, "dbg"},
+		{"test", "default", []string{"default"}, nil},
+		{"build", "bld", []string{"bld"}, "bld"},
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.command, func(t *testutil.T) {
@@ -168,10 +182,16 @@ func TestResetFlagDefaults(t *testing.T) {
 			// ResetFlagDefaults should reset to defaults for the given command
 			v = "randovalue"
 			sl = []string{"rando", "value"}
+			soru.Set("randovalue")
 			ResetFlagDefaults(&cmd, flagRegistry)
 
-			t.CheckDeepEqual(v, test.expectedValue)
-			t.CheckDeepEqual(sl, test.expectedSlice)
+			t.CheckDeepEqual(test.expectedValue, v)
+			t.CheckDeepEqual(test.expectedSlice, sl)
+			if test.expectedVar == nil {
+				t.CheckDeepEqual((*string)(nil), soru.Value())
+			} else {
+				t.CheckDeepEqual(soru.String(), test.expectedVar)
+			}
 		})
 	}
 }

pkg/skaffold/config/types.go

@@ -16,7 +16,9 @@ limitations under the License.
 
 package config
 
-import "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
+import (
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
+)
 
 // StringOrUndefined holds the value of a flag of type `string`,
 // that's by default `undefined`.
@@ -39,6 +41,11 @@ func (s *StringOrUndefined) Set(v string) error {
 	return nil
 }
 
+func (s *StringOrUndefined) SetNil() error {
+	s.value = nil
+	return nil
+}
+
 func (s *StringOrUndefined) String() string {
 	if s.value == nil {
 		return ""

pkg/skaffold/config/types_test.go

@@ -38,6 +38,16 @@ func TestStringOrUndefinedUsage(t *testing.T) {
 	testutil.CheckDeepEqual(t, "Usage:\n\nFlags:\n      --flag string   use it like this\n", output.String())
 }
 
+func TestStringOrUndefined_SetNil(t *testing.T) {
+	var s StringOrUndefined
+	s.Set("hello")
+	testutil.CheckDeepEqual(t, "hello", s.String())
+	s.SetNil()
+	testutil.CheckDeepEqual(t, "", s.String())
+	testutil.CheckDeepEqual(t, (*string)(nil), s.value)
+	testutil.CheckDeepEqual(t, (*string)(nil), s.Value())
+}
+
 func TestStringOrUndefined(t *testing.T) {
 	tests := []struct {
 		description string