From d6569338df293a0694fa3739fcf45de72fbfeb66 Mon Sep 17 00:00:00 2001
From: Tejal Desai <tejaldesai@google.com>
Date: Mon, 16 Sep 2019 13:47:04 -0700
Subject: [PATCH] Small refactors

1. Refactor signature
2. Move `Deployment.WithStatus` to `withStatus` since it was used for
testing.
---
 pkg/skaffold/deploy/resource/deployment.go |  6 --
 pkg/skaffold/deploy/status_check.go        |  4 +-
 pkg/skaffold/deploy/status_check_test.go   | 72 ++++++++++++++++------
 3 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/pkg/skaffold/deploy/resource/deployment.go b/pkg/skaffold/deploy/resource/deployment.go
index a393ad73687..b6b854d064a 100644
--- a/pkg/skaffold/deploy/resource/deployment.go
+++ b/pkg/skaffold/deploy/resource/deployment.go
@@ -82,9 +82,3 @@ func NewDeployment(name string, ns string, deadline time.Duration) *Deployment {
 		status:    newStatus("", nil),
 	}
 }
-
-// For testing
-func (d *Deployment) WithStatus(details string, err error) *Deployment {
-	d.UpdateStatus(details, err)
-	return d
-}
diff --git a/pkg/skaffold/deploy/status_check.go b/pkg/skaffold/deploy/status_check.go
index b1cd4664807..fb16cd826d9 100644
--- a/pkg/skaffold/deploy/status_check.go
+++ b/pkg/skaffold/deploy/status_check.go
@@ -81,7 +81,7 @@ func StatusCheck(ctx context.Context, defaultLabeller *DefaultLabeller, runCtx *
 			defer wg.Done()
 			pollDeploymentRolloutStatus(ctx, kubectl.NewFromRunContext(runCtx), d)
 			pending := c.markProcessed()
-			printStatusCheckSummary(d, pending, c.total, out)
+			printStatusCheckSummary(out, d, pending, c.total)
 		}(d)
 	}
 
@@ -166,7 +166,7 @@ func getDeadline(d int) time.Duration {
 	return defaultStatusCheckDeadline
 }
 
-func printStatusCheckSummary(d *resource.Deployment, pending int, total int, out io.Writer) {
+func printStatusCheckSummary(out io.Writer, d *resource.Deployment, pending int, total int) {
 	status := fmt.Sprintf("%s %s", tabHeader, d)
 	if err := d.Status().Error(); err != nil {
 		status = fmt.Sprintf("%s failed.%s Error: %s.",
diff --git a/pkg/skaffold/deploy/status_check_test.go b/pkg/skaffold/deploy/status_check_test.go
index b69db95b8a8..04592269fd9 100644
--- a/pkg/skaffold/deploy/status_check_test.go
+++ b/pkg/skaffold/deploy/status_check_test.go
@@ -258,10 +258,16 @@ func TestGetDeployStatus(t *testing.T) {
 		{
 			description: "one error",
 			deps: []*resource.Deployment{
-				resource.NewDeployment("dep1", "test", time.Second).
-					WithStatus("success", nil),
-				resource.NewDeployment("dep2", "test", time.Second).
-					WithStatus("error", errors.New("could not return within default timeout")),
+				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,
@@ -269,21 +275,35 @@ func TestGetDeployStatus(t *testing.T) {
 		{
 			description: "no error",
 			deps: []*resource.Deployment{
-				resource.NewDeployment("dep1", "test", time.Second).
-					WithStatus("success", nil),
-				resource.NewDeployment("dep2", "test", time.Second).
-					WithStatus("running", nil),
+				withStatus(
+					resource.NewDeployment("dep1", "test", time.Second),
+					"success",
+					nil,
+				),
+				withStatus(resource.NewDeployment("dep2", "test", time.Second),
+					"running",
+					nil,
+				),
 			},
 		},
 		{
 			description: "multiple errors",
 			deps: []*resource.Deployment{
-				resource.NewDeployment("dep1", "test", time.Second).
-					WithStatus("success", nil),
-				resource.NewDeployment("dep2", "test", time.Second).
-					WithStatus("error", errors.New("could not return within default timeout")),
-				resource.NewDeployment("dep3", "test", time.Second).
-					WithStatus("error", errors.New("ERROR")),
+				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"},
@@ -385,10 +405,11 @@ func TestPrintSummaryStatus(t *testing.T) {
 		testutil.Run(t, test.description, func(t *testutil.T) {
 			out := new(bytes.Buffer)
 			printStatusCheckSummary(
-				resource.NewDeployment("dep", "test", 0).WithStatus("", test.err),
+				out,
+				withStatus(resource.NewDeployment("dep", "test", 0), "", test.err),
 				int(test.pending),
 				10,
-				out)
+			)
 			t.CheckDeepEqual(test.expected, out.String())
 		})
 	}
@@ -431,8 +452,11 @@ func TestPrintStatus(t *testing.T) {
 					"succes",
 					nil,
 				),
-				resource.NewDeployment("r2", "test", 1).
-					WithStatus("pending", nil),
+				withStatus(
+					resource.NewDeployment("r2", "test", 1),
+					"pending",
+					nil,
+				),
 			},
 			expectedOut: " - test:deployment/r2 pending\n",
 		},
@@ -444,8 +468,11 @@ func TestPrintStatus(t *testing.T) {
 					"succes",
 					nil,
 				),
-				resource.NewDeployment("r2", "test", 1).
-					WithStatus("", fmt.Errorf("context deadline expired")),
+				withStatus(
+					resource.NewDeployment("r2", "test", 1),
+					"",
+					fmt.Errorf("context deadline expired"),
+				),
 			},
 			expectedOut: " - test:deployment/r2 context deadline expired\n",
 		},
@@ -466,3 +493,8 @@ func withDone(d *resource.Deployment, details string, err error) *resource.Deplo
 	d.MarkDone()
 	return d
 }
+
+func withStatus(d *resource.Deployment, details string, err error) *resource.Deployment {
+	d.UpdateStatus(details, err)
+	return d
+}