Merge pull request #2930 from tejal29/fix_status_check_err_msg

Change final status check error message to be more concise.
GoogleContainerTools · Sep 26, 2019 · 8adf254 · 8adf254
2 parents 667bd76 + c6a349d
commit 8adf254
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 64 deletions.
diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go
@@ -53,6 +53,7 @@ const (
 type counter struct {
 	total   int
 	pending int32
+	failed  int32
 }
 
 func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext, out io.Writer) error {
@@ -76,7 +77,7 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *
 		go func(r Resource) {
 			defer wg.Done()
 			pollResourceStatus(ctx, runCtx, r)
-			pending := c.markProcessed()
+			pending := c.markProcessed(r.Status().Error())
 			printStatusCheckSummary(out, r, pending, c.total)
 		}(d)
 	}
@@ -88,7 +89,7 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *
 
 	// Wait for all deployment status to be fetched
 	wg.Wait()
-	return getSkaffoldDeployStatus(deployments)
+	return getSkaffoldDeployStatus(c)
 }
 
 func getDeployments(client kubernetes.Interface, ns string, l *DefaultLabeller, deadlineDuration time.Duration) ([]Resource, error) {
@@ -133,17 +134,11 @@ func pollResourceStatus(ctx context.Context, runCtx *runcontext.RunContext, r Re
 	}
 }
 
-func getSkaffoldDeployStatus(resources []Resource) error {
-	var errorStrings []string
-	for _, r := range resources {
-		if err := r.Status().Error(); err != nil {
-			errorStrings = append(errorStrings, fmt.Sprintf("resource %s failed due to %s", r, err.Error()))
-		}
-	}
-	if len(errorStrings) == 0 {
+func getSkaffoldDeployStatus(c *counter) error {
+	if c.failed == 0 {
 		return nil
 	}
-	return fmt.Errorf("following resources are not stable:\n%s", strings.Join(errorStrings, "\n"))
+	return fmt.Errorf("%d/%d deployment(s) failed", c.failed, c.total)
 }
 
 func getDeadline(d int) time.Duration {
@@ -217,6 +212,9 @@ func newCounter(i int) *counter {
 	}
 }
 
-func (c *counter) markProcessed() int {
+func (c *counter) markProcessed(err error) int {
+	if err != nil {
+		atomic.AddInt32(&c.failed, 1)
+	}
 	return int(atomic.AddInt32(&c.pending, -1))
 }
diff --git a/pkg/skaffold/deploy/status_check_test.go b/pkg/skaffold/deploy/status_check_test.go
@@ -251,73 +251,39 @@ func TestPollResourceStatus(t *testing.T) {
 
 func TestGetDeployStatus(t *testing.T) {
 	tests := []struct {
-		description    string
-		deps           []Resource
-		expectedErrMsg []string
-		shouldErr      bool
+		description string
+		counter     *counter
+		expected    string
+		shouldErr   bool
 	}{
 		{
 			description: "one error",
-			deps: []Resource{
-				withStatus(
-					resource.NewDeployment("dep1", "test", time.Second),
-					"success",
-					nil,
-				),
-				withStatus(
-					resource.NewDeployment("dep2", "test", time.Second),
-					"error",
-					errors.New("could not return within default timeout"),
-				),
-			},
-			expectedErrMsg: []string{"dep2 failed due to could not return within default timeout"},
-			shouldErr:      true,
+			counter:     &counter{total: 2, failed: 1},
+			expected:    "1/2 deployment(s) failed",
+			shouldErr:   true,
 		},
 		{
 			description: "no error",
-			deps: []Resource{
-				withStatus(
-					resource.NewDeployment("dep1", "test", time.Second),
-					"success",
-					nil,
-				),
-				withStatus(resource.NewDeployment("dep2", "test", time.Second),
-					"running",
-					nil,
-				),
-			},
+			counter:     &counter{total: 2},
 		},
 		{
 			description: "multiple errors",
-			deps: []Resource{
-				withStatus(
-					resource.NewDeployment("dep1", "test", time.Second),
-					"success",
-					nil,
-				),
-				withStatus(
-					resource.NewDeployment("dep2", "test", time.Second),
-					"error",
-					errors.New("could not return within default timeout"),
-				),
-				withStatus(
-					resource.NewDeployment("dep3", "test", time.Second),
-					"error",
-					errors.New("ERROR"),
-				),
-			},
-			expectedErrMsg: []string{"dep2 failed due to could not return within default timeout",
-				"dep3 failed due to ERROR"},
-			shouldErr: true,
+			counter:     &counter{total: 3, failed: 2},
+			expected:    "2/3 deployment(s) failed",
+			shouldErr:   true,
+		},
+		{
+			description: "0 deployments",
+			counter:     &counter{},
 		},
 	}
 
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			err := getSkaffoldDeployStatus(test.deps)
+			err := getSkaffoldDeployStatus(test.counter)
 			t.CheckError(test.shouldErr, err)
-			for _, msg := range test.expectedErrMsg {
-				t.CheckErrorContains(msg, err)
+			if test.shouldErr {
+				t.CheckErrorContains(test.expected, err)
 			}
 		})
 	}