Allow Start Last Run in the Overview to work again - #3227
Conversation
openshift-ci-robot
added
the
size/M
There was a problem hiding this comment.
Rare instances it wouldn't start immediately and showed - which I felt could be better shown as Pending.
There was a problem hiding this comment.
This probably shouldn't be a hook. accessReview is optional (from rerunPipeline) but it must call this function every time due to the way hooks work.
|
/assign @sahil143 This is fixing of your feature work during the merge conflicts that make this call no longer work. Please review and let me know if you think the heart of what you had is still here. |
4541c76 to
185e8a0
Compare
There was a problem hiding this comment.
Why are you checking for metadata.name and namespace here? If pipelineRun is there then metadata should be there.
There was a problem hiding this comment.
Not according to the type. PipelineRun doesn't define it, but extends K8sResourceKind and it specifies it as optional. Of which inside metadata, name and namespace are also optional.
There was a problem hiding this comment.
AFAIK if pipelineRun exists then it will have metadata for sure. Check out the K8sResourceCommon type (metadata is not optional) we are migrating to it. Now, it's only used for DeploymentKind but it will be used for other types as well.
I don't think a Resource would exist without it's name.
There was a problem hiding this comment.
@sahil143 unless the type we are currently using says this, I won't write code that "assumes" optional params are there. Types should not be just ignored when it's inconvenient. This code is currently only executed on click so it's not a render-cycle slowdown.
There was a problem hiding this comment.
resources is not being used in the method
There was a problem hiding this comment.
Nope, but it's the signature of the KebabAction, and I need the customData for my callbacks:
export type KebabAction = (
kind: K8sKind,
obj: K8sResourceKind,
resources?: any,
customData?: any,
) => KebabOption;
There was a problem hiding this comment.
We can define {pipelineRunRedirect: true} here and make it optional.
There was a problem hiding this comment.
Not entirely true. customData not being defined would get the default value. But if I provided null or a different set of customData I'd lose the ability to predefine my value.
This is a thought exercise of course, since I did change the logic up a bit. But for sake of argument, you should deal with this in two spots, because = ... only works on params that are explicitly defined as undefined or just not provided.
| type RerunPipelineData = { | ||
| onComplete?: (pipelineRun: PipelineRun) => void; | ||
| }; | ||
| const rerunPipeline: KebabAction = ( |
There was a problem hiding this comment.
I logged https://jira.coreos.com/browse/ODC-2184 to address the fact that this function was converted to being a rereun pipeline runs.
| export const rerunPipelineAndStay: KebabAction = (kind: K8sKind, pipelineRun: PipelineRun) => { | ||
| return rerunPipeline(kind, pipelineRun); | ||
| }; | ||
|
|
||
| export const rerunPipelineAndRedirect: KebabAction = (kind: K8sKind, pipelineRun: PipelineRun) => { | ||
| return rerunPipeline(kind, pipelineRun, null, { onComplete: handlePipelineRunSubmit }); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why this change? I think flag thing (pipelineRunRedirect) worked fine.
You are still adding a check
typeof onComplete === 'function' ? onComplete : () => {}
There was a problem hiding this comment.
By default we can keep the flag true or false depending on majority usage of flag and make customData optional. WDYT?
There was a problem hiding this comment.
I want to make the API of using this not complex. The KebabActions are of a particular type, the caller shouldn't need to jump through hoops. I feel as an outsider calling to this utility of functions, I shouldn't care about how we set it up. rerunPipelineAndRedirect or rerunPipelineAndStay are very straight forward and easy to understand when you see the method call and easy to call.
|
/lgtm verified changes locally. It works!! |
Yup :) I posted that in my description of the PR. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, rohitkrai03, sahil143 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
6 participants
https://jira.coreos.com/browse/ODC-2177
Fixes the Start Last Run button on the Overview pane in Topology. It ran into a conflict with another work that reworked the API.
Issue:
Fixed:
The shaking appears related to https://jira.coreos.com/browse/ODC-2122 if you look at the dev tools you can see the vibration happens at the time it remounts.