Fail if one pipeline has StatusCheck set to true and another has it s…

…et to false

docs/content/en/docs/workflows/ci-cd.md

@@ -36,6 +36,10 @@ healthy before proceeding with the next steps in the pipeline.
 `healthcheck` is enabled by default; it can be disabled with the `--status-check=false`
 flag, or by setting the `statusCheck` field of the deployment config stanza in
 the `skaffold.yaml` to false.
+
+If the `skaffold.yaml` contains multiple pipelines, it is invalid for one to
+have `statusCheck` explicitly set to `true` and a second to have `statusCheck`
+explicitly set to `false`.
 {{</alert>}}
 
 To determine if a `Deployment` resource is up and running, Skaffold relies on `kubectl rollout status` to obtain its status.

pkg/skaffold/runner/deploy.go

@@ -201,7 +201,11 @@ func failIfClusterIsNotReachable() error {
 
 func (r *SkaffoldRunner) performStatusCheck(ctx context.Context, out io.Writer) error {
 	// Check if we need to perform deploy status
-	if !r.runCtx.StatusCheck() {
+	enabled, err := r.runCtx.StatusCheck()
+	if err != nil {
+		return err
+	}
+	if enabled != nil && !*enabled {
 		return nil
 	}
 

pkg/skaffold/runner/runcontext/context.go

@@ -110,16 +110,26 @@ func (ps Pipelines) TestCases() []*latest.TestCase {
 	return tests
 }
 
-func (ps Pipelines) StatusCheck() *bool {
-	var sc *bool
-	// set the group status check to disabled if any pipeline has StatusCheck
-	// set to false.
+func (ps Pipelines) StatusCheck() (*bool, error) {
+	var enabled, disabled bool
 	for _, p := range ps.pipelines {
-		if p.Deploy.StatusCheck != nil && !*p.Deploy.StatusCheck {
-			sc = util.BoolPtr(false)
+		if p.Deploy.StatusCheck != nil {
+			if *p.Deploy.StatusCheck {
+				enabled = true
+			} else {
+				disabled = true
+			}
 		}
 	}
-	return sc
+	if enabled && disabled {
+		return nil, fmt.Errorf("cannot explicitly enable StatusCheck in one pipeline and explicitly disable it in another pipeline, see https://skaffold.dev/docs/workflows/ci-cd/#waiting-for-skaffold-deployments-using-healthcheck")
+	}
+	// set the group status check to disabled if any pipeline has StatusCheck
+	// set to false.
+	if disabled {
+		return util.BoolPtr(false), nil
+	}
+	return util.BoolPtr(true), nil
 }
 
 func (ps Pipelines) StatusCheckDeadlineSeconds() int {
@@ -158,16 +168,19 @@ 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 {
+func (rc *RunContext) StatusCheck() (*bool, error) {
 	scOpts := rc.Opts.StatusCheck.Value()
-	scConfig := rc.Pipelines.StatusCheck()
+	scConfig, err := rc.Pipelines.StatusCheck()
+	if err != nil {
+		return nil, err
+	}
 	switch {
 	case scOpts != nil:
-		return *scOpts
+		return util.BoolPtr(*scOpts), nil
 	case scConfig != nil:
-		return *scConfig
+		return util.BoolPtr(*scConfig), nil
 	default:
-		return true
+		return util.BoolPtr(true), nil
 	}
 }
 

pkg/skaffold/runner/runcontext/context_test.go

@@ -19,6 +19,8 @@ package runcontext
 import (
 	"testing"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -89,3 +91,60 @@ func TestRunContext_UpdateNamespaces(t *testing.T) {
 		})
 	}
 }
+
+func TestPipelines_StatusCheck(t *testing.T) {
+	tests := []struct {
+		description   string
+		statusCheckP1 *bool
+		statusCheckP2 *bool
+		wantEnabled   *bool
+		wantErr       bool
+	}{
+		{
+			description: "both pipelines' statusCheck values are unspecified",
+			wantEnabled: util.BoolPtr(true),
+		},
+		{
+			description:   "one pipeline's statusCheck value is true",
+			statusCheckP1: util.BoolPtr(true),
+			wantEnabled:   util.BoolPtr(true),
+		},
+		{
+			description:   "one pipeline's statusCheck value is false",
+			statusCheckP1: util.BoolPtr(false),
+			wantEnabled:   util.BoolPtr(false),
+		},
+		{
+			description:   "one pipeline's statusCheck value is true, one pipeline's statusCheck value is false",
+			statusCheckP1: util.BoolPtr(true),
+			statusCheckP2: util.BoolPtr(false),
+			wantErr:       true,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			p := &Pipelines{
+				pipelines: []latest.Pipeline{
+					{
+						Deploy: latest.DeployConfig{
+							StatusCheck: test.statusCheckP1,
+						},
+					},
+					{
+						Deploy: latest.DeployConfig{
+							StatusCheck: test.statusCheckP2,
+						},
+					},
+				},
+			}
+			gotEnabled, err := p.StatusCheck()
+			if err != nil && !test.wantErr {
+				t.Errorf("p.StatusCheck() got error %v, want no error", err)
+			}
+			if err == nil && test.wantErr {
+				t.Errorf("p.StatusCheck() got no error, want error")
+			}
+			t.CheckDeepEqual(test.wantEnabled, gotEnabled)
+		})
+	}
+}