some error handling and fixing output

pkg/skaffold/deploy/resources/deployment.go

@@ -26,7 +26,9 @@ import (
 )
 
 const (
-	DeploymentType = "deployment"
+	DeploymentType    = "deployment"
+	KubectlKilled     = "Killed"
+	KubectlConnection = "KubectlConnection"
 )
 
 type Deployment struct {
@@ -45,8 +47,9 @@ func (d *Deployment) CheckStatus(ctx context.Context, runCtx *runcontext.RunCont
 	kubeCtl := kubectl.NewFromRunContext(runCtx)
 	b, err := kubeCtl.RunOut(ctx, "rollout", "status", "deployment", d.name, "--namespace", d.namespace, "--watch=false")
 	if err != nil {
-		d.UpdateStatus("", err.Error(), err)
-		if !strings.Contains(err.Error(), "Unable to connect to the server") {
+		reason, details := parseKubectlError(err.Error())
+		d.UpdateStatus(details, reason, err)
+		if reason != KubectlConnection {
 			d.checkComplete()
 		}
 		return
@@ -64,6 +67,17 @@ func (d *Deployment) Deadline() time.Duration {
 }
 
 func (d *Deployment) WithError(err error) *Deployment {
-	d.UpdateStatus("", "", err)
+	d.UpdateStatus("", err.Error(), err)
 	return d
 }
+
+func parseKubectlError(errMsg string) (string, string) {
+	errMsg = strings.TrimSuffix(errMsg, "\n")
+	if strings.Contains(errMsg, "Unable to connect to the server") {
+		return KubectlConnection, errMsg
+	}
+	if strings.Contains(errMsg, "signal: killed") {
+		return KubectlKilled, "kubectl killed due to timeout"
+	}
+	return errMsg, errMsg
+}

pkg/skaffold/deploy/resources/deployment_test.go

@@ -61,7 +61,7 @@ func TestDeploymentCheckStatus(t *testing.T) {
 			description: "rollout kubectl client connection error",
 			command: testutil.NewFakeCmd(t).
 				WithRunOutErr(rolloutCmd, "", fmt.Errorf("Unable to connect to the server")),
-			expectedReason: "Unable to connect to the server",
+			expectedReason: "KubectlConnection",
 		},
 	}
 

pkg/skaffold/deploy/resources/resource.go

@@ -19,11 +19,16 @@ package resources
 import (
 	"fmt"
 	"io"
+	"strings"
 	"time"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
 )
 
+const (
+	TabHeader = " -"
+)
+
 type ResourceObj struct {
 	name      string
 	namespace string
@@ -43,8 +48,8 @@ func (r *ResourceObj) UpdateStatus(msg string, reason string, err error) {
 	newStatus := Status{details: msg, reason: reason, err: err}
 	if !r.status.Equal(&newStatus) {
 		r.status.err = err
-		r.status.details = msg
-		r.status.reason = reason
+		r.status.details = strings.TrimSuffix(msg, "\n")
+		r.status.reason = strings.TrimSuffix(reason, "\n")
 		r.status.lastUpdated = time.Now().UnixNano()
 	}
 }
@@ -74,7 +79,7 @@ func (r *ResourceObj) ReportSinceLastUpdated(out io.Writer) {
 		return
 	}
 	r.status.lastReported = time.Now().UnixNano()
-	color.Default.Fprintln(out, fmt.Sprintf("%s %s", r.String(), r.status.String()))
+	color.Default.Fprintln(out, fmt.Sprintf("%s %s %s", TabHeader, r.String(), r.status.String()))
 }
 
 type Status struct {
@@ -91,13 +96,18 @@ func (rs *Status) Error() error {
 }
 
 func (rs *Status) Equal(other *Status) bool {
-	return rs.reason == other.reason && rs.err == other.err
-
+	if rs.reason == other.reason {
+		return true
+	}
+	if rs.err != nil && other.err != nil {
+		return rs.err.Error() == other.err.Error()
+	}
+	return false
 }
 
 func (rs *Status) String() string {
 	if rs.err != nil {
-		return fmt.Sprintf("is pending due to %s", rs.err.Error())
+		return fmt.Sprintf("is pending due to %s", strings.TrimSuffix(rs.err.Error(), "\n"))
 	}
 	return fmt.Sprintf("is pending due to %s", rs.details)
 }

pkg/skaffold/deploy/resources/resource_test.go

@@ -53,12 +53,13 @@ func TestReportSinceLastUpdated(t *testing.T) {
 	}{
 		{
 			description: "updating an error status",
+			message:     "cannot pull image",
 			err:         fmt.Errorf("cannot pull image"),
-			expected:    "deployment/test is pending due to cannot pull image\n",
+			expected:    " - deployment/test is pending due to cannot pull image\n",
 		}, {
 			description: "updating a non error status",
 			message:     "is waiting for container",
-			expected:    "deployment/test is pending due to is waiting for container\n",
+			expected:    " - deployment/test is pending due to is waiting for container\n",
 		},
 	}
 	for _, test := range tests {
@@ -81,7 +82,7 @@ func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) {
 		{
 			description: "report first time should write to out",
 			times:       1,
-			expected:    "deployment/test is pending due to cannot pull image\n",
+			expected:    " - deployment/test is pending due to cannot pull image\n",
 		}, {
 			description: "report 2nd time should not write to out",
 			times:       2,
@@ -117,24 +118,24 @@ func TestUpdateStatus(t *testing.T) {
 			description: "updateTimestamp should not change for same statuses",
 			old:         Status{details: "same", reason: "same", err: nil},
 			new:         Status{details: "same", reason: "same", err: nil},
-		}, {
-			description: "updateTimestamp should not change for same statuses if details change",
-			old:         Status{details: "same", reason: "same", err: nil},
-			new:         Status{details: "another", reason: "same", err: nil},
 		}, {
 			description:  "updateTimestamp should change if reason change",
 			old:          Status{details: "same", reason: "same", err: nil},
 			new:          Status{details: "same", reason: "another", err: nil},
 			expectChange: true,
 		}, {
-			description:  "updateTimestamp should change if error change",
+			description: "updateTimestamp should not change if reason son",
+			old:         Status{details: "same", reason: "same", err: nil},
+			new:         Status{details: "same", reason: "same", err: fmt.Errorf("see this error")},
+		}, {
+			description:  "updateTimestamp should change if reason and err change",
 			old:          Status{details: "same", reason: "same", err: nil},
-			new:          Status{details: "same", reason: "same", err: fmt.Errorf("see this error")},
+			new:          Status{details: "same", reason: "another", err: fmt.Errorf("see this error")},
 			expectChange: true,
 		}, {
-			description:  "updateTimestamp should change if both reason and error change",
+			description:  "updateTimestamp should change if both reason and details change",
 			old:          Status{details: "same", reason: "same", err: nil},
-			new:          Status{reason: "error", err: fmt.Errorf("cannot pull image")},
+			new:          Status{details: "error", reason: "error", err: nil},
 			expectChange: true,
 		},
 	}

pkg/skaffold/deploy/status_check.go

@@ -57,9 +57,7 @@ type Checker struct {
 	labeller      *DefaultLabeller
 	client        kubernetes.Interface
 	numDeps       int
-	numPods       int
 	processedDeps int32
-	processedPods int32
 }
 
 func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *runcontext.RunContext, out io.Writer) error {
@@ -105,7 +103,7 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *
 
 	// Retrieve pending resource states
 	go func() {
-		checker.printResourceStatus(rs)
+		checker.printResourceStatus(rs, deadline)
 	}()
 
 	// Wait for all resource status to be fetched
@@ -158,11 +156,13 @@ func (c *Checker) CheckResourceStatus(r Resource) {
 }
 
 // Print resource statuses until all status check are completed or context is cancelled.
-func (c *Checker) printResourceStatus(rs []Resource) {
+func (c *Checker) printResourceStatus(rs []Resource, deadline int32) {
+	timeoutContext, cancel := context.WithTimeout(c.context, time.Duration(deadline)*time.Second)
+	defer cancel()
 	for {
 		allResourcesCheckComplete := true
 		select {
-		case <-c.context.Done():
+		case <-timeoutContext.Done():
 			return
 		case <-time.After(reportStatusTime):
 			for _, r := range rs {
@@ -201,11 +201,11 @@ func (c *Checker) printStatusCheckSummary(r Resource) {
 			fmt.Sprintf("%s failed %s. Error: %s.",
 				r.String(),
 				waitingMsg,
-				strings.TrimRight(err.Error(), "\n"),
+				strings.TrimSuffix(err.Error(), "\n"),
 			),
 		)
 	} else {
-		color.Default.Fprintln(c.out, fmt.Sprintf("%s is ready. %s", r.String(), waitingMsg))
+		color.Default.Fprintln(c.out, fmt.Sprintf("%s %s is ready. %s", resources.TabHeader, r.String(), waitingMsg))
 	}
 }
 

pkg/skaffold/deploy/status_check_test.go

@@ -29,7 +29,6 @@ import (
 	fakekubeclientset "k8s.io/client-go/kubernetes/fake"
 	utilpointer "k8s.io/utils/pointer"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -42,7 +41,7 @@ func TestFetchDeployments(t *testing.T) {
 		shouldErr   bool
 	}{
 		{
-			description: "multiple deployments in same namespace",
+			description: "multiple resources.Deployments in same namespace",
 			deps: []*appsv1.Deployment{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -71,7 +70,7 @@ func TestFetchDeployments(t *testing.T) {
 				resources.NewDeployment("dep2", "test", 20*time.Second),
 			},
 		}, {
-			description: "command flag deadline is less than deployment spec.",
+			description: "command flag deadline is less than resources.Deployment spec.",
 			deps: []*appsv1.Deployment{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -88,7 +87,7 @@ func TestFetchDeployments(t *testing.T) {
 			expected: []*resources.Deployment{resources.NewDeployment("dep1", "test", 200*time.Second)},
 		},
 		{
-			description: "multiple deployments with 1 no progress deadline set",
+			description: "multiple resources.Deployments with 1 no progress deadline set",
 			deps: []*appsv1.Deployment{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -116,11 +115,11 @@ func TestFetchDeployments(t *testing.T) {
 			},
 		},
 		{
-			description: "no deployments",
+			description: "no resources.Deployments",
 			expected:    []*resources.Deployment{},
 		},
 		{
-			description: "multiple deployments in different namespaces",
+			description: "multiple resources.Deployments in different namespaces",
 			deps: []*appsv1.Deployment{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -148,7 +147,7 @@ func TestFetchDeployments(t *testing.T) {
 			},
 		},
 		{
-			description: "deployment in correct namespace but not deployed by skaffold",
+			description: "resources.Deployment in correct namespace but not deployed by skaffold",
 			deps: []*appsv1.Deployment{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -164,7 +163,7 @@ func TestFetchDeployments(t *testing.T) {
 			expected: []*resources.Deployment{},
 		},
 		{
-			description: "deployment in correct namespace deployed by skaffold but different run",
+			description: "resources.Deployment in correct namespace deployed by skaffold but different run",
 			deps: []*appsv1.Deployment{
 				{
 					ObjectMeta: metav1.ObjectMeta{
@@ -234,37 +233,3 @@ func TestIsSkaffoldDeployInError(t *testing.T) {
 		})
 	}
 }
-
-func TestCheckResourceStatus(t *testing.T) {
-	rolloutCmd := "kubectl --context kubecontext --namespace test rollout status deployment dep --watch=false"
-	var tests = []struct {
-		description string
-		command     util.Command
-		expected    string
-		shouldErr   bool
-	}{
-		{
-			description: "some output",
-			command: testutil.NewFakeCmd(t).
-				WithRunOut(rolloutCmd, "Waiting for replicas to be available"),
-			expected: "Waiting for replicas to be available",
-		},
-		{
-			description: "no output",
-			command: testutil.NewFakeCmd(t).
-				WithRunOut(rolloutCmd, ""),
-		},
-		{
-			description: "rollout status error",
-			command: testutil.NewFakeCmd(t).
-				WithRunOutErr(rolloutCmd, "", fmt.Errorf("error")),
-			shouldErr: true,
-		},
-	}
-
-	for _, test := range tests {
-		testutil.Run(t, test.description, func(t *testutil.T) {
-
-		})
-	}
-}

pkg/skaffold/runner/deploy.go

@@ -19,6 +19,7 @@ package runner
 import (
 	"context"
 	"io"
+	"time"
 
 	"github.com/pkg/errors"
 
@@ -47,12 +48,13 @@ func (r *SkaffoldRunner) Deploy(ctx context.Context, out io.Writer, artifacts []
 func (r *SkaffoldRunner) performStatusCheck(ctx context.Context, out io.Writer) error {
 	// Check if we need to perform deploy status
 	if r.runCtx.Opts.StatusCheck {
+		start := time.Now()
 		color.Default.Fprintln(out, "Waiting for deployments to stabilize")
 		err := statusCheck(ctx, r.defaultLabeller, r.runCtx, out)
 		if err != nil {
-			color.Default.Fprintln(out, err.Error())
+			return err
 		}
-		return err
+		color.Default.Fprintln(out, "Deployments stabilized in Xs", time.Since(start))
 	}
 	return nil
 }