Bug 1801857: Cleanup Pipeline Builder #4240

andrewballantyne · 2020-02-07T00:23:10Z

Fixes:

https://issues.redhat.com/browse/ODC-2854

Analysis / Root cause:

Pipeline Builder landed in a really rough shape, this PR should clean up a lot of the issues and serve to enable it again in the UI.

Solution Description:

Lots of cleanup and consolidation happened with this fix.

Screen shots / Gifs for design review:

cc @openshift/team-devconsole-ux

Unit test coverage report:

Unit tests still to come (may come in a different PR for ease of reviewing)

Test setup:

Pipeline Operator is needed, but otherwise entering through the "create pipeline" button on the Pipeline list page is all you need to do.

Browser conformance:

Chrome
Firefox
Safari
Edge

serenamarie125

Still seeing the column headers when there are no pipeline parameters and resources. Unsure if that is part of another fix?

serenamarie125

wow i'm impressed, really nice job with this Andrew! Look forward to looking at it in detail soon

andrewballantyne · 2020-02-07T01:03:32Z

Still seeing the column headers when there are no pipeline parameters and resources. Unsure if that is part of another fix?

@serenamarie125 Yup, see #4180

andrewballantyne · 2020-02-07T01:06:47Z

wow i'm impressed, really nice job with this Andrew! Look forward to looking at it in detail soon

Thanks @serenamarie125 - I've just pushed another commit to try and fix the odd balance of padding around the visualization. Not sure why, but the layout engine has some odd padding on the top and left that it didn't have when I originally did this work pre-ff.

Hopefully we get a good run down tomorrow and shake out the last few things that are a bit odd in the UX department.

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

abhinandan13jan · 2020-02-11T12:52:59Z

@andrewballantyne Ran this locally and it works well. Just left comments at two places where variable naming could be more descriptive.

This takes care of the earlier issues. However, I just felt the connectors are a bit confusing.
Suppose I have two parallel tasks and I want to add a single task to run either before both of them(previous stage) or after both of them(next stage). Apart from this one thing, the PR looks good. The connectors displayed confuse me. IMO, connectors might work better if they are stage based. Let me know if this looks ok as of now?

Parallel tasks

Adding a task before them

Adding a task after them

but it should have been

abhinandan13jan · 2020-02-11T12:53:59Z

/test analyze

andrewballantyne · 2020-02-11T13:06:41Z

@abhinandan13jan I discussed this with @serenamarie125 - it will likely be a limitation for the first pass of the Pipeline Builder. We had talked about an "advanced" mode but we decided to keep it to the straight 3 actions (all of which will not connect disconnected paths).

andrewballantyne · 2020-02-11T15:58:16Z

Resource selection issue is related to the fix Rohit is doing in #4266

frontend/packages/dev-console/src/components/pipelines/pipeline-topology/factories.ts

openshift-ci-robot · 2020-02-11T18:39:15Z

@andrewballantyne: This pull request references Bugzilla bug 1801857, which is invalid:

expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1801857: Cleanup Pipeline Builder

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 · 2020-02-11T18:40:02Z

/bugzilla refresh

openshift-ci-robot · 2020-02-11T18:40:08Z

@andrewballantyne: This pull request references Bugzilla bug 1801857, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

/bugzilla refresh

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.

abhinandan13jan · 2020-02-12T08:53:38Z

/lgtm

christianvogt · 2020-02-14T03:08:08Z

...end/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderHeader.tsx

Why don't you use a flex layout and align items center?

christianvogt · 2020-02-14T03:10:21Z

frontend/packages/dev-console/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx

Why the cast?

Hold over from the previous design I was working with... there was a type error I couldn't figure out, so I casted it to get around it.

Removing

christianvogt · 2020-02-14T03:11:21Z

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

Use consistent names / values.

christianvogt · 2020-02-14T03:13:16Z

.../packages/dev-console/src/components/pipelines/pipeline-builder/task-sidebar/TaskSidebar.tsx

Suggested change

{inputResources && inputResources.map(renderResource('inputs'))}

{inputResources.map(renderResource('inputs'))}

christianvogt · 2020-02-14T03:13:24Z

.../packages/dev-console/src/components/pipelines/pipeline-builder/task-sidebar/TaskSidebar.tsx

Suggested change

{outputResources && outputResources.map(renderResource('outputs'))}

{outputResources.map(renderResource('outputs'))}

christianvogt · 2020-02-14T03:17:34Z

...ages/dev-console/src/components/pipelines/pipeline-builder/task-sidebar/TaskSidebarParam.tsx

Why aren't you using the formik fields we have for this?

I need to do custom save logic and Formik messing with the value directly caused desync issues with the errors + updating of the values. I figured I'd just manage all things Task related and let Formik manage the form items (name, params, resources on the main page).

I suspect I could find a way to make the logic exist in the yup schema... but I have not yet tackled that.

christianvogt · 2020-02-14T03:18:00Z

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

why & {} ?
Can it contain any field with any value?

I don't think {} means anything special... it just means an empty object (no key/values). Which in large is useless... I'll remove it.

It was a hold over from when I didn't have PipelineBuilderTaskBase that contained common items. I just never removed it when I was cleaning it up.

christianvogt · 2020-02-14T03:24:31Z

Tried to edit the pipeline and save it while in the following state:

christianvogt · 2020-02-14T03:26:01Z

During edit, i changed the resource name. The node didn't become an error. The save button was enabled. Saving resulted in an error.

christianvogt · 2020-02-14T20:18:43Z

/lgtm
/approve

openshift-ci-robot · 2020-02-14T20:19:33Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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-02-14T21:30:02Z

/retest

openshift-bot · 2020-02-14T21:31:42Z

/retest

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

openshift-ci-robot · 2020-02-15T00:14:35Z

@andrewballantyne: All pull requests linked via external trackers have merged. Bugzilla bug 1801857 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1801857: Cleanup Pipeline Builder

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 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 7, 2020

openshift-ci-robot requested review from abhi-kn and jeff-phillips-18 February 7, 2020 00:23

openshift-ci-robot added the component/dev-console Related to dev-console label Feb 7, 2020

andrewballantyne force-pushed the cleanup-pipeline-builder branch from 75e3e2a to d93857b Compare February 7, 2020 00:34

openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Feb 7, 2020

serenamarie125 reviewed Feb 7, 2020

View reviewed changes

andrewballantyne force-pushed the cleanup-pipeline-builder branch from d93857b to 5b5f7c6 Compare February 7, 2020 01:05

abhinandan13jan reviewed Feb 11, 2020

View reviewed changes

...ole/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.tsx Outdated Show resolved Hide resolved

abhinandan13jan reviewed Feb 11, 2020

View reviewed changes

...ole/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.tsx Outdated Show resolved Hide resolved

andrewballantyne force-pushed the cleanup-pipeline-builder branch from 5b5f7c6 to b5e3c9e Compare February 11, 2020 15:40

jeff-phillips-18 reviewed Feb 11, 2020

View reviewed changes

frontend/packages/dev-console/src/components/pipelines/pipeline-topology/factories.ts Outdated Show resolved Hide resolved

andrewballantyne changed the title ~~Cleanup Pipeline Builder~~ Bug 1801857: Cleanup Pipeline Builder Feb 11, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 11, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 11, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 11, 2020

openshift-ci-robot assigned abhinandan13jan Feb 12, 2020

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

andrewballantyne force-pushed the cleanup-pipeline-builder branch from b5e3c9e to 202a761 Compare February 13, 2020 19:58

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

andrewballantyne force-pushed the cleanup-pipeline-builder branch from 202a761 to 5ea443a Compare February 13, 2020 20:20

christianvogt reviewed Feb 14, 2020

View reviewed changes

andrewballantyne force-pushed the cleanup-pipeline-builder branch from fab3489 to 1cbfd98 Compare February 14, 2020 17:31

Cleanup Pipeline Builder

6dfc737

andrewballantyne force-pushed the cleanup-pipeline-builder branch from 2f4f0f6 to 6dfc737 Compare February 14, 2020 20:14

openshift-ci-robot assigned christianvogt Feb 14, 2020

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

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

openshift-merge-robot merged commit e36c523 into openshift:master Feb 15, 2020

spadgett added this to the v4.4 milestone Feb 17, 2020

	{inputResources && inputResources.map(renderResource('inputs'))}
	{inputResources.map(renderResource('inputs'))}

	{outputResources && outputResources.map(renderResource('outputs'))}
	{outputResources.map(renderResource('outputs'))}

Bug 1801857: Cleanup Pipeline Builder #4240

Bug 1801857: Cleanup Pipeline Builder #4240

Uh oh!

Conversation

andrewballantyne commented Feb 7, 2020

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

andrewballantyne commented Feb 7, 2020

Uh oh!

Uh oh!

Uh oh!

abhinandan13jan commented Feb 11, 2020

Uh oh!

abhinandan13jan commented Feb 11, 2020

Uh oh!

andrewballantyne commented Feb 11, 2020

Uh oh!

andrewballantyne commented Feb 11, 2020

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 11, 2020

Uh oh!

andrewballantyne commented Feb 11, 2020

Uh oh!

openshift-ci-robot commented Feb 11, 2020

Uh oh!

abhinandan13jan commented Feb 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

christianvogt commented Feb 14, 2020

Uh oh!

christianvogt commented Feb 14, 2020

Uh oh!

christianvogt commented Feb 14, 2020

Uh oh!

openshift-ci-robot commented Feb 14, 2020

Uh oh!

andrewballantyne commented Feb 14, 2020

Uh oh!

openshift-bot commented Feb 14, 2020

Uh oh!

openshift-ci-robot commented Feb 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

andrewballantyne commented Feb 7, 2020 •

edited

Loading

andrewballantyne Feb 14, 2020 •

edited

Loading