From eb5cbcb3af74c46670b6305a7f3aafd3b7a27a8a Mon Sep 17 00:00:00 2001 From: tejal29 Date: Tue, 19 May 2020 01:50:05 -0700 Subject: [PATCH 1/2] move filed reported to changed --- pkg/skaffold/deploy/resource/deployment.go | 16 +++++++++------- pkg/skaffold/deploy/resource/deployment_test.go | 5 +++-- pkg/skaffold/deploy/resource/status.go | 7 ++++--- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/pkg/skaffold/deploy/resource/deployment.go b/pkg/skaffold/deploy/resource/deployment.go index 474f3d21b10..6829c7cd02a 100644 --- a/pkg/skaffold/deploy/resource/deployment.go +++ b/pkg/skaffold/deploy/resource/deployment.go @@ -54,11 +54,14 @@ func (d *Deployment) Deadline() time.Duration { func (d *Deployment) UpdateStatus(details string, err error) { updated := newStatus(details, err) - if !d.status.Equal(updated) { - d.status = updated - if strings.Contains(details, rollOutSuccess) || isErrAndNotRetryAble(err) { - d.done = true - } + if d.status.Equal(updated) { + d.status.changed = false + return + } + d.status = updated + d.status.changed = true + if strings.Contains(details, rollOutSuccess) || isErrAndNotRetryAble(err) { + d.done = true } } @@ -111,10 +114,9 @@ func (d *Deployment) IsStatusCheckComplete() bool { } func (d *Deployment) ReportSinceLastUpdated() string { - if d.status.reported { + if !d.status.changed { return "" } - d.status.reported = true return fmt.Sprintf("%s: %s", d, d.status) } diff --git a/pkg/skaffold/deploy/resource/deployment_test.go b/pkg/skaffold/deploy/resource/deployment_test.go index d84cc45596e..e675766a225 100644 --- a/pkg/skaffold/deploy/resource/deployment_test.go +++ b/pkg/skaffold/deploy/resource/deployment_test.go @@ -206,7 +206,7 @@ func TestReportSinceLastUpdated(t *testing.T) { dep.UpdateStatus(test.message, test.err) t.CheckDeepEqual(test.expected, dep.ReportSinceLastUpdated()) - t.CheckTrue(dep.status.reported) + t.CheckTrue(dep.status.changed) }) } } @@ -231,9 +231,10 @@ func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { dep := NewDeployment("test", "test-ns", 1) - dep.UpdateStatus("cannot pull image", nil) var actual string for i := 0; i < test.times; i++ { + // update to same status + dep.UpdateStatus("cannot pull image", nil) actual = dep.ReportSinceLastUpdated() } t.CheckDeepEqual(test.expected, actual) diff --git a/pkg/skaffold/deploy/resource/status.go b/pkg/skaffold/deploy/resource/status.go index f048923a894..05e041ecb39 100644 --- a/pkg/skaffold/deploy/resource/status.go +++ b/pkg/skaffold/deploy/resource/status.go @@ -17,9 +17,9 @@ limitations under the License. package resource type Status struct { - err error - details string - reported bool + err error + details string + changed bool } func (rs Status) Error() error { @@ -47,5 +47,6 @@ func newStatus(msg string, err error) Status { return Status{ details: msg, err: err, + changed: true, } } From a745d28b86683fd7354c89713b8a7f12173358e9 Mon Sep 17 00:00:00 2001 From: tejal29 Date: Thu, 28 May 2020 14:49:01 -0700 Subject: [PATCH 2/2] changed multiple times reported one --- pkg/skaffold/deploy/resource/deployment.go | 3 +- .../deploy/resource/deployment_test.go | 35 ++++++++++++------- pkg/skaffold/deploy/resource/status.go | 7 ++-- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/pkg/skaffold/deploy/resource/deployment.go b/pkg/skaffold/deploy/resource/deployment.go index 6829c7cd02a..9339348fb24 100644 --- a/pkg/skaffold/deploy/resource/deployment.go +++ b/pkg/skaffold/deploy/resource/deployment.go @@ -114,9 +114,10 @@ func (d *Deployment) IsStatusCheckComplete() bool { } func (d *Deployment) ReportSinceLastUpdated() string { - if !d.status.changed { + if d.status.reported && !d.status.changed { return "" } + d.status.reported = true return fmt.Sprintf("%s: %s", d, d.status) } diff --git a/pkg/skaffold/deploy/resource/deployment_test.go b/pkg/skaffold/deploy/resource/deployment_test.go index e675766a225..18c50998dec 100644 --- a/pkg/skaffold/deploy/resource/deployment_test.go +++ b/pkg/skaffold/deploy/resource/deployment_test.go @@ -213,29 +213,40 @@ func TestReportSinceLastUpdated(t *testing.T) { func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) { var tests = []struct { - description string - times int - expected string + description string + statuses []string + reportStatusSeq []bool + expected string }{ { - description: "report first time should return status", - times: 1, - expected: "test-ns:deployment/test: cannot pull image", + description: "report first time should return status", + statuses: []string{"cannot pull image"}, + reportStatusSeq: []bool{true}, + expected: "test-ns:deployment/test: cannot pull image", + }, + { + description: "report 2nd time should not return when same status", + statuses: []string{"cannot pull image", "cannot pull image"}, + reportStatusSeq: []bool{true, true}, + expected: "", }, { - description: "report 2nd time should not return", - times: 2, - expected: "", + description: "report called after multiple changes but last status was not changed.", + statuses: []string{"cannot pull image", "changed but not reported", "changed but not reported", "changed but not reported"}, + reportStatusSeq: []bool{true, false, false, true}, + expected: "test-ns:deployment/test: changed but not reported", }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { dep := NewDeployment("test", "test-ns", 1) var actual string - for i := 0; i < test.times; i++ { + for i, status := range test.statuses { // update to same status - dep.UpdateStatus("cannot pull image", nil) - actual = dep.ReportSinceLastUpdated() + dep.UpdateStatus(status, nil) + if test.reportStatusSeq[i] { + actual = dep.ReportSinceLastUpdated() + } } t.CheckDeepEqual(test.expected, actual) }) diff --git a/pkg/skaffold/deploy/resource/status.go b/pkg/skaffold/deploy/resource/status.go index 05e041ecb39..86a204d8b86 100644 --- a/pkg/skaffold/deploy/resource/status.go +++ b/pkg/skaffold/deploy/resource/status.go @@ -17,9 +17,10 @@ limitations under the License. package resource type Status struct { - err error - details string - changed bool + err error + details string + changed bool + reported bool } func (rs Status) Error() error {