Skip to content

Conversation

@andrewballantyne
Copy link
Contributor

@andrewballantyne andrewballantyne commented Jan 19, 2020

Fixes:
https://issues.redhat.com/browse/ODC-2605

Analysis / Root cause:
Implements the foundation of the Pipeline Builder and replaces the existing rendering logic to be more inline with what the Pipeline spec can present as a node network.

Solution Description:
Pulled the @console/topology code into Pipelines. This gives us the power of a layout engine and a free and open canvas to draw in.

Screen shots / Gifs for design review:
@openshift/team-devconsole-ux
Screen Shot 2020-01-18 at 7 37 34 PM

Note: the top visualization is the same rendering issue from https://issues.redhat.com/browse/ODC-2464 fixed.

There are some unexpected outcomes out of the layout as of right now:

Screen Shot 2020-01-18 at 7 37 45 PM

I have no current intent on addressing the above issues until after the Pipeline Builder feature is completed. We'll likely need to work on a constraint system with Dagre which may use "ghost" groups or something (unsure of the solution at this time).

Pipeline Runs:

PipelineTopology-ComplexPipelineRun

PipelineTopology-P2P

Unit test coverage report:

  • TODO: Add tests

Test setup:

  • Install the OpenShift Pipeline Operator
  • Add pipelines (could install the ones from frontend/packages/dev-console/yamls/pipelines/install_pipeline_mocks.sh or create / upload your own)
  • View the new visualization in Pipeline and Pipeline Runs (and in the Add flow)

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

/kind feature
cc @jeff-phillips-18 @christianvogt

@openshift-ci-robot openshift-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 19, 2020
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serenamarie125 I took a causal stab at solving https://issues.redhat.com/browse/ODC-1860 when I stumbled upon the issue in my work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I likely can gut a lot of this too... I just haven't looked too much into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeff-phillips-18 This is the hack we discussed - we need to find a better way for me to get a notification when the layout finishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes for DagreLayout were rushed and not really thought through. @jeff-phillips-18 @christianvogt Feel free to comment on direction / changes. I'll likely conflict with your work Jeff when it comes to your topology work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reconcile once the changes are in, you can use the GRAPH_LAYOUT_END_EVENT event once its there.

@andrewballantyne andrewballantyne force-pushed the pipeline-topology-foundation branch 2 times, most recently from 725260d to c7e7a44 Compare January 19, 2020 03:00
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 19, 2020
@andrewballantyne andrewballantyne force-pushed the pipeline-topology-foundation branch from c7e7a44 to b745f71 Compare January 19, 2020 04:22
@serenamarie125
Copy link
Contributor

@andrewballantyne this is looking really good! will this PR also cover the tooltips associated with the PLR visualization? I'm assuming they shouldn't be affected, but you never know :)

Is it also worth including screenshots of what the PL vs PLR visualizations look like?

@andrewballantyne
Copy link
Contributor Author

@andrewballantyne this is looking really good! will this PR also cover the tooltips associated with the PLR visualization? I'm assuming they shouldn't be affected, but you never know :)

Is it also worth including screenshots of what the PL vs PLR visualizations look like?

Doh, of course, I'll update with it. I tested it under the hood but didn't post the screenshots 😦 Not sure what I was thinking haha.

@andrewballantyne
Copy link
Contributor Author

@serenamarie125 Apologizes for that; Pipeline Runs have been added to the description (from the Pipeline Run details page).

@andrewballantyne andrewballantyne force-pushed the pipeline-topology-foundation branch from b745f71 to 5f923f2 Compare January 20, 2020 10:55
@andrewballantyne
Copy link
Contributor Author

/test backend

@andrewballantyne
Copy link
Contributor Author

/retest

1 similar comment
@andrewballantyne
Copy link
Contributor Author

/retest

@spadgett
Copy link
Member

/hold
this didn't actually pass e2e tests

@openshift-ci-robot 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 20, 2020
@spadgett
Copy link
Member

/hold cancel
rerunning tests

@openshift-ci-robot 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 20, 2020
@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console 5f923f2 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.

@andrewballantyne andrewballantyne mentioned this pull request Jan 23, 2020
5 tasks
endPoint.clone().translate(-targetNode.getBounds().width / 2, 0),
)}
stroke="var(--pf-global--BorderColor--light-100)"
fill="none"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather these styles be in a .scss file but ok.

@jeff-phillips-18
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andrewballantyne, jeff-phillips-18
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:

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

@andrewballantyne
Copy link
Contributor Author

Folded into #4055 for CI sanity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/dev-console Related to dev-console kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants