-
Notifications
You must be signed in to change notification settings - Fork 40
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
Feature/delivery alerts #768
Conversation
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Codecov Report
@@ Coverage Diff @@
## deploy/hammer #768 +/- ##
=================================================
- Coverage 53.71% 51.99% -1.72%
=================================================
Files 263 270 +7
Lines 6695 7018 +323
Branches 902 958 +56
=================================================
+ Hits 3596 3649 +53
- Misses 2958 3230 +272
+ Partials 141 139 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…ays shown Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…s to display Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
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.
Thanks for this effort, Aaron!
This was really fast!
I left you minimal suggestions that you can consider.
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Thanks for the review @Angatupyry! I've further simplified the implementation, b082bdf, as we are generating delivery alert IDs using timestamps, we can find out which ones are the newest. This resolves a bug that I found, where newer warnings don't overwrite older warnings if they are not resolved. |
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.
LGTM!
…sing carts Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Cancel is only available if the The current implementation dispatches the cancellation, we can roll with this for now. If it ever makes things easier on your end, I can remove that, and let you perform the cancellation.
Good catch, fixed in 003a31b |
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@xiyuoh per DM, since updating action and cancelling task is in the same try except scope, the action is not updated if the task cancellation fails, while the button still becomes disabled. Disabling the button should come from feedback on whether the action has been updated instead. |
…en properly cancelled Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…was clicked Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Merging this to keep the diff small, will apply fixes if needed afterwards. |
What's new
delivery_alerts
backendAssumptions
error
, there are no recover modes for that task, and this feature assumes it has been cancelled before emitting this alert.How it looks like
Pardon the long video, here it displays triggering the delivery alerts using the API server,
action
field, before the alert is allowed to be closedsimplescreenrecorder-2023-09-20_00.42.09.mp4
Self-checks