Edit pipeline builder #4055

andrewballantyne · 2020-01-23T17:46:40Z

Fixes:
Folded the PRs into this one now that it has been reviewed by @christianvogt - Additional changes will be more commits

Dependency on #4004 (https://issues.redhat.com/browse/ODC-2605) -- 1st commit
Dependency on #4031 (https://issues.redhat.com/browse/ODC-2448) -- 2nd commit
Dependency on #4033 (https://issues.redhat.com/browse/ODC-2447) -- 3rd commit
Dependency on #4049 (https://issues.redhat.com/browse/ODC-2449) -- 4th commit
https://issues.redhat.com/browse/ODC-2450 -- 5th commit

Tickets covered are:

https://issues.redhat.com/browse/ODC-2605 (visualization foundation)
https://issues.redhat.com/browse/ODC-2448 (builder foundation)
https://issues.redhat.com/browse/ODC-2447 (page)
https://issues.redhat.com/browse/ODC-2449 (sidebar)
https://issues.redhat.com/browse/ODC-2450 (edit existing)

Additional comments beyond the 1st one are all reviewer feedback.

Analysis / Root cause:
I've completed walked through the builder and produced a Pipeline I like. There however is an issue with the way I set it up - it'd be nice to go back through the builder to adjust it.

Solution Description:
Inject an existing Pipeline into the PipelineBuilder as to start off with a pre-built structure.

Screen shots / Gifs for design review:

There is a laundry list of touch ups and writing tests I need to do. But this is the essence of the Pipeline Builder (and all 4 previous PRs build it up in different ways).

Unit test coverage report:

Unit Tests

Test setup:

Install the Pipeline Operator (this comes with cluster tasks from 0.8.x onwards)
Go to the Pipelines page and "Create Pipeline"

Browser conformance:

Chrome
Firefox
Safari
Edge

andrewballantyne · 2020-01-23T18:14:11Z

Was waiting for the initial bot to kick in... but it seems busy with other things.

/kind feature
/assign @christianvogt

cc @openshift/team-devconsole-ux

serenamarie125

Looking good 🎉

sspeiche · 2020-01-23T18:25:24Z

Looks great, love the "this is not the param you are looking for" sample text

andrewballantyne · 2020-01-23T18:28:50Z

Looks great, love the "this is not the param you are looking for" sample text

To be honest, forgot that was there lol. Wasn't reading the text when I was recording. It's part of my test param Pipeline I have, so if this shows up in the logs I made a bug somewhere 😅

christianvogt · 2020-01-23T18:40:39Z

...ole/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.tsx

@@ -142,8 +142,8 @@ const TaskComponent: React.FC<TaskProps> = ({
    </>
  );
  return (
-    <li className={cx('odc-pipeline-vis-task')}>
+    <div className={cx('odc-pipeline-vis-task')}>


no need to use classnames lib here for a single class

christianvogt · 2020-01-23T18:44:56Z

...kages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderVisualization.tsx

+  return (
+    <PipelineTopologyGraph
+      // TODO: fix this; the graph layout isn't properly laying out nodes
+      key={nodes.map((n) => n.id).join('-')}


No need for a key here.

Layout is a pain atm, if I don't force a re-render it doesn't properly layout nodes when new ones are added. Have an intent to circle back to this once Jeff's stuff is in.

christianvogt · 2020-01-23T18:47:14Z

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

+  const [clusterTasks, setClusterTasks] = React.useState<PipelineResourceTask[]>(null);
+  const [loadErrorMsg, setLoadErrorMsg] = React.useState<string>(undefined);
+
+  React.useEffect(() => {


These async calls to update state need to be guarded against updating state on an unmounted component.

christianvogt · 2020-01-23T18:47:46Z

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

+          .then((res: PipelineResourceTask[]) => {
+            setNamespacedTasks(res);
+          })


nit:

.then(setNamespacedTasks)

christianvogt · 2020-01-23T18:48:20Z

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

+        .then((res: PipelineResourceTask[]) => {
+          setClusterTasks(res);
+        })


nit:

.then(setClusterTasks)

christianvogt · 2020-01-23T19:13:03Z

...ages/dev-console/src/components/pipelines/pipeline-topology/PipelineVisualizationSurface.tsx

+    <div style={{ height: maxSize?.height, width: maxSize?.width }}>
+      <VisualizationSurface visualization={vis} />


Need to improve topology to support sizing based on contents.

Pipeline code (this code)? Or Topology?

Topology. This is fine for now

christianvogt · 2020-01-23T19:13:41Z

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

+const SpacerNode: React.FC<{ element: Node }> = () => {
+  return <g />;
+};


Dagre trickery?

To get the design we want, yeah... parallel to parallel is just straight lines to Dagre... placing a node as a focal point allows them to flow to an invisible point and then flow back out.

christianvogt · 2020-01-23T19:16:04Z

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

+          Select task <CaretDownIcon />
+        </Button>
+      </div>
+      <Popper


We need to extract this and the other instances into a single reusable component later.

christianvogt · 2020-01-23T19:31:08Z

frontend/packages/dev-console/src/plugin.tsx

+    properties: {
+      perspective: 'dev',
+      exact: true,
+      path: [`/k8s/ns/:ns/${referenceForModel(PipelineModel)}/~new/:pipelineName/builder`],


Why ~new in the edit url?
/k8s/ns/:ns/${referenceForModel(PipelineModel)}/:pipelineName/builder

I did that originally... it's caught by the Details page and thinks I want a tab in there. I am unsure how to override that. Order in the Plugin file doesn't seem to do it.

Apparently I was wrong... not sure how I tested that and didn't get it to work. 🤷‍♂ Fixed now.

christianvogt · 2020-01-23T19:31:47Z

frontend/packages/dev-console/src/utils/pipeline-actions.tsx

+    const {
+      metadata: { name, namespace },
+    } = pipeline;
+    history.push(`/k8s/ns/${namespace}/${referenceForModel(PipelineModel)}/~new/${name}/builder`);


Suggested change

history.push(`/k8s/ns/${namespace}/${referenceForModel(PipelineModel)}/~new/${name}/builder`);

history.push(`/k8s/ns/${namespace}/${referenceForModel(PipelineModel)}/${name}/builder`);

christianvogt · 2020-01-23T19:40:03Z

Clicked the right + button which resulted in the bottom right task being created. Expected it created between the two nodes. Possibly because the middle node had no task set. If i set the task first then it works correctly.

Probably a related issue. Clicked all three + signs on the first task set and ended up with this. Expected the middle task to have all other tasks connected but the first task got the bottom one.

andrewballantyne · 2020-01-23T23:26:46Z

/retest

abhinandan13jan · 2020-01-24T11:01:09Z

...le/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.scss

@@ -2,7 +2,6 @@ $border-color: var(--pf-global--BorderColor--light-100);
 .odc-pipeline-vis-task {
  position: relative;
  width: 10em;
-  margin-top: 1.5em;


nit: I understand this is an existing file... However, should we use pf-react-tokens instead later

Not sure I understand. Your problem is with the sass styles or the hardcoded value? I believe I made the margin a pf variable when I moved it.

this is not something urgent. While going through the code, I just thought that the remaining styles like width:10em, should they be converted to pf units. This could be taken up in later PRs

Ah gotcha... you mean the width haha... I thought you were talking about the margin-top

This was all done at the time to make the nodes reactive to font size. The layout was dynamic too.
Now that we have a fixed layout and node sizes at can clean up the old code later

On my cleanup list 😞

karthikjeeyar · 2020-01-24T11:23:04Z

By adding a node in between two other nodes and removing it from the side panel breaks the UI.

I observed same issue when introducing a left node to the very first task and by removing it from sidepanel

abhinandan13jan · 2020-01-24T11:25:37Z

Suppose I want to arrive at this,

If I follow in order and do the following to the first task -> click bottom + then right + of first task , I get to this.
But, if I do it in order -> click right + then click bottom + of first task, I have no further way to do this and there is no reset.So, I have to cancel and come back. Is this expected?

abhinandan13jan · 2020-01-24T11:30:51Z

clicking on the task bubble gave this issue

backend.js:6 TypeError: Cannot read property 'value' of undefined
The above error occurred in the <TaskSidebarParam> component:
    in TaskSidebarParam (created by TaskSidebar)
    in div (created by TaskSidebar)
    in TaskSidebar (created by Sidebar)
    in div (created by Sidebar)
    in Transition (created by CSSTransition)

abhinandan13jan · 2020-01-24T11:34:18Z

While changing the name in the Sidebar for one of the buildah task, I backspaced till zero characters and got this issue.

 Error: Edge with ID 'build-app~to~' has no target.
    at BaseEdge.push../packages/topology/src/elements/BaseEdge.ts.BaseEdge.getTarget (:9000/static/dev-console-topology~git-import-form~pipeline-builder-edit-page~pipeline-builder-page~pipeline-detai~5e936f59-1d3453939c1c018d376a.js:3925)

andrewballantyne · 2020-01-24T11:44:20Z

#4055 (comment)
Unfortunately there are UX issues with the design. I've not had time to vet this with Serena and come up with a new design. Once a node on the right side of the graph is not pointing at anything there is no way to rejoin it with another line. Not sure if I misunderstood the design but I'll need to discuss with UX.

andrewballantyne · 2020-01-24T11:45:25Z

#4055 (comment) ah, damn. That was overlooked. Good catch. Live updating the form + visualization has its issues haha.

andrewballantyne · 2020-01-24T11:47:11Z

#4055 (comment)
Yeah, I spotted this late yesterday. The task setup needs some work. When one is removed I don't stitch it back together. Need to rework some stuff to reuse the logic.

abhinandan13jan · 2020-01-24T13:09:11Z

The Builder fails to load over Create/Edit flow in Admin perspective because of route issues

andrewballantyne · 2020-01-24T15:26:11Z

/test e2e-gcp-console

openshift-bot · 2020-01-25T08:33:12Z

/retest

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

andrewballantyne · 2020-01-25T09:14:43Z

/retest

openshift-bot · 2020-01-25T09:51:56Z

/retest

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

openshift-bot · 2020-01-25T14:41:53Z

/retest

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

openshift-bot · 2020-01-25T14:50:36Z

/retest

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

openshift-bot · 2020-01-25T15:03:18Z

/retest

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

openshift-bot · 2020-01-25T15:16:18Z

/retest

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

andrewballantyne · 2020-01-25T15:53:52Z

/retest

openshift-bot · 2020-01-25T15:55:14Z

/retest

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

andrewballantyne · 2020-01-25T16:09:06Z

/hold

Looks like there is another flake that is preventing any tests from running.

andrewballantyne · 2020-01-25T19:15:09Z

/retest

andrewballantyne · 2020-01-25T19:37:52Z

/hold cancel

Looks like it's good to go again.

andrewballantyne · 2020-01-25T20:37:48Z

/retest

andrewballantyne · 2020-01-25T22:01:14Z

/test e2e-gcp-console

andrewballantyne · 2020-01-25T23:26:17Z

/retest

serenamarie125 · 2020-01-26T01:00:21Z

/lgtm

openshift-ci-robot · 2020-01-26T01:00:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, andrewballantyne, christianvogt, karthikjeeyar, serenamarie125

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,christianvogt]
~~frontend/packages/topology/OWNERS~~ [christianvogt]

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

andrewballantyne · 2020-01-26T03:03:47Z

/retest

openshift-bot · 2020-01-26T03:11:15Z

/retest

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

andrewballantyne · 2020-01-26T04:07:53Z

/retest

openshift-bot · 2020-01-26T04:29:11Z

/retest

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

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 23, 2020

openshift-ci-robot assigned christianvogt Jan 23, 2020

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 23, 2020

serenamarie125 approved these changes Jan 23, 2020

View reviewed changes

openshift-ci-robot requested review from abhi-kn and jeff-phillips-18 January 23, 2020 18:56

openshift-ci-robot added the component/dev-console Related to dev-console label Jan 23, 2020

christianvogt reviewed Jan 23, 2020

View reviewed changes

andrewballantyne force-pushed the edit-pipeline-builder branch 2 times, most recently from 30b5b65 to a98f8f8 Compare January 23, 2020 22:50

This was referenced Jan 23, 2020

Pipeline builder task sidebar #4049

Closed

Pipeline builder page #4033

Closed

Pipeline builder visualization #4031

Closed

Pipeline Topology Implementation #4004

Closed

abhinandan13jan reviewed Jan 24, 2020

View reviewed changes

andrewballantyne force-pushed the edit-pipeline-builder branch from 97597d7 to 52cbb05 Compare January 24, 2020 14:08

Pipeline Builder

6b60f12

andrewballantyne force-pushed the edit-pipeline-builder branch from bd36046 to 6b60f12 Compare January 25, 2020 15:56

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

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2020

openshift-ci-robot assigned serenamarie125 Jan 26, 2020

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

openshift-merge-robot merged commit 4168efa into openshift:master Jan 26, 2020

spadgett added this to the v4.4 milestone Jan 27, 2020

		<div style={{ height: maxSize?.height, width: maxSize?.width }}>
		<VisualizationSurface visualization={vis} />

	history.push(`/k8s/ns/${namespace}/${referenceForModel(PipelineModel)}/~new/${name}/builder`);
	history.push(`/k8s/ns/${namespace}/${referenceForModel(PipelineModel)}/${name}/builder`);

Edit pipeline builder #4055

Edit pipeline builder #4055

Conversation

andrewballantyne commented Jan 23, 2020 • edited

andrewballantyne commented Jan 23, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

sspeiche commented Jan 23, 2020

andrewballantyne commented Jan 23, 2020

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

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

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

christianvogt commented Jan 23, 2020 • edited

andrewballantyne commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinandan13jan Jan 24, 2020 • edited

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

karthikjeeyar commented Jan 24, 2020

abhinandan13jan commented Jan 24, 2020 • edited

abhinandan13jan commented Jan 24, 2020 • edited

abhinandan13jan commented Jan 24, 2020 • edited

andrewballantyne commented Jan 24, 2020

andrewballantyne commented Jan 24, 2020

andrewballantyne commented Jan 24, 2020

abhinandan13jan commented Jan 24, 2020 • edited

andrewballantyne commented Jan 24, 2020

openshift-bot commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020

openshift-bot commented Jan 25, 2020

openshift-bot commented Jan 25, 2020

openshift-bot commented Jan 25, 2020

openshift-bot commented Jan 25, 2020

openshift-bot commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020

openshift-bot commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020 • edited

andrewballantyne commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020

andrewballantyne commented Jan 25, 2020

serenamarie125 commented Jan 26, 2020

openshift-ci-robot commented Jan 26, 2020

andrewballantyne commented Jan 26, 2020

openshift-bot commented Jan 26, 2020

andrewballantyne commented Jan 26, 2020

openshift-bot commented Jan 26, 2020

andrewballantyne commented Jan 23, 2020 •

edited

christianvogt commented Jan 23, 2020 •

edited

abhinandan13jan Jan 24, 2020 •

edited

abhinandan13jan commented Jan 24, 2020 •

edited

abhinandan13jan commented Jan 24, 2020 •

edited

abhinandan13jan commented Jan 24, 2020 •

edited

abhinandan13jan commented Jan 24, 2020 •

edited

andrewballantyne commented Jan 25, 2020 •

edited