Skip to content

fix status check event error reporting #4101

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

Merged

Conversation

tejal29
Copy link
Contributor

@tejal29 tejal29 commented May 4, 2020

Fixes: #4100

Description

Add a defer func to always add a event, failed or successful for StatusCheck

User facing changes (remove if N/A)
no. Event API Changes.

  1. In case an intermediate error happened, ( in this case force via code change) you see a corresponding error event.
tejaldesai@tejaldesai-macbookpro2 microservices (pod_hookup)curl localhost:50052/v1/events
{"result":{"timestamp":"2020-05-04T23:24:08.171420Z","event":{"metaEvent":{"entry":"Starting Skaffold: \u0026{Version:v1.8.0-127-g7b7380859-dirty ConfigVersion:skaffold/v2beta3 GitVersion: GitCommit:7b73808593e6c262cfabfc24d279d425682cf920 GitTreeState:dirty BuildDate:2020-05-04T16:23:51Z GoVersion:go1.14.1 Compiler:gc Platform:darwin/amd64}","metadata":{"build":{"numberOfArtifacts":2,"builders":[{"type":"DOCKER","count":2}],"type":"LOCAL"},"deploy":{"deployers":[{"type":"KUBECTL","count":1}],"cluster":"GKE"}}}}}}
{"result":{"timestamp":"2020-05-04T23:24:09.536583Z","event":{"deployEvent":{"status":"In Progress"}},"entry":"Deploy started"}}
{"result":{"timestamp":"2020-05-04T23:24:10.518799Z","event":{"deployEvent":{"status":"Complete"}},"entry":"Deploy complete"}}
...

{"result":{"timestamp":"2020-05-04T23:31:02.383694Z","event":{"statusCheckEvent":{"status":"Started"}},"entry":"Status check started"}}
{"result":{"timestamp":"2020-05-04T23:31:02.384302Z","event":{"statusCheckEvent":{"status":"Failed","err":"force"}},"entry":"Status check failed"}}
curl: (18) transfer closed with outstanding read data remaining

tejaldesai@tejaldesai-macbookpro2 microservices (pod_hookup)
  1. For success scenarios, the behavior is same as before
.....
{"result":{"timestamp":"2020-05-04T23:32:32.522157Z","event":{"statusCheckEvent":{"status":"Started"}},"entry":"Status check started"}}
{"result":{"timestamp":"2020-05-04T23:32:33.139690Z","event":{"resourceStatusCheckEvent":{"resource":"deployment/leeroy-app","status":"In Progress","message":"deployment/leeroy-app: "}},"entry":"Resource deployment/leeroy-app status updated to In Progress"}}
...

{"result":{"timestamp":"2020-05-04T23:32:34.550763Z","event":{"statusCheckEvent":
{"status":"Succeeded"}},"entry":"Status check succeeded"}}

Follow-up Work (remove if N/A)

@tejal29 tejal29 force-pushed the fix_status_check_err_reporting branch 2 times, most recently from d97d90c to 175e107 Compare May 4, 2020 23:38
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

I think this change would be cleaner if you'd used a delegate that wraps the body of the StatusCheck result, sends the event and then returns the result. Using defer for that seems a bit fragile - for example the return error in L77 is already different from the err that we are sending in the event.

@balopat
Copy link
Contributor

balopat commented May 4, 2020

plus, it would be great to have tests for our events...I'll open a bug for that

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #4101 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted Files Coverage Δ
pkg/skaffold/deploy/status_check.go 65.06% <0.00%> (-3.25%) ⬇️

@tejal29 tejal29 force-pushed the fix_status_check_err_reporting branch from 175e107 to 4ddb955 Compare May 5, 2020 01:05
@tejal29 tejal29 merged commit 0ad8d0d into GoogleContainerTools:master May 5, 2020
@tejal29 tejal29 deleted the fix_status_check_err_reporting branch April 15, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status check fails to report failed event for intermediate errors.
3 participants