-
Notifications
You must be signed in to change notification settings - Fork 26
SRVKP-8998: fix Pipeline Builder UI shows stale parameter data for tasks with same name across namespaces #678
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
SRVKP-8998: fix Pipeline Builder UI shows stale parameter data for tasks with same name across namespaces #678
Conversation
47acb2a to
35a8268
Compare
|
@anwesha-palit-redhat: This pull request references SRVKP-8998 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| export const fetchArtifactHubTasks = async ( | ||
| query: string, | ||
| limit: number = 20, | ||
| limit = 20, |
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.
lets keep the type
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 was a lint error, due to auto inference, so will switch it back
| taskExists = true; | ||
| } catch (error) { | ||
| console.warn( | ||
| `Error fetching Task as task does not exist ${taskName}, so safely deleting from PipelineBuilder`, |
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.
lets the the RC if its really 404
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.
I think the whole onRemove() should be a callback... if anything then for a better readability
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, thanks
| ); | ||
| } | ||
| if (taskExists) { | ||
| await k8sDelete({ |
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 risks deleting a user-managed Task that just happens to share the name. Shouldnt we delete Tasks that the builder installed in this flow, checking TektonTaskAnnotation.installedFrom ?
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.
That’s a good point — Using installedFrom limits cleanup to builder-installed tasks, but users may also install or update tasks via CLI / YAML. In this case, they were installing tasks using CLI and then adding them via the builder.
Therefore, I implemented this as I felt a more general approach might be safer long term — one that ensures consistent cleanup while still protecting user-managed resources.
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.
Updated it according to what is suggested
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.
Updated according to what @vikram-raj suggested, i.e, revert to the original version and not to delete the tasks, only update the UI builder
| useCleanupOnFailure(failedTasks, onUpdateTasks, taskGroup); | ||
|
|
||
| // Get all existing task names from taskGroup and installed tasks | ||
| const getExistingTaskNames = (): string[] => { |
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.
the current implementation does not count with duplicated names. I would suggest something like:
const taskNames = new Set<string>();
// Add all tasks currently in the builder
[
...taskGroup.tasks,
...taskGroup.finallyTasks,
...taskGroup.listTasks,
...taskGroup.loadingTasks,
...taskGroup.finallyListTasks,
].forEach((t) => {
if (t?.name) taskNames.add(t.name);
});
// Add installed catalog items (avoid duplicates)
catalogService.items.forEach((catalogItem) => {
const name = catalogItem.data?.metadata?.name;
if (name) taskNames.add(name);
});
return Array.from(taskNames);
};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, thanks!
| ).catch(() => | ||
| setFailedTasks([...failedTasks, item.data.task.name]), | ||
| ); | ||
| // Checking if task with same name already exists, if yes then create with a different name to avoid conflict |
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: These two blocks are pretty much the same, just using a different model. Lets rather create a callback which will take the model and its parameters
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, thanks
|
@jhadvig: changing LGTM is restricted to collaborators In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
35a8268 to
e00cb08
Compare
…llation of tasks with name name
e00cb08 to
261eac7
Compare
vikram-raj
left a comment
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: anwesha-palit-redhat, vikram-raj 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 |
|
/cherry-pick release-v1.20.x |
|
/cherry-pick release-v1.19.x |
|
@anwesha-palit-redhat: new pull request created: #805 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@anwesha-palit-redhat: new pull request created: #806 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick release-v1.18.x |
|
/cherry-pick release-v1.17.x |
|
@anwesha-palit-redhat: #678 failed to apply on top of branch "release-v1.18.x": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@anwesha-palit-redhat: #678 failed to apply on top of branch "release-v1.17.x": In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Description of the Problem
The customer experiences confusion and incorrect task configuration when editing pipelines through the Pipeline Builder UI.
Because the side panel displays stale or incorrect parameter data for tasks that share the same name across namespaces, users may unknowingly save or deploy pipelines with invalid configurations.
This leads to pipeline failures and loss of confidence in the UI’s reliability.
Workaround
A partial workaround exists:
Users can manually verify and edit the Pipeline YAML to ensure it references the correct task and parameters from the intended namespace.
However, this requires advanced Tekton knowledge and defeats the purpose of the visual builder — making it unsuitable for most users.
Versions
Steps to Reproduce
openshift-pipelinesnamespace.openshift-pipelinesnamespace.Reproducibility
Customer Impact
Root Cause
In the previous implementation, installation of Artifact Hub tasks failed because tasks with the same names already existed in the cluster resolver or namespace.
As a result:
Fix Implemented
Added a check for existing tasks with the same name before installation.
If a conflict is found, a safe name is generated and used for creating a new task.
During task removal, the system:
This ensures consistent task data and prevents stale or incorrect parameter references.
Screen Recordings
(Since GitHub upload size limits apply, videos are uploaded to Google Drive)