-
Notifications
You must be signed in to change notification settings - Fork 356
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
feat: support updating web hook url [MD-482] #9890
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9890 +/- ##
==========================================
+ Coverage 54.57% 59.18% +4.61%
==========================================
Files 1250 751 -499
Lines 156266 104452 -51814
Branches 3598 3597 -1
==========================================
- Hits 85283 61824 -23459
+ Misses 70852 42496 -28356
- Partials 131 132 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Looks good! Just some minor comments.
}; | ||
|
||
// The updated webhook. | ||
determined.webhook.v1.PatchWebhook webhook = 1; |
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.
nit: I'd prefer reusing determined.webhook.v1.Webhook
c36212e
to
e958c3c
Compare
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
|
||
for _, t := range ts { | ||
if t.TriggerType != TriggerTypeTaskLog { | ||
return nil |
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.
is it expected that you receive requests to edit non-TaskLog triggers? Also do we want to exit the function as soon as we encounter the first non-Task log type trigger?
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.
is it expected that you receive requests to edit non-TaskLog triggers?
Yes. I'm skipping it because only TaskLog triggers are stored in cache. https://github.com/determined-ai/determined/pull/9890/files/c08e8b902b779999dee41e3d7b287f67453d8750#diff-3ea86afd178c30910e3fdbc3f40b950a61d566b1e70bc4d932bbba3e693c2802R71-R78
Also do we want to exit the function as soon as we encounter the first non-Task log type trigger?
Good catch! I shouldn't exit the function.
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!
c1339b4
to
c8fc477
Compare
The failures in CI |
Ticket
MD-482
Description
Users can edit the url of a webhook.
Test Plan
CI passes.
Checklist
docs/release-notes/
See Release Note for details.