Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move field reported to changed #4222

Merged
merged 2 commits into from
Jun 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions pkg/skaffold/deploy/resource/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
tejal29 marked this conversation as resolved.
Show resolved Hide resolved
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this return appropriate here? You still need to check the isErrAndNotRetryable(err) to possibly set done?

Copy link
Contributor Author

@tejal29 tejal29 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm no. the status is already same as previous state. We don't need to perform any checks. The done status should have been set in the last iteration.

}
d.status = updated
d.status.changed = true
if strings.Contains(details, rollOutSuccess) || isErrAndNotRetryAble(err) {
d.done = true
}
}

Expand Down Expand Up @@ -111,7 +114,7 @@ func (d *Deployment) IsStatusCheckComplete() bool {
}

func (d *Deployment) ReportSinceLastUpdated() string {
if d.status.reported {
if d.status.reported && !d.status.changed {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right as .reported is only ever changed here and it's always set to true.

I think you just want to track the last-reported status and the current status, and compare them here.

Copy link
Contributor Author

@tejal29 tejal29 Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briandealwis This works correctly because, for every new status seen, we create a status with constructor newStatus.
The .reported set to false.
See deployment.go L57

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok 👍

return ""
}
d.status.reported = true
Expand Down
38 changes: 25 additions & 13 deletions pkg/skaffold/deploy/resource/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,35 +206,47 @@ 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)
})
}
}

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)
dep.UpdateStatus("cannot pull image", nil)
var actual string
for i := 0; i < test.times; i++ {
actual = dep.ReportSinceLastUpdated()
for i, status := range test.statuses {
// update to same status
dep.UpdateStatus(status, nil)
if test.reportStatusSeq[i] {
actual = dep.ReportSinceLastUpdated()
}
}
t.CheckDeepEqual(test.expected, actual)
})
Expand Down
2 changes: 2 additions & 0 deletions pkg/skaffold/deploy/resource/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package resource
type Status struct {
err error
details string
changed bool
reported bool
}

Expand Down Expand Up @@ -47,5 +48,6 @@ func newStatus(msg string, err error) Status {
return Status{
details: msg,
err: err,
changed: true,
}
}