Add Topology Edge Selection w/ Side Panel #4003

jeff-phillips-18 · 2020-01-18T21:07:59Z

Addresses story: https://issues.redhat.com/browse/ODC-2354
Fixes https://issues.redhat.com/browse/ODC-2797
Fixes https://issues.redhat.com/browse/ODC-2726

/cc @openshift/team-devconsole-ux

jeff-phillips-18 · 2020-01-20T12:49:10Z

/kind feature

serenamarie125

@jeff-phillips-18 this looks great, you don't have the Actual traffic connector shown here, but I assume that's coming in a follow up PR?

jeff-phillips-18 · 2020-01-21T20:05:53Z

@jeff-phillips-18 this looks great, you don't have the Actual traffic connector shown here, but I assume that's coming in a follow up PR?

Correct, we do not have those connectors in yet. see #4026

christianvogt · 2020-01-22T20:46:01Z

Used the Move action on a the connects-to connector and got many warnings / errors logged in the console. The connection did move in topology though.

The source node is showing up in the target dropdown of the Move action. This implies i'm going to reconnect the to the source node. This isn't allow. We should remove the source as an option in the menu.

I don't think we should disable the current target node in the Move action target dropdown. Simply perform no action if the user keeps the current target selected.

~~Unable to create a service binding request anymore. The tooltip shows it wants to create an SBR but on release, I get an error in in the console and a visual connector is created instead.~~ Found issue on master as well.

jeff-phillips-18 · 2020-01-22T21:57:19Z

Updated as suggested. Not seeing errors any more. Still see some warnings due to patternfly/patternfly-react#3549

nemesis09

@jeff-phillips-18
The move action on a visual connector allows event source as a valid dropdown option as target for a deployment. and throws an error on submit (Move)

nemesis09 · 2020-01-22T22:12:55Z

The move action for a visual connector allows database as valid dropdown option as target and throws an error on submit (move). It updates the connection to database on topology though and changes the connector to type binding

jeff-phillips-18 · 2020-01-22T22:22:55Z

@nemesis09 Did you try that after my last push?

nemesis09 · 2020-01-22T22:50:48Z

@nemesis09 Did you try that after my last push?

Sorry, I didn't try it after the latest push when I got the errors.
But now that I have pulled in the latest push-

the form shows an error message on submit for event source and database as target, but the error message doesn't describe the error and rather shows error is undefined
although the form shows error message for database as target on submit, the connector on topology is updated to connect to database as binding connector
the Move button is disabled after submitting with database or event source, but it doesn't get enabled again on selecting other valid options from the dropdown. We need to launch it using the Move action again.

jeff-phillips-18 · 2020-01-22T23:18:46Z

Fixed modal to show the correct error.

Errors updated the edges are reported already in https://issues.redhat.com/browse/ODC-2726 which is fixed in #3986

nemesis09 · 2020-01-23T14:00:54Z

The modal doesn't show any error now, and submitting(Move) with Event Source as a target, just turns out to be a no op

nemesis09 · 2020-01-23T14:06:59Z

I tried locally with changes pulled from #3986,
this solves the errors for moving connection from a deployment as visual connector to a database as a binding connector

jeff-phillips-18 · 2020-01-23T15:09:26Z

Removed event sources from the list of available targets..

nemesis09 · 2020-01-23T16:17:54Z

The modal disables the submit(move) button on error, but doesn't enable it back if a different target is selected from the dropdown.

serenamarie125

looks good!

nemesis09 · 2020-01-24T02:40:50Z

tested locally. works as expected.
/lgtm

andrewballantyne

The type issues are minor since they are not exposed methods yet... We definitely need tests for the utilities so when that is being done we can add the types as likely we'll want to export those functions for tests.

@jeff-phillips-18 please log a Task, assign it to yourself, and put it in the next sprint (179), next week we'll need to write the missing tests. Also please fix the types at the same time.

andrewballantyne · 2020-01-24T13:09:44Z

frontend/packages/dev-console/src/components/topology/actions/edgeActions.ts

+  };
+};
+
+const deleteConnection = (edge: BaseEdge) => {


Suggested change

const deleteConnection = (edge: BaseEdge) => {

const deleteConnection = (edge: BaseEdge): KebabOption => {

We need to make sure we type output of newly created objects to insure they stay properly typed (if anything in the type changes later).

andrewballantyne · 2020-01-24T13:11:07Z

frontend/packages/dev-console/src/components/topology/actions/edgeActions.ts

+} from '../const';
+import { moveConnectionModal } from '../components/MoveConnectionModal';
+
+const moveConnection = (edge: BaseEdge, availableTargets: Node[]) => {


Same here...

Suggested change

const moveConnection = (edge: BaseEdge, availableTargets: Node[]) => {

const moveConnection = (edge: BaseEdge, availableTargets: Node[]): KebabOption => {

andrewballantyne · 2020-01-24T13:12:07Z

frontend/packages/dev-console/src/components/topology/actions/edgeActions.ts

+      actions.push(deleteConnection(edge));
+      break;
+    default:
+      break;


Suggested change

break;

Default clauses almost never need a break unless you're doing conditional paths within it.

andrewballantyne · 2020-01-24T13:14:48Z

frontend/packages/dev-console/src/components/topology/components/MoveConnectionModal.tsx

+
+const ConnectedMoveConnectionModal = connect(mapStateToProps)(MoveConnectionModal);
+
+export const moveConnectionModal = createModalLauncher((props: MoveConnectionModalProps) => (


😕 There are a lot of different ways to make modals in our codebase. I'll need to take an action up to talk to Christian and find out what the best way is and try to centralize around it.

openshift-ci-robot · 2020-01-24T13:18:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, jeff-phillips-18, nemesis09, serenamarie125

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]

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-24T13:18:39Z

/retest

openshift-bot · 2020-01-24T13:29:39Z

/retest

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

jeff-phillips-18 · 2020-01-24T14:24:47Z

Added task https://issues.redhat.com/browse/ODC-2823

openshift-bot · 2020-01-24T14:47:31Z

/retest

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

openshift-bot · 2020-01-24T15:13:47Z

/retest

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

andrewballantyne · 2020-01-24T16:18:13Z

/retest

Looks like a flake...

Kubernetes resource CRUD operations Pod : displays a YAML editor for creating a new resource instance

openshift-bot · 2020-01-24T16:47:21Z

/retest

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

openshift-bot · 2020-01-24T17:11:58Z

/retest

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

jeff-phillips-18 · 2020-01-24T18:11:40Z

/retest

spadgett · 2020-01-24T20:47:05Z

/hold
for merge queue fix #4065

spadgett · 2020-01-25T04:24:16Z

/hold cancel
/retest

openshift-bot · 2020-01-25T04:26:14Z

/retest

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 18, 2020

openshift-ci-robot requested review from nemesis09 and sahil143 January 18, 2020 21:08

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

jeff-phillips-18 force-pushed the edge-select branch from 55bfa5b to 538c22b Compare January 20, 2020 11:58

jeff-phillips-18 changed the title ~~[WIP] Add Topology Edge Selection w/ Side Panel~~ Add Topology Edge Selection w/ Side Panel Jan 20, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2020

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

jeff-phillips-18 force-pushed the edge-select branch from 538c22b to 4751826 Compare January 21, 2020 12:03

serenamarie125 reviewed Jan 21, 2020

View reviewed changes

divyanshiGupta mentioned this pull request Jan 21, 2020

Add kiali URL in traffic connector side panel #4026

Merged

Add Topology Edge Selection w/ Side Panel

86845fc

jeff-phillips-18 force-pushed the edge-select branch from 4751826 to 9a8ba97 Compare January 22, 2020 21:56

nemesis09 reviewed Jan 22, 2020

View reviewed changes

Updates per review comments

2797f70

jeff-phillips-18 force-pushed the edge-select branch from 9a8ba97 to 2797f70 Compare January 22, 2020 23:17

Fix error creating connectors in Topology

9df16b1

jeff-phillips-18 mentioned this pull request Jan 23, 2020

Fix error creating connectors in Topology #3986

Closed

Remove event sources from target nodes list.

438a24f

Allow retry after errors

a754420

serenamarie125 approved these changes Jan 23, 2020

View reviewed changes

openshift-ci-robot assigned nemesis09 Jan 24, 2020

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

andrewballantyne approved these changes Jan 24, 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 Jan 24, 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 24, 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-merge-robot merged commit 9a70e79 into openshift:master Jan 25, 2020

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

jeff-phillips-18 deleted the edge-select branch December 2, 2020 13:37

	const deleteConnection = (edge: BaseEdge) => {
	const deleteConnection = (edge: BaseEdge): KebabOption => {

	const moveConnection = (edge: BaseEdge, availableTargets: Node[]) => {
	const moveConnection = (edge: BaseEdge, availableTargets: Node[]): KebabOption => {


		const ConnectedMoveConnectionModal = connect(mapStateToProps)(MoveConnectionModal);

		export const moveConnectionModal = createModalLauncher((props: MoveConnectionModalProps) => (

Add Topology Edge Selection w/ Side Panel #4003

Add Topology Edge Selection w/ Side Panel #4003

Uh oh!

Conversation

jeff-phillips-18 commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeff-phillips-18 commented Jan 20, 2020

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 21, 2020

Uh oh!

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

Uh oh!

Uh oh!

jeff-phillips-18 commented Jan 22, 2020

Uh oh!

nemesis09 left a comment • 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!

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

Uh oh!

Uh oh!

jeff-phillips-18 commented Jan 22, 2020

Uh oh!

nemesis09 commented Jan 22, 2020

Uh oh!

jeff-phillips-18 commented Jan 22, 2020

Uh oh!

nemesis09 commented Jan 23, 2020

Uh oh!

nemesis09 commented Jan 23, 2020

Uh oh!

jeff-phillips-18 commented Jan 23, 2020

Uh oh!

nemesis09 commented Jan 23, 2020

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

nemesis09 commented Jan 24, 2020

Uh oh!

andrewballantyne left a comment

Choose a reason for hiding this comment

Uh oh!

andrewballantyne Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

andrewballantyne Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

andrewballantyne Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

andrewballantyne Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jan 24, 2020

Uh oh!

andrewballantyne commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

jeff-phillips-18 commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

andrewballantyne commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

openshift-bot commented Jan 24, 2020

Uh oh!

jeff-phillips-18 commented Jan 24, 2020

Uh oh!

jeff-phillips-18 commented Jan 18, 2020 •

edited

Loading

christianvogt commented Jan 22, 2020 •

edited

Loading

nemesis09 left a comment •

edited

Loading

nemesis09 commented Jan 22, 2020 •

edited

Loading