Merge pull request #2865 from tejal29/add_reported_since

Update a deployment health check status only its changed.

pkg/skaffold/deploy/resource/deployment.go

@@ -50,7 +50,18 @@ func (d *Deployment) Status() Status {
 }
 
 func (d *Deployment) UpdateStatus(details string, err error) {
-	d.status = newStatus(details, err)
+	updated := newStatus(details, err)
+	if !d.status.Equal(updated) {
+		d.status = updated
+	}
+}
+
+func (d *Deployment) ReportSinceLastUpdated() string {
+	if d.status.reported {
+		return ""
+	}
+	d.status.reported = true
+	return fmt.Sprintf("%s %s", d, d.status)
 }
 
 func NewDeployment(name string, ns string, deadline time.Duration) *Deployment {

pkg/skaffold/deploy/resource/deployment_test.go

@@ -0,0 +1,84 @@
+/*
+Copyright 2019 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package resource
+
+import (
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/testutil"
+	"github.com/pkg/errors"
+)
+
+func TestReportSinceLastUpdated(t *testing.T) {
+	var tests = []struct {
+		description string
+		message     string
+		err         error
+		expected    string
+	}{
+		{
+			description: "updating an error status",
+			message:     "cannot pull image",
+			err:         errors.New("cannot pull image"),
+			expected:    "test-ns:deployment/test cannot pull image",
+		},
+		{
+			description: "updating a non error status",
+			message:     "is waiting for container",
+			expected:    "test-ns:deployment/test is waiting for container",
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			dep := NewDeployment("test", "test-ns", 1)
+			dep.UpdateStatus(test.message, test.err)
+			t.CheckDeepEqual(test.expected, dep.ReportSinceLastUpdated())
+			// Check reported is set to true.
+			t.CheckDeepEqual(true, dep.status.reported)
+		})
+	}
+}
+
+func TestReportSinceLastUpdatedMultipleTimes(t *testing.T) {
+	var tests = []struct {
+		description string
+		times       int
+		expected    string
+	}{
+		{
+			description: "report first time should return status",
+			times:       1,
+			expected:    "test-ns:deployment/test cannot pull image",
+		},
+		{
+			description: "report 2nd time should not return",
+			times:       2,
+			expected:    "",
+		},
+	}
+	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++ {
+				actual = dep.ReportSinceLastUpdated()
+			}
+			t.CheckDeepEqual(test.expected, actual)
+		})
+	}
+}

pkg/skaffold/deploy/resource/status.go

@@ -17,8 +17,9 @@ limitations under the License.
 package resource
 
 type Status struct {
-	err     error
-	details string
+	err      error
+	details  string
+	reported bool
 }
 
 func (rs Status) Error() error {
@@ -32,6 +33,16 @@ func (rs Status) String() string {
 	return rs.details
 }
 
+func (rs Status) Equal(other Status) bool {
+	if rs.details != other.details {
+		return false
+	}
+	if rs.err != nil && other.err != nil {
+		return rs.err.Error() == other.err.Error()
+	}
+	return rs.err == other.err
+}
+
 func newStatus(msg string, err error) Status {
 	return Status{
 		details: msg,

pkg/skaffold/deploy/resource/status_test.go

@@ -18,6 +18,7 @@ package resource
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
 	"github.com/GoogleContainerTools/skaffold/testutil"
@@ -57,3 +58,45 @@ func TestString(t *testing.T) {
 		})
 	}
 }
+
+func TestEqual(t *testing.T) {
+	var tests = []struct {
+		description string
+		old         Status
+		new         Status
+		expected    bool
+	}{
+		{
+			description: "status should be same for same details and error",
+			old:         Status{details: "Waiting for 0/1 replicas to be available...", err: nil},
+			new:         Status{details: "Waiting for 0/1 replicas to be available...", err: nil},
+			expected:    true,
+		},
+		{
+			description: "status should be new if error messages are same",
+			old:         Status{details: "same", err: errors.New("same error")},
+			new:         Status{details: "same", err: errors.New("same error")},
+			expected:    true,
+		},
+		{
+			description: "status should be new if error is different",
+			old:         Status{details: "same", err: nil},
+			new:         Status{details: "same", err: fmt.Errorf("see this error")},
+		},
+		{
+			description: "status should be new if details and err are different",
+			old:         Status{details: "same", err: nil},
+			new:         Status{details: "same", err: fmt.Errorf("see this error")},
+		},
+		{
+			description: "status should be new if details change",
+			old:         Status{details: "same", err: nil},
+			new:         Status{details: "error", err: nil},
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.CheckDeepEqual(test.expected, test.old.Equal(test.new))
+		})
+	}
+}