-
Notifications
You must be signed in to change notification settings - Fork 605
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
check for taskRef kind in the TR spec #7005
Conversation
/kind bug |
/cc @jerolimov |
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.
Tested it locally, works fine. But I have one small change request.
obj.spec.taskRef?.kind === ClusterTaskModel.kind | ||
? referenceForModel(ClusterTaskModel) | ||
: referenceForModel(TaskModel); |
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.
Can we keep the result of referenceForModel outside of this Row function? So that it's not required to recalculate the kind for each row (also when its just a few string operations). More important for would be here, that we then keep the const together at one place. Wdyt?
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.
done. PTAL
0dbd4f0
to
9b1c5e6
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.
Tested this locally, works fine 👍
/lgtm
/assign @andrewballantyne |
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.
This looks like it will work fine, but I think we can go one step further and place a new utility with the others like it.
@@ -42,7 +43,9 @@ const TaskRunsRow: RowFunction<TaskRunKind> = ({ obj, index, key, style, ...prop | |||
<TableData className={tableColumnClasses[3]}> | |||
{obj.spec.taskRef?.name ? ( | |||
<ResourceLink | |||
kind={taskReference} | |||
kind={ | |||
obj.spec.taskRef?.kind === ClusterTaskModel.kind ? clusterTaskReference : taskReference |
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.
So we already have this kinda check ... but not quite this exact check in the pipeline-augment.ts file. Perhaps we just expand this with another wrapper function:
Ie, add this:
const getModelReferenceFromTaskKind = (kind: string): GroupVersionKind => {
const model = getResourceModelFromTaskKind(kind);
return referenceForModel(model);
}
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.
So sorry for the confusion... please place this utility with the others in the pipeline-augment.ts
file noted above.
9b1c5e6
to
23fbdf4
Compare
23fbdf4
to
6cfa1a2
Compare
Can we co-locate the utility? Sorry for the confusion. |
6cfa1a2
to
240d097
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, debsmita1, jerolimov 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 |
Fixes:
https://issues.redhat.com/browse/ODC-5031
Root Analysis:
The Task col in the TaskRun list is always considering the taskRef to be of kind
Task
. So, in a scenario where TaskRun references ClusterTask, the linked task name gives a 404 errorSolution Description:
added a check for the taskRef kind
GIF: