Fixes deletion of recurring runs #1185
Conversation
…e from pipeline details page
frontend/src/lib/Buttons.ts
Outdated
| @@ -80,11 +80,12 @@ export default class Buttons { | |||
| } | |||
|
|
|||
| public delete(getSelectedIds: () => string[], resourceName: 'pipeline' | 'recurring run config', | |||
| callback: (selectedIds: string[], success: boolean) => void, useCurrentResource: boolean): ToolbarActionConfig { | |||
| callback: (selectedIds: string[], success: boolean) => void, useCurrentResource: boolean, | |||
| refreshOnComplete: boolean): ToolbarActionConfig { | |||
There was a problem hiding this comment.
Isn't this always desired? If it is, we can just always call it here.
If it isn't, why can't refresh be optionally called by the callsite as part of the callback?
There was a problem hiding this comment.
It isn't desirable when you're on a specific resource's page (e.g. Run Details), and you delete that resource. In that case, a request is sent to the server to reload the page for a resource that no longer exists before redirecting to whatever page you actually want to be on.
The second part/suggestion would work. My thinking was just that this would prevent duplicate code in the callbacks
There was a problem hiding this comment.
Why not use the useCurrentResource flag then? Seems like it has the same information.
There was a problem hiding this comment.
No strong opinion here. I thought maybe it would be useful to have an explicit flag so that the implications are clear, but there's no reason AFAICT that we couldn't use the useCurrentResource (or situation where that wouldn't make sense at the moment).
I can change it if you think it's better to not add the extra flag
There was a problem hiding this comment.
I think it's a generally good idea to simplify the interface provided the caller doesn't need to know this flag is used for two different things. It seems internal to the method's implementation.
There was a problem hiding this comment.
Sounds good. It's been removed
| 'pipeline', | ||
| this._deleteCallback.bind(this), | ||
| true, /* useCurrentResource */ | ||
| false, /* refreshOnComplete */ |
There was a problem hiding this comment.
If there's a good reason why we don't refresh after delete, it's worth calling it out in a comment here.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Feature: Add torchserve custom server bert sample * Fix: updated Readme - added deployment yaml example * Fix: Update service host for inference request * Fix: Remove port from deployment yaml - Fix spelling in readme doc - Fix space in readme doc * Fix: move bert example to a sub folder * Fix: readme doc
Also redirects after deleting pipeline from pipeline details page.
Fixes #1181
This change is