Make status-check flag nillable (#5712)

* Make status-check flag nillable

* Address code review comments

Author: maggieneterval

cmd/skaffold/app/cmd/flags.go

@@ -277,9 +277,10 @@ var flagRegistry = []Flag{
 		Usage:         "Wait for deployed resources to stabilize",
 		Value:         &opts.StatusCheck,
 		DefValue:      true,
-		FlagAddMethod: "BoolVar",
+		FlagAddMethod: "Var",
 		DefinedOn:     []string{"dev", "debug", "deploy", "run", "apply"},
 		IsEnum:        true,
+		NoOptDefVal:   "true",
 	},
 	{
 		Name:          "render-only",

cmd/skaffold/app/cmd/init_test.go

@@ -157,7 +157,7 @@ func TestFlagsToConfigVersion(t *testing.T) {
 
 			// we ignore Skaffold options
 			test.expectedConfig.Opts = capturedConfig.Opts
-			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedConfig, capturedConfig, cmp.AllowUnexported(cfg.StringOrUndefined{}))
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedConfig, capturedConfig, cmp.AllowUnexported(cfg.StringOrUndefined{}, cfg.BoolOrUndefined{}))
 		})
 	}
 }

pkg/skaffold/config/options.go

@@ -49,7 +49,6 @@ type SkaffoldOptions struct {
 	Force                 bool
 	NoPrune               bool
 	NoPruneChildren       bool
-	StatusCheck           bool
 	AutoBuild             bool
 	AutoSync              bool
 	AutoDeploy            bool
@@ -68,6 +67,7 @@ type SkaffoldOptions struct {
 	DetectMinikube    bool
 	// Experimental is the entrypoint to run skaffold v3 before it's fully implemented.
 	Experimental bool
+	StatusCheck  BoolOrUndefined
 
 	PortForward        PortForwardOptions
 	CustomTag          string

pkg/skaffold/config/types.go

@@ -53,6 +53,54 @@ func (s *StringOrUndefined) String() string {
 	return *s.value
 }
 
+// BoolOrUndefined holds the value of a flag of type `bool`,
+// that's by default `undefined`.
+// We use this instead of just `bool` to differentiate `undefined`
+// and `false` values.
+type BoolOrUndefined struct {
+	value *bool
+}
+
+func (s *BoolOrUndefined) Type() string {
+	return "bool"
+}
+
+func (s *BoolOrUndefined) Value() *bool {
+	return s.value
+}
+
+func (s *BoolOrUndefined) Set(v string) error {
+	switch v {
+	case "true":
+		s.value = util.BoolPtr(true)
+	case "false":
+		s.value = util.BoolPtr(false)
+	default:
+		s.value = nil
+	}
+	return nil
+}
+
+func (s *BoolOrUndefined) SetNil() error {
+	s.value = nil
+	return nil
+}
+
+func (s *BoolOrUndefined) String() string {
+	b := s.value
+	if b == nil {
+		return ""
+	}
+	if *b {
+		return "true"
+	}
+	return "false"
+}
+
+func NewBoolOrUndefined(v *bool) BoolOrUndefined {
+	return BoolOrUndefined{value: v}
+}
+
 // Muted lists phases for which logs are muted.
 type Muted struct {
 	Phases []string

pkg/skaffold/config/types_test.go

@@ -84,6 +84,73 @@ func TestStringOrUndefined(t *testing.T) {
 	}
 }
 
+func TestBoolOrUndefinedUsage(t *testing.T) {
+	var output bytes.Buffer
+
+	cmd := &cobra.Command{}
+	cmd.Flags().Var(&BoolOrUndefined{}, "bool-flag", "use it like this")
+	cmd.SetOut(&output)
+	cmd.Usage()
+
+	testutil.CheckDeepEqual(t, "Usage:\n\nFlags:\n      --bool-flag   use it like this\n", output.String())
+}
+
+func TestBoolOrUndefined_SetNil(t *testing.T) {
+	var s BoolOrUndefined
+	s.Set("false")
+	testutil.CheckDeepEqual(t, "false", s.String())
+	s.SetNil()
+	testutil.CheckDeepEqual(t, "", s.String())
+	testutil.CheckDeepEqual(t, (*bool)(nil), s.value)
+	testutil.CheckDeepEqual(t, (*bool)(nil), s.Value())
+}
+
+func TestBoolOrUndefined(t *testing.T) {
+	tests := []struct {
+		description string
+		args        []string
+		expected    *bool
+	}{
+		{
+			description: "undefined",
+			args:        []string{},
+			expected:    nil,
+		},
+		{
+			description: "empty",
+			args:        []string{"--bool-flag="},
+			expected:    nil,
+		},
+		{
+			description: "invalid",
+			args:        []string{"--bool-flag=invalid"},
+			expected:    nil,
+		},
+		{
+			description: "true",
+			args:        []string{"--bool-flag=true"},
+			expected:    util.BoolPtr(true),
+		},
+		{
+			description: "false",
+			args:        []string{"--bool-flag=false"},
+			expected:    util.BoolPtr(false),
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			var flag BoolOrUndefined
+
+			cmd := &cobra.Command{}
+			cmd.Flags().Var(&flag, "bool-flag", "")
+			cmd.SetArgs(test.args)
+			cmd.Execute()
+
+			t.CheckDeepEqual(test.expected, flag.value)
+		})
+	}
+}
+
 func TestMuted(t *testing.T) {
 	tests := []struct {
 		phases                  []string

pkg/skaffold/runner/deploy_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/client"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -43,25 +44,32 @@ func TestDeploy(t *testing.T) {
 	tests := []struct {
 		description string
 		testBench   *TestBench
-		statusCheck bool
+		statusCheck config.BoolOrUndefined
 		shouldErr   bool
 		shouldWait  bool
 	}{
 		{
 			description: "deploy shd perform status check",
 			testBench:   &TestBench{},
-			statusCheck: true,
+			statusCheck: config.NewBoolOrUndefined(nil),
+			shouldWait:  true,
+		},
+		{
+			description: "deploy shd perform status check",
+			testBench:   &TestBench{},
+			statusCheck: config.NewBoolOrUndefined(util.BoolPtr(true)),
 			shouldWait:  true,
 		},
 		{
 			description: "deploy shd not perform status check",
 			testBench:   &TestBench{},
+			statusCheck: config.NewBoolOrUndefined(util.BoolPtr(false)),
 		},
 		{
 			description: "deploy shd not perform status check when deployer is in error",
 			testBench:   &TestBench{deployErrors: []error{errors.New("deploy error")}},
 			shouldErr:   true,
-			statusCheck: true,
+			statusCheck: config.NewBoolOrUndefined(util.BoolPtr(true)),
 		},
 	}
 

pkg/skaffold/runner/runcontext/context.go

@@ -146,6 +146,14 @@ func (rc *RunContext) Deployers() []latest.DeployType { return rc.Pipelines.Depl
 
 func (rc *RunContext) TestCases() []*latest.TestCase { return rc.Pipelines.TestCases() }
 
+func (rc *RunContext) StatusCheck() bool {
+	sc := rc.Opts.StatusCheck.Value()
+	if sc == nil {
+		return true
+	}
+	return *sc
+}
+
 func (rc *RunContext) StatusCheckDeadlineSeconds() int {
 	return rc.Pipelines.StatusCheckDeadlineSeconds()
 }
@@ -185,7 +193,6 @@ func (rc *RunContext) RenderOnly() bool                          { return rc.Opt
 func (rc *RunContext) RenderOutput() string                      { return rc.Opts.RenderOutput }
 func (rc *RunContext) SkipRender() bool                          { return rc.Opts.SkipRender }
 func (rc *RunContext) SkipTests() bool                           { return rc.Opts.SkipTests }
-func (rc *RunContext) StatusCheck() bool                         { return rc.Opts.StatusCheck }
 func (rc *RunContext) Tail() bool                                { return rc.Opts.Tail }
 func (rc *RunContext) Trigger() string                           { return rc.Opts.Trigger }
 func (rc *RunContext) WaitForDeletions() config.WaitForDeletions { return rc.Opts.WaitForDeletions }