feat: Add workspaceId, mode, name to webhook #9820
Conversation
✅ Deploy Preview for determined-ui canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9820 +/- ##
==========================================
- Coverage 54.71% 54.64% -0.08%
==========================================
Files 1261 1261
Lines 155969 156179 +210
Branches 3590 3589 -1
==========================================
- Hits 85345 85344 -1
- Misses 70492 70703 +211
Partials 132 132
Flags with carried forward coverage won't be shown. Click here to find out more.
|
webui/react/src/hooks/useFeature.ts
Outdated
| description: 'Webhook improvement', | ||
| friendlyName: 'Webhook improvement', |
There was a problem hiding this comment.
nit: friendlyName should be Title Case, description should include at least vaguely what improvements are included
| const workspaces = Loadable.match(useObservable(workspaceStore.workspaces), { | ||
| _: () => [], | ||
| Loaded: (ws) => ws, | ||
| }); |
There was a problem hiding this comment.
sugg: const workspaces = useObservable(workspaceStore.workspaces).getOrElse([])
| }; | ||
|
|
||
| export interface Settings extends InteractiveTableSettings { | ||
| columns: WebhookColumnName[]; | ||
| workspace?: number[]; |
There was a problem hiding this comment.
nit: workspace -> workspaceIds
| @@ -83,6 +103,14 @@ const WebhooksView: React.FC = () => { | |||
| fetchWebhooks(); | |||
| }, [fetchWebhooks]); | |||
|
|
|||
| useEffect(() => { | |||
| if (settings.workspace?.length) { | |||
There was a problem hiding this comment.
super nit: if (Array.isArray(settings.workspace)) {
There was a problem hiding this comment.
Sure, but this feels a bit unsafe for me, in case the settings.workspace is set to 0 accidentally..
| @@ -83,6 +103,14 @@ const WebhooksView: React.FC = () => { | |||
| fetchWebhooks(); | |||
| }, [fetchWebhooks]); | |||
|
|
|||
| useEffect(() => { | |||
| if (settings.workspace?.length) { | |||
| setFilteredWebhooks(webhooks.filter((w) => settings.workspace?.includes(w.workspaceId))); | |||
There was a problem hiding this comment.
sugg: as written this line has a nested loop. a possibly more performant alternative would be:
const idSet = new Set(settings.workspace);
setFilteredWebhooks(webhooks.filter((w) => idSet.has(w.workspaceId)));
| initialValue={V1WebhookMode.WORKSPACE} | ||
| label="Trigger by" | ||
| name="mode" | ||
| rules={[{ message: 'Webhook mode is required ', required: true }]}> |
There was a problem hiding this comment.
super nit: remove trailing space
| label="Trigger by" | ||
| name="mode" | ||
| rules={[{ message: 'Webhook mode is required ', required: true }]}> | ||
| <Select options={modeOptions} placeholder="Select mode of Webhook" /> |
There was a problem hiding this comment.
super nit: maybe just Select webhook mode?
| @@ -132,6 +157,25 @@ const WebhookCreateModalComponent: React.FC<Props> = ({ onSuccess }: Props) => { | |||
| id={idPrefix + FORM_ID} | |||
| layout="vertical" | |||
| onFieldsChange={onChange}> | |||
| {f_webhook && ( | |||
| <> | |||
| <Form.Item label="Workspace" name="workspaceId"> | |||
There was a problem hiding this comment.
nit: this says it's required but doesn't seem to be in code. i see that there's a default value but you also allow the select to be cleared
| const workspaces = Loadable.match(useObservable(workspaceStore.workspaces), { | ||
| _: () => [], | ||
| Loaded: (ws) => ws, | ||
| }); |
There was a problem hiding this comment.
nit: const workspaces = useObservable(workspaceStore.workspaces).getOrElse([])
| value: V1WebhookMode.WORKSPACE, | ||
| }, | ||
| { | ||
| label: 'Specific experiment with matching configuration', |
There was a problem hiding this comment.
q: can this mode ever match multiple experiments? if so maybe change copy to Specific experiment(s) with...
There was a problem hiding this comment.
@EmilyBonar Thanks for reviewing!
Feel free to try out the feature, since the only changes here are related to creating webhooks, using webhooks should work as expected.
Ticket
MD-477
Description
Part of workload alerting project.
ERD link: https://hpe-aiatscale.atlassian.net/wiki/spaces/ENGINEERIN/pages/1666809868/Workload+Alerting+ERD
Add workspaceId, name, mode to webhook
Test Plan
Login as admin, with "Webhook improvement" flag enabled, navigate to webhook page.
The webhook creation modal should have new fields, and the table should display new fields.
With feature flag off, the webhook should still work as expected.
Checklist
docs/release-notes/See Release Note for details.