Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow #6719

rottencandy · 2020-09-23T10:09:46Z

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

Analysis / Root cause:
The dropdown only allows for the selection of the exposed ports from the builder image.

Solution Description:
The user should have the ability to specify their target port to ensure the route works.
Relevant Slack thread: https://coreos.slack.com/archives/CHC2R6AGG/p1600757882002000

Screen shots / Gifs for design review:

cc: @openshift/team-devconsole-ux

Unit test coverage report:
Unchanged.

Test setup:

Navigate to +Add -> From git
Fill form, select Routing in advanced options
Notice the port input field provides typeahead for default ports, and also allows adding a custom port.

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-09-23T10:09:51Z

@rottencandy: This pull request references Bugzilla bug 1881881, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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.

In response to this:

Bug 1881881: Replace route dropdown input with typeahead select menu in Git flow

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.

rottencandy · 2020-09-23T11:48:59Z

/kind bug

rottencandy · 2020-09-28T17:21:02Z

/cc @christianvogt

openshift-ci-robot · 2020-09-28T17:22:19Z

@rottencandy: This pull request references Bugzilla bug 1881881, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.7.0" 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.

In response to this:

Bug 1881881: Replace route dropdown input with typeahead select menu in Git flow

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.

jerolimov · 2020-09-28T21:46:03Z

frontend/packages/dev-console/src/components/import/route/CreateRoute.tsx

-    );
-    return acc;
-  }, {});
+  const portOptions = ports.map((port) => port.containerPort);


When you convert the ports here to strings the Select automatically matches the user input and do not show "Create "8080"" if the option is already available. Wdyt?

Suggested change

const portOptions = ports.map((port) => port.containerPort);

const portOptions: string[] = ports.map((port) => port.containerPort.toString());

Updated. Not sure if there is something we can do about the "Create ..." option, even the Patternfly demos behave the same way: https://www.patternfly.org/v4/documentation/react/components/select#typeahead

Okay, thanks for revalidating this. I though that the .toString() solves this issue.

jerolimov · 2020-09-28T21:59:36Z

frontend/packages/dev-console/src/components/import/route/CreateRoute.tsx


 const CreateRoute: React.FC = () => {
  const {
    values: {
      image: { ports },
-      route: { defaultUnknownPort, targetPort },
+      route: { defaultUnknownPort },
    },
  } = useFormikContext<FormikValues>();


Suggested change

} = useFormikContext<FormikValues>();

} = useFormikContext<DeployImageFormData | GitImportFormData>();

Adding this types so that the ports and defaultUnknownPort vars are typed.

Updated, didn't know we could do that. Now I'm wondering why we even use FormikValues type :P.

jerolimov · 2020-09-28T22:06:32Z

@rottencandy Technical this works fine, I have added two comments to the code. I also want ask you if you want rename your new component from import/route/RouteInputField to import/route/PortInputField. For me this matches more the special case it handles here.

In this case a port validation 1-65535 would be also a good idea. It's not required, but I missed this when entering some invalid numbers.

Wdyt?

rottencandy · 2020-09-29T07:42:27Z

@rottencandy Technical this works fine, I have added two comments to the code. I also want ask you if you want rename your new component from import/route/RouteInputField to import/route/PortInputField. For me this matches more the special case it handles here.

Thanks for the review @jerolimov , Renamed to PortInputField.

In this case a port validation 1-65535 would be also a good idea. It's not required, but I missed this when entering some invalid numbers.

Currently we do the validation using a regex(/^\d+$/), I wasn't able to find a way to check for a maximum number using regex.

jerolimov · 2020-09-29T08:44:50Z

Currently we do the validation using a regex(/^\d+$/)

We could use a number check similar to this instead of regex for the unknownTargetPort validation scheme:

const portValidationSchema = yup
  .number()
  .typeError('Port must be an Integer.')
  .integer('Port must be an Integer.')
  .min(1, 'Port must be between 1 and 65535.')
  .max(65535, 'Port must be between 1 and 65535.');

Didn't know the typeError check before. It was the message if you enter "asd". Integer ensures that user could not enter "3.1415". Wdyt? Is it worth to update the PR again? :)

rottencandy · 2020-09-29T09:17:07Z

We could use a number check similar to this instead of regex for the unknownTargetPort validation scheme:
const portValidationSchema = yup
  .number()
  .typeError('Port must be an Integer.')
  .integer('Port must be an Integer.')
  .min(1, 'Port must be between 1 and 65535.')
  .max(65535, 'Port must be between 1 and 65535.');
Didn't know the typeError check before. It was the message if you enter "asd". Integer ensures that user could not enter "3.1415". Wdyt? Is it worth to update the PR again? :)

Updated.

jerolimov · 2020-09-29T21:38:11Z

/retest

rottencandy · 2020-09-30T17:04:12Z

Updated to behave the same way when selecting Knative Service.

jerolimov · 2020-09-30T18:00:48Z

Hmm, I'm really sorry but could not verify this issue. I tried to set a custom port for the route and select "Knative Service" as Deployment. The created Route has not set the port anywhere.

Then I tried this on master, and changed ServerlessRouteSection.tsx so that the <InputField name="route.unknownTargetPort" ... was always used. And the Route does not contain the entered port in this case as well. Only when selecting a port from the Dropdown (on master) the value was set in the Route correctly. So maybe this is already an issue before? @invincibleJai what do you think?

@rottencandy Unfortunately, I'm now thinking about the unifying between two different cases. Before we have <InputField name="route.unknownTargetPort" ... OR <DropdownField name="route.targetPort" .... But with this PR we always use the unknownTargetPort. If we have no other reason to keep targetPort which I maybe oversee at the moment, we should maybe simplify the complete createRoute algorithm and should always use only one variable / form field here? 🤔

invincibleJai · 2020-10-01T04:44:28Z

/assign

rottencandy · 2020-10-01T07:31:05Z

@jerolimov The knative service function prioritized targetPort over unknownTargetPort, which is why it was never using the entered port, I've updated it to use unknownTargetPort when available.

AFAIK, targetPort is mainly being used in two places:

In the edit flow, when user does not intend to change port, targetPort will be set as the existing port value(in full format eg.8080-tcp).
In the image stream search fields, both internal & external fields set targetPort to the first of available ports, not sure why it is being set here.

christianvogt · 2020-10-01T18:34:53Z

One more small issue:

import from git https://github.com/nodeshift-starters/react-web-ap and set port to 3000
from topology right click the deployment and select edit <mydeployment>
open the routing advanced section
click the x in the target port text field
observe the placeholder shows 8080
complete the form
if you navigate to the route associated with this deployment, the port remains as 3000-tcp

rottencandy · 2020-10-05T12:15:40Z

One more small issue:

* import from git `https://github.com/nodeshift-starters/react-web-ap` and set port to 3000

* from topology right click the deployment and select `edit <mydeployment>`

* open the routing advanced section

* click the `x` in the target port text field

* observe the placeholder shows `8080`

* complete the form

* if you navigate to the route associated with this deployment, the port remains as `3000-tcp`

Fixed.

rottencandy · 2020-10-05T12:36:30Z

/bugzilla refresh

openshift-ci-robot · 2020-10-05T12:36:41Z

@rottencandy: This pull request references Bugzilla bug 1881881, 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.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

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.

rottencandy · 2020-10-05T14:47:42Z

/retest

rottencandy · 2020-10-06T07:55:40Z

/retest

rottencandy · 2020-10-07T07:06:56Z

/retest

debsmita1 · 2020-10-14T09:45:26Z

One more small issue:

* import from git `https://github.com/nodeshift-starters/react-web-ap` and set port to 3000

* from topology right click the deployment and select `edit <mydeployment>`

* open the routing advanced section

* click the `x` in the target port text field

* observe the placeholder shows `8080`

* complete the form

* if you navigate to the route associated with this deployment, the port remains as `3000-tcp`

Fixed.

@rottencandy I could reproduce the above issue for knative service workload. Have a look at the gif below:

rottencandy · 2020-10-15T12:39:54Z

Updated edit flow to use defaultUnknownPort when port is input is cleared.

debsmita1 · 2020-10-20T07:43:10Z

verified this locally
/lgtm

jerolimov · 2020-10-21T16:13:03Z

/retest

christianvogt · 2020-10-21T21:02:01Z

/approve

openshift-ci-robot · 2020-10-21T21:02:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgliwa01, christianvogt, debsmita1, rottencandy, 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~~ [christianvogt]
~~frontend/packages/knative-plugin/OWNERS~~ [christianvogt]

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-10-22T02:04:22Z

@rottencandy: All pull requests linked via external trackers have merged:

openshift/console#6719

Bugzilla bug 1881881 has been moved to the MODIFIED state.

In response to this:

Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow

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.

rottencandy · 2020-11-02T07:54:23Z

/cherrypick release-4.6

openshift-cherrypick-robot · 2020-11-02T07:54:32Z

@rottencandy: new pull request created: #7064

In response to this:

/cherrypick release-4.6

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 bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 23, 2020

openshift-ci-robot requested review from debsmita1 and karthikjeeyar September 23, 2020 10:10

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

openshift-ci-robot requested a review from christianvogt September 28, 2020 17:21

jerolimov reviewed Sep 28, 2020

View reviewed changes

rottencandy force-pushed the routeinput branch from 755b1be to 43aec9f Compare September 29, 2020 07:36

rottencandy force-pushed the routeinput branch from 43aec9f to a41fb6a Compare September 29, 2020 07:44

rottencandy force-pushed the routeinput branch from a41fb6a to 984bd82 Compare September 29, 2020 09:16

rottencandy force-pushed the routeinput branch from 984bd82 to 9c1e4e3 Compare September 29, 2020 13:08

rottencandy force-pushed the routeinput branch from 9c1e4e3 to 44bfac9 Compare September 30, 2020 17:03

rottencandy changed the title ~~Bug 1881881: Replace route dropdown input with typeahead select menu in Git flow~~ Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow Sep 30, 2020

openshift-ci-robot assigned invincibleJai Oct 1, 2020

rottencandy force-pushed the routeinput branch from 44bfac9 to 175816c Compare October 1, 2020 07:20

openshift-ci-robot added the component/knative Related to knative-plugin label Oct 1, 2020

rottencandy force-pushed the routeinput branch from 710a489 to c37779d Compare October 5, 2020 12:14

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Oct 5, 2020

rottencandy force-pushed the routeinput branch from c37779d to 19d7814 Compare October 5, 2020 13:11

rottencandy force-pushed the routeinput branch from 19d7814 to 6cb02d2 Compare October 15, 2020 12:38

replace route input with typeahead select menu

d3c5c46

rottencandy force-pushed the routeinput branch from 6cb02d2 to d3c5c46 Compare October 15, 2020 13:14

openshift-ci-robot assigned debsmita1 Oct 20, 2020

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

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

openshift-merge-robot merged commit cf36763 into openshift:master Oct 22, 2020

spadgett added this to the v4.7 milestone Oct 26, 2020

openshift-cherrypick-robot mentioned this pull request Nov 2, 2020

[release-4.6] Bug 1893620: Replace route dropdown input with typeahead select menu in import/edit flow #7064

Merged

rottencandy deleted the routeinput branch December 7, 2020 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow #6719

Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow #6719

rottencandy commented Sep 23, 2020 •

edited

openshift-ci-robot commented Sep 23, 2020

rottencandy commented Sep 23, 2020

rottencandy commented Sep 28, 2020

openshift-ci-robot commented Sep 28, 2020

jerolimov Sep 28, 2020

rottencandy Sep 29, 2020

jerolimov Sep 29, 2020

jerolimov Sep 28, 2020

rottencandy Sep 29, 2020

jerolimov commented Sep 28, 2020

rottencandy commented Sep 29, 2020

jerolimov commented Sep 29, 2020

rottencandy commented Sep 29, 2020

jerolimov commented Sep 29, 2020

rottencandy commented Sep 30, 2020

jerolimov commented Sep 30, 2020 •

edited

invincibleJai commented Oct 1, 2020

rottencandy commented Oct 1, 2020 •

edited

christianvogt commented Oct 1, 2020

rottencandy commented Oct 5, 2020

rottencandy commented Oct 5, 2020

openshift-ci-robot commented Oct 5, 2020

rottencandy commented Oct 5, 2020

rottencandy commented Oct 6, 2020

rottencandy commented Oct 7, 2020

debsmita1 commented Oct 14, 2020

rottencandy commented Oct 15, 2020

debsmita1 commented Oct 20, 2020

jerolimov commented Oct 21, 2020

christianvogt commented Oct 21, 2020

openshift-ci-robot commented Oct 21, 2020

openshift-ci-robot commented Oct 22, 2020

rottencandy commented Nov 2, 2020

openshift-cherrypick-robot commented Nov 2, 2020

	const portOptions = ports.map((port) => port.containerPort);
	const portOptions: string[] = ports.map((port) => port.containerPort.toString());

	} = useFormikContext<FormikValues>();
	} = useFormikContext<DeployImageFormData \| GitImportFormData>();

Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow #6719

Bug 1881881: Replace route dropdown input with typeahead select menu in import/edit flow #6719

Conversation

rottencandy commented Sep 23, 2020 • edited

openshift-ci-robot commented Sep 23, 2020

rottencandy commented Sep 23, 2020

rottencandy commented Sep 28, 2020

openshift-ci-robot commented Sep 28, 2020

jerolimov Sep 28, 2020

Choose a reason for hiding this comment

rottencandy Sep 29, 2020

Choose a reason for hiding this comment

jerolimov Sep 29, 2020

Choose a reason for hiding this comment

jerolimov Sep 28, 2020

Choose a reason for hiding this comment

rottencandy Sep 29, 2020

Choose a reason for hiding this comment

jerolimov commented Sep 28, 2020

rottencandy commented Sep 29, 2020

jerolimov commented Sep 29, 2020

rottencandy commented Sep 29, 2020

jerolimov commented Sep 29, 2020

rottencandy commented Sep 30, 2020

jerolimov commented Sep 30, 2020 • edited

invincibleJai commented Oct 1, 2020

rottencandy commented Oct 1, 2020 • edited

christianvogt commented Oct 1, 2020

rottencandy commented Oct 5, 2020

rottencandy commented Oct 5, 2020

openshift-ci-robot commented Oct 5, 2020

rottencandy commented Oct 5, 2020

rottencandy commented Oct 6, 2020

rottencandy commented Oct 7, 2020

debsmita1 commented Oct 14, 2020

rottencandy commented Oct 15, 2020

debsmita1 commented Oct 20, 2020

jerolimov commented Oct 21, 2020

christianvogt commented Oct 21, 2020

openshift-ci-robot commented Oct 21, 2020

openshift-ci-robot commented Oct 22, 2020

rottencandy commented Nov 2, 2020

openshift-cherrypick-robot commented Nov 2, 2020

rottencandy commented Sep 23, 2020 •

edited

jerolimov commented Sep 30, 2020 •

edited

rottencandy commented Oct 1, 2020 •

edited