-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
handle StatusCheck Events implementation logic #2929
Conversation
Codecov Report
|
@@ -33,6 +33,8 @@ const ( | |||
Complete = "Complete" | |||
Failed = "Failed" | |||
Info = "Information" | |||
Started = "Started" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use "In Progress" instead of "Started" (since In Progress implies that a process has Started) and "Complete" instead of "Succeeded"? That way we have consistency with other events. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I had it "completed" instead of "succeeded" befor to be consistent. But one of the comments from IDE team on the internal doc was to change it to "succeeded" so it goes well with "Failed"
We shd change "completed" to "succeeded" which I can do in another PR.
Regarding "started" to "in progress", there is a delay between when the status check startes and when we receive a first update. Hence I added Started for the first event and subsequent events are "in Progress" with a text update on how many resources are pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other alternative is to have a In progress with message "2/2 deployment are still pending" instead of strated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, could we open an issue to change "completed" to "succeeded"?
Personally I'm leaning towards having "In Progress" with the message you said above, just to match, but I don't feel super strongly about it.
Relates to #176
Description
User facing changes
n/a
Before
n/a
After
n/a
Next PRs.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
examples
dir, please copy them tointegration/examples
integration/examples
dir, should be tested in integration testReviewer Notes
Release Notes
Describe any user facing changes here so maintainer can include it in the release notes, or delete this block.