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.

Sign up for GitHub

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

Jump to bottom

move field reported to changed #4222

Merged
merged 2 commits into from
Jun 5, 2020
Merged

move field reported to changed #4222

merged 2 commits into from
Jun 5, 2020

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented May 19, 2020 •
edited
Loading

Description
I am working on hooking up pod statuses into skaffold.
With pod statuses now coming in, we need a variable to track if a pod status which is part of deployment is changed to print out deployment and pod statuses together like this.

- deployment/leeroy-web is ready. [1/2 deployment(s) still pending]
 - deployment/leeroy-app: waiting for rollout to finish: 1 old replicas are pending termination...
    - pod/leeroy-app-85dfb77b44-md2dd: container leeroy-app is waiting to start: leeroy-app1 can't be pulled
 - deployment/leeroy-app: waiting for rollout to finish: 1 old replicas are pending termination...
    - pod/leeroy-app-85dfb77b44-wemde: container leeroy-app in error: Back-off pulling image "leeroy-app1"

Repurposing the previous reported to indicate if a deployment status was changed because a new pod came status was available or new pod came up.

@codecov
Copy link

codecov bot commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #4222 into master will decrease coverage by 0.40%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/resource/deployment.go 93.33% <100.00%> (+0.35%) ⬆️
pkg/skaffold/deploy/resource/status.go 100.00% <100.00%> (ø)
pkg/skaffold/runner/deploy.go 38.33% <0.00%> (-22.20%) ⬇️
pkg/skaffold/runner/intent.go 79.31% <0.00%> (-20.69%) ⬇️
pkg/skaffold/kubernetes/watcher.go 87.50% <0.00%> (-12.50%) ⬇️
...affold/kubernetes/portforward/kubectl_forwarder.go 60.97% <0.00%> (-7.32%) ⬇️
pkg/skaffold/server/server.go 53.65% <0.00%> (-4.92%) ⬇️
pkg/skaffold/runner/new.go 64.51% <0.00%> (-4.84%) ⬇️
pkg/skaffold/util/cmd.go 47.61% <0.00%> (-4.77%) ⬇️
...ffold/kubernetes/portforward/resource_forwarder.go 84.31% <0.00%> (-3.45%) ⬇️
... and 24 more

@tejal29 tejal29 mentioned this pull request May 19, 2020
Merged
7 tasks
@briandealwis briandealwis changed the title move filed reported to changed move field reported to changed May 20, 2020
@tejal29 tejal29 requested a review from tstromberg as a code owner May 28, 2020 21:49
@pull-request-size pull-request-size bot added size/M and removed size/S labels May 28, 2020
@tejal29 tejal29 force-pushed the changeLastReported branch from c1a42fd to a745d28 Compare May 28, 2020 21:51
briandealwis
briandealwis reviewed May 29, 2020
View reviewed changes
pkg/skaffold/deploy/resource/deployment.go
@@ -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 •
edited
Loading

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 👍

pkg/skaffold/deploy/resource/deployment.go
}
if d.status.Equal(updated) {
d.status.changed = false
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 •
edited
Loading

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.

briandealwis
briandealwis approved these changes Jun 5, 2020
View reviewed changes
pkg/skaffold/deploy/resource/deployment.go
@@ -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.

Ah ok 👍

@tejal29 tejal29 merged commit 08b086c into GoogleContainerTools:master Jun 5, 2020
@fmotrifork fmotrifork mentioned this pull request Jun 12, 2020
Merged
@tejal29 tejal29 deleted the changeLastReported branch April 15, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briandealwis briandealwis briandealwis approved these changes

@balopat balopat Awaiting requested review from balopat

@dgageot dgageot Awaiting requested review from dgageot

@nkubala nkubala Awaiting requested review from nkubala

@tstromberg tstromberg Awaiting requested review from tstromberg

Assignees
No one assigned
Labels
cla: yes size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tejal29 @briandealwis @googlebot