Bug 1869600: add resource icon to the items in the `select task` dropdown in the pipeline builder page #6345

debsmita1 · 2020-08-17T17:20:55Z

Fixes:

Root Analysis:
Tasks and ClusterTasks are not visually distinguishable in the Select Task dropdown list in the pipeline builder page.

Solution Description:

added resource badge to the items in the Select Task dropdown
fixed the issue with Task & ClusterTask having the same name
fixed the issue with Tasks not showing up again in the dropdown if it is already selected

GIF:

debsmita1 · 2020-08-17T17:22:05Z

/kind bug

openshift-ci-robot · 2020-08-18T10:32:17Z

@debsmita1: This pull request references Bugzilla bug 1869600, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1869600: add resource icon to the items in the select task dropdown in the pipeline builder page

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/test-infra repository.

divyanshiGupta · 2020-08-18T17:05:12Z

@debsmita1 I think we should also add the resource badge to the builder nodes as without the badge the user wont be able to distinguish between the resources with same name just by looking at the pipeline. For example in the attached image one resource is a Task and the other is ClusterTask

cc: @beaumorley @bgliwa01

debsmita1 · 2020-08-19T06:52:01Z

@debsmita1 I think we should also add the resource badge to the builder nodes as without the badge the user wont be able to distinguish between the resources with same name just by looking at the pipeline. For example in the attached image one resource is a Task and the other is ClusterTask

cc: @beaumorley @bgliwa01

@andrewballantyne ^

andrewballantyne · 2020-08-19T13:14:24Z

@debsmita1 I think we should also add the resource badge to the builder nodes as without the badge the user wont be able to distinguish between the resources with same name just by looking at the pipeline. For example in the attached image one resource is a Task and the other is ClusterTask

cc: @beaumorley @bgliwa01

@andrewballantyne ^

No, I think at this point it does not matter. Selecting in a mixed list is a problem because you don't know which is which... but the name in the bubble is a custom name (it just pre-selects as the name of task you took). If you click on the bubble the badge is at the top of the sidebar and the "name" field is a custom name you can use for this task irrespective of what it was originally named.

So I think we are good with just the badge in the dropdown list. @bgliwa01 Please let me know your thoughts.

bgliwa01 · 2020-08-19T14:06:59Z

Agreed @andrewballantyne

andrewballantyne

I'm going to test around a bit and see how this idea goes... but I think this solves the layout issue if we always generate a new name (with a hash if conflicted).

andrewballantyne · 2020-08-20T20:19:57Z

frontend/packages/dev-console/src/components/pipelines/pipeline-topology/TaskListNode.tsx

@@ -82,7 +84,7 @@ const TaskListNode: React.FC<TaskListNodeProps> = ({ element, unselectedText })
          <div className="pf-c-dropdown pf-m-expanded">
            <ul className="pf-c-dropdown__menu pf-m-align-right oc-kebab__popper-items odc-task-list-node__list-items">
              {options.map((option) => (
-                <li key={option.label}>
+                <li key={`${option.label}-${option.icon?.['props']?.kind}`}>


Not a fan of digging into props unless you absolutely have to... I think since we have a little sandbox of code here, we could change things up a bit with the help of TypeScript.

type KeyedKebabOption = KebabOption & { key: string }; const taskToOption = (task: PipelineResourceTask, callback: NewTaskNodeCallback): KeyedKebabOption => {

That way we return a option.key value that we can use. You can set key simply to ${kind}-${name} with the data.

andrewballantyne · 2020-08-20T20:24:10Z

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts

  return {
-    name: resource.metadata.name,
+    name: `${resource.metadata.name}-${kind.toLowerCase()}`,


If we do this, we are not really avoiding the problem entirely. We are likely to be okay, but let's avoid anything that will lead to potentially another bug (or if QE is on game, a failure of the QE Review).

Recommend a generate name function (maybe we can put this in shared, or here -- wherever for now):

export const safeName = (reservedNames: string[], desiredName: string): string => { if (reservedNames.includes(desiredName)) { const newName = `${desiredName}-${getRandomChars()}`; // maybe `getRandomChars(3)`? if (reservedNames.includes(newName)) { return safeName(reservedNames, desiredName); } return newName; } return desiredName; }; // use like: name: safeName(reservedNames, resource.metadata.name),

Then you update the two uses of convertResourceToTask to include a 3rd param (I'd make it the first param personally) of a string[].

Sample usage:

const usedNames = tasks.map((t) => t.name); const newPipelineTask: PipelineTask = convertResourceToTask(usedNames, resource, runAfter);

andrewballantyne · 2020-08-20T20:26:25Z

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/hooks.ts

@@ -135,7 +135,10 @@ export const useNodes = (

  // TODO: Fix id collisions then remove this utility; we shouldn't need to trim the tasks
  const noDuplicates = (resource: PipelineResourceTask) =>
-    !taskGroupRef.current.tasks.find((pt) => pt.name === resource.metadata.name);
+    !taskGroupRef.current.tasks.find(
+      (pt) => pt.taskRef.name === resource.metadata.name && pt.taskRef.kind === resource.kind,


We can't do resource.metadata.name against pt.taskRef.name because this will prevent any double uses of a given task. This isn't the end of the world, but likely will be a problem for some use-cases.

However! With my recent changes suggested, I think we can actually get rid of this function since we'll always will generate a new name.

removing this piece of code should fix this bug https://issues.redhat.com/browse/ODC-3182, isn't it ?

That is correct @debsmita1 good find of that ticket. That should indeed resolve that issue.

debsmita1 · 2020-08-21T17:42:54Z

/retest

andrewballantyne

Couple tiny feedback points.

andrewballantyne · 2020-08-21T17:55:43Z

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/update-utils.ts

@@ -201,14 +201,16 @@ const addListNode: UpdateOperationAction<UpdateOperationAddData> = (tasks, listT
  }
 };

+const getTaskNames = (tasks: PipelineTask[]) => tasks.map((t) => t.name);


Not sure this was needed as it's a single operation, but technically it is DRY 😄 So nicely done 👍

andrewballantyne · 2020-08-21T17:56:56Z

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts

@@ -10,6 +10,7 @@ import {
 import { getTaskParameters } from '../resource-utils';
 import { TASK_ERROR_STRINGS, TaskErrorType } from './const';
 import { PipelineBuilderFormikValues, PipelineBuilderFormValues, TaskErrorMap } from './types';
+import { getRandomChars } from '@console/shared';


andrewballantyne · 2020-08-21T17:59:46Z

frontend/public/components/utils/kebab.tsx

@@ -528,6 +528,8 @@ export type KebabOption = {
  icon?: React.ReactNode;
 };

+export type KeyedKebabOption = KebabOption & { key: string };


Not sure we needed to add this to the common code. This was just for our use-case. I think it's probably best to move this back to TaskListNode.tsx. The current KebabOption uses the label as the key... so this feels a little weird in the common code.

andrewballantyne · 2020-08-21T18:00:30Z

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/utils.ts

@@ -29,15 +30,28 @@ export const taskParamIsRequired = (param: PipelineResourceTaskParam): boolean =
  return !('default' in param);
 };

+export const safeName = (reservedNames: string[], desiredName: string): string => {
+  if (reservedNames.includes(desiredName)) {
+    const newName = `${desiredName}-${getRandomChars(5)}`;


I'd have gone with the default (6) or shorten it to 3 (half). Not sure this makes really any difference though. Was there a scenario you were worried about using less characters? (ie, any specific reason for 5?)

No specific reason. Changed it to 3

openshift-ci-robot · 2020-08-21T18:12:09Z

@debsmita1: This pull request references Bugzilla bug 1869600, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1869600: add resource icon to the items in the select task dropdown in the pipeline builder page

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/test-infra repository.

andrewballantyne

/lgtm

andrewballantyne · 2020-08-21T18:18:41Z

/lgtm cancel

@divyanshiGupta Actually could you do a review here as well. Trying to encourage two point reviews. Didn't see you didn't add a lgtm until I did.

invincibleJai

Verified the changes looks good !!

invincibleJai · 2020-08-24T10:53:48Z

/lgtm

openshift-ci-robot · 2020-08-24T10:54:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, debsmita1, invincibleJai

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:

~~frontend/packages/dev-console/OWNERS~~ [andrewballantyne,invincibleJai]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

debsmita1 · 2020-08-24T17:30:58Z

/test e2e-gcp-console

openshift-bot · 2020-08-24T18:56:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-24T19:10:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-08-24T22:21:26Z

@debsmita1: All pull requests linked via external trackers have merged: openshift/console#6345. Bugzilla bug 1869600 has been moved to the MODIFIED state.

In response to this:

Bug 1869600: add resource icon to the items in the select task dropdown in the pipeline builder page

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/test-infra repository.

debsmita1 · 2020-08-25T05:50:08Z

/cherrypick release-4.5

openshift-cherrypick-robot · 2020-08-25T05:50:17Z

@debsmita1: #6345 failed to apply on top of branch "release-4.5":

Applying: add resource icon to task and cluster-task
Using index info to reconstruct a base tree...
M	frontend/packages/dev-console/src/components/pipelines/pipeline-builder/update-utils.ts
M	frontend/packages/dev-console/src/components/pipelines/pipeline-topology/TaskListNode.tsx
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/dev-console/src/components/pipelines/pipeline-topology/TaskListNode.tsx
CONFLICT (content): Merge conflict in frontend/packages/dev-console/src/components/pipelines/pipeline-topology/TaskListNode.tsx
Auto-merging frontend/packages/dev-console/src/components/pipelines/pipeline-builder/update-utils.ts
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 add resource icon to task and cluster-task
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-4.5

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/test-infra repository.

openshift-ci-robot added the component/dev-console Related to dev-console label Aug 17, 2020

openshift-ci-robot requested review from jeff-phillips-18 and nemesis09 August 17, 2020 17:21

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 17, 2020

debsmita1 changed the title ~~add resource icon to task and cluster-task~~ Bug 1869600: add resource icon to the items in the select task dropdown in the pipeline builder page Aug 18, 2020

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 18, 2020

debsmita1 force-pushed the add-resource-badge branch 2 times, most recently from c065fd2 to 4283248 Compare August 19, 2020 14:27

andrewballantyne suggested changes Aug 20, 2020

View reviewed changes

debsmita1 force-pushed the add-resource-badge branch from 4283248 to af64a03 Compare August 21, 2020 17:39

openshift-ci-robot added the component/core Related to console core functionality label Aug 21, 2020

debsmita1 requested a review from andrewballantyne August 21, 2020 17:40

andrewballantyne suggested changes Aug 21, 2020

View reviewed changes

add resource icon to task and cluster-task

dd1e7ed

debsmita1 force-pushed the add-resource-badge branch from af64a03 to dd1e7ed Compare August 21, 2020 18:09

andrewballantyne approved these changes Aug 21, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Aug 21, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 21, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020

andrewballantyne approved these changes Aug 21, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2020

invincibleJai approved these changes Aug 24, 2020

View reviewed changes

openshift-ci-robot assigned invincibleJai Aug 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2020

openshift-merge-robot merged commit b544100 into openshift:master Aug 24, 2020

debsmita1 mentioned this pull request Aug 26, 2020

Bug 1872625: cherry-pick 4.5 add resource icon to task and cluster-task #6446

Merged

spadgett added this to the v4.6 milestone Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1869600: add resource icon to the items in the `select task` dropdown in the pipeline builder page #6345

Bug 1869600: add resource icon to the items in the `select task` dropdown in the pipeline builder page #6345

debsmita1 commented Aug 17, 2020 •

edited

Loading

debsmita1 commented Aug 17, 2020

openshift-ci-robot commented Aug 18, 2020

divyanshiGupta commented Aug 18, 2020

debsmita1 commented Aug 19, 2020

andrewballantyne commented Aug 19, 2020

bgliwa01 commented Aug 19, 2020

andrewballantyne left a comment

andrewballantyne Aug 20, 2020

andrewballantyne Aug 20, 2020

andrewballantyne Aug 20, 2020

debsmita1 Aug 21, 2020

andrewballantyne Aug 21, 2020 •

edited

Loading

debsmita1 commented Aug 21, 2020

andrewballantyne left a comment

andrewballantyne Aug 21, 2020

andrewballantyne Aug 21, 2020

andrewballantyne Aug 21, 2020

andrewballantyne Aug 21, 2020

debsmita1 Aug 21, 2020

openshift-ci-robot commented Aug 21, 2020

andrewballantyne left a comment

andrewballantyne commented Aug 21, 2020

invincibleJai left a comment

invincibleJai commented Aug 24, 2020

openshift-ci-robot commented Aug 24, 2020

debsmita1 commented Aug 24, 2020

openshift-bot commented Aug 24, 2020

openshift-bot commented Aug 24, 2020

openshift-ci-robot commented Aug 24, 2020

debsmita1 commented Aug 25, 2020

openshift-cherrypick-robot commented Aug 25, 2020

Bug 1869600: add resource icon to the items in the select task dropdown in the pipeline builder page #6345

Bug 1869600: add resource icon to the items in the select task dropdown in the pipeline builder page #6345

Conversation

debsmita1 commented Aug 17, 2020 • edited Loading

debsmita1 commented Aug 17, 2020

openshift-ci-robot commented Aug 18, 2020

divyanshiGupta commented Aug 18, 2020

debsmita1 commented Aug 19, 2020

andrewballantyne commented Aug 19, 2020

bgliwa01 commented Aug 19, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

debsmita1 commented Aug 21, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 21, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Aug 21, 2020

invincibleJai left a comment

Choose a reason for hiding this comment

invincibleJai commented Aug 24, 2020

openshift-ci-robot commented Aug 24, 2020

debsmita1 commented Aug 24, 2020

openshift-bot commented Aug 24, 2020

openshift-bot commented Aug 24, 2020

openshift-ci-robot commented Aug 24, 2020

debsmita1 commented Aug 25, 2020

openshift-cherrypick-robot commented Aug 25, 2020

Bug 1869600: add resource icon to the items in the `select task` dropdown in the pipeline builder page #6345

Bug 1869600: add resource icon to the items in the `select task` dropdown in the pipeline builder page #6345

debsmita1 commented Aug 17, 2020 •

edited

Loading

andrewballantyne Aug 21, 2020 •

edited

Loading