Pipeline builder visualization #4031

andrewballantyne · 2020-01-21T19:51:49Z

Fixes:
Dependency on #4004 (https://issues.redhat.com/browse/ODC-2605) -- 1st commit
https://issues.redhat.com/browse/ODC-2448 -- 2nd commit

Do review of the second commit while we wait for #4004 to get in.

Analysis / Root cause:
Using a hugely skeleton page, mount and render a Pipeline Builder Visualization.

Solution Description:
Using the work done in PR #4004, I've implemented a couple new nodes, supported a different set of layout options and fixed a couple layout rendering issues with parallel-to-parallel that are not in #4004.

New Nodes:

ListTaskNode - which has functionality for selecting from existing ClusterTasks or current-namespace Tasks
BuilderNode - which comes with the ability to create new nodes with a runAfter reference to, from or to share in the same runAfters of the current node (these create ListTaskNodes)

Screen shots / Gifs for design review:
The magic in action:

Current caveats:

[Design] There is no way to rejoin once you reach two parallel nodes at the "last column"; they remain indefinitely parallel
[Design, Code] I've removed selected tasks from the dropdown -- this is not ideal and I intend to fix it, but it's unfortunately a mistake I made early and some refactoring of the way the callbacks / nodes are built is needed (id === taskName, and only one node can exist with a given id)
[Code] There is a flicker on the first node selection because I am tossing the stage away on every new node... this is because the layout refuses to relayout my nodes on each pass -- I've tried Jeff's PR (Topology: Fire an event when graph layout completes #4023) and it sorta fixes the issue but not fully - need to rework this once both of our PRs are in
[Design] The tooltip is still getting in the way - do I disable it? Do I make it pop-up?
[Design] Two nodes next to each other slightly overlap "(+)" and the one on the right overlaps.
[Design, Code] The nodes won't always remain on the parallel path they are on... sometimes the two (or more) parallel paths will swap when a new node is added; this is obviously undesirable for the user, will need to figure out a way to avoid that (may be fixed when I stop throwing away the layout on every new node, re Jeff's fix)

Unit test coverage report:

Write 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-21T19:53:02Z

/kind feature
/hold
/assign @christianvogt

cc @openshift/team-devconsole-ux

openshift-ci-robot · 2020-01-21T19:57:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andrewballantyne
To complete the pull request process, please assign christianvogt
You can assign the PR to them by writing /assign @christianvogt in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [andrewballantyne]
frontend/packages/topology/OWNERS

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

openshift-ci-robot · 2020-01-21T22:07:21Z

@andrewballantyne: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-console	`b59a470`	link	`/test e2e-gcp-console`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

serenamarie125

this is a bit picky :) but the first time you select a task, it looks like the whole visualizatoin "blinks", but it doesn't seem to do that as you select subsequent tasks

andrewballantyne · 2020-01-22T02:46:37Z

this is a bit picky :) but the first time you select a task, it looks like the whole visualizatoin "blinks", but it doesn't seem to do that as you select subsequent tasks

Totally valid, I'd like to fix it too. I noted this in the description:

[Code] There is a flicker on the first node selection because I am tossing the stage away on every new node... this is because the layout refuses to relayout my nodes on each pass -- I've tried Jeff's PR (#4023) and it sorta fixes the issue but not fully - need to rework this once both of our PRs are in

abhinandan13jan · 2020-01-22T11:25:42Z

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

-    ? `${resourcePathFromModel(PipelineRunModel, pipelineRun, namespace)}/logs/${name}`
+  const path = pipelineRunName
+    ? `${resourcePathFromModel(PipelineRunModel, pipelineRunName, namespace)}/logs/${name}`
    : undefined;


do we need to keep the undefined?

It appears not, but this was pre-existing code. My change was just to change the variable away from pipelineRun because that implicitly means the K8s resource and not just the name.

abhinandan13jan · 2020-01-22T11:28:32Z

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

+import { convertResourceToTask } from './utils';
+
+type UseTasks = {
+  namespacedTasks: K8sResourceKind[] | null;


we dont need | null, do we?

No, I suppose that is true... I was just trying to the actual values. I guess any object can be null.

abhinandan13jan · 2020-01-22T11:29:42Z

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

+            setNamespacedTasks(res);
+          })
+          .catch(() => {
+            setLoadErrorMsg(`Failed to load namespace Tasks. ${loadErrorMsg || ''}`);


do we need || ''

I believe so...

let value = null; `Hello World, ${value}` // "Hello World, null"

jeff-phillips-18 · 2020-01-22T22:04:59Z

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

+      onFocus={() => setShowAdd(true)}
+      onBlur={() => setShowAdd(false)}
+      onMouseOver={() => setShowAdd(true)}
+      onMouseOut={() => setShowAdd(false)}


You could use the topology package useHover here for mouse over and mouse out. That gives you the default delay for changing state on hovers.

Oooh, how fancy. I'll be sure to update it to do that.

jeff-phillips-18 · 2020-01-23T22:31:24Z

Double scroll bars when tooltip is displayed below the visual area.
Can't get to the horizontal scrollbar without scrolling to the bottom
Graph and header should have some outer padding.

andrewballantyne · 2020-01-23T22:40:09Z

Double scroll bars when tooltip is displayed below the visual area.
Can't get to the horizontal scrollbar without scrolling to the bottom
Graph and header should have some outer padding.

haha, yeah... this was just a shim page. The proper page has scrollbars built in and shouldn't have this issue. Good review catch though, easily could be missed.

andrewballantyne · 2020-01-23T22:54:04Z

Folded into #4055 for CI sanity.

Pipeline Topology Implementation

5f923f2

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

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

openshift-ci-robot assigned christianvogt Jan 21, 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 21, 2020

openshift-ci-robot requested review from debsmita1 and jeff-phillips-18 January 21, 2020 19:57

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

Pipeline Builder Visualization

b59a470

andrewballantyne force-pushed the pipeline-builder-visualization branch from da81c22 to b59a470 Compare January 21, 2020 20:33

andrewballantyne mentioned this pull request Jan 21, 2020

Pipeline builder page #4033

Closed

5 tasks

serenamarie125 reviewed Jan 22, 2020

View reviewed changes

abhinandan13jan reviewed Jan 22, 2020

View reviewed changes

jeff-phillips-18 reviewed Jan 22, 2020

View reviewed changes

This was referenced Jan 23, 2020

Pipeline builder task sidebar #4049

Closed

Edit pipeline builder #4055

Merged

andrewballantyne closed this Jan 23, 2020

Pipeline builder visualization #4031

Pipeline builder visualization #4031

Uh oh!

Conversation

andrewballantyne commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Do review of the second commit while we wait for #4004 to get in.

Uh oh!

andrewballantyne commented Jan 21, 2020

Uh oh!

openshift-ci-robot commented Jan 21, 2020

Uh oh!

openshift-ci-robot commented Jan 21, 2020

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

andrewballantyne commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 23, 2020

Uh oh!

andrewballantyne commented Jan 23, 2020

Uh oh!

andrewballantyne commented Jan 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andrewballantyne commented Jan 21, 2020 •

edited

Loading