Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add additional route settings to UI #5997

Merged
merged 1 commit into from
Dec 2, 2015

Conversation

spadgett
Copy link
Member

https://trello.com/c/WSVk6KQk

  • Adds support for creating secure routs during the create flow.
  • Adds support creating routes for existing services.

Fixes #6093

TODO:

Routing section during create flow:

openshift_web_console

After clicking "Show options for secure routes":

openshift_web_console

Fields are enabled and disabled depending on termination type. (Selecting Passthrough disables the Path field, etc.).

Add route from overview:

openshift_web_console

Add route from service page:

openshift_web_console

Create route from routes page:

openshift_web_console

Standalone add route page:

openshift_web_console

/cc @jwforres @jcantrill

@spadgett
Copy link
Member Author

I'm building on the work already done by @jcantrill for file upload / certificates.

@spadgett
Copy link
Member Author

I could switch to bootstrap-styled selects.

openshift_web_console

@jwforres
Copy link
Member

I definitely like consistently having the fields below the label, it feels a little weird with the selects to the right and all the other inputs beneath.

I do like select dropdowns that have the same look across all browsers. How's the accessbility on the bootstrap ones? Are they still actually tags?

@jwforres
Copy link
Member

@gruiz17 FYI

@pweil- anything missing?

@spadgett
Copy link
Member Author

@jwforres It's a real select, although it doesn't actually look consistent across browsers... Not a fan of how it looks in Firefox.

http://getbootstrap.com/css/#selects

@spadgett
Copy link
Member Author

I guess it's not too bad in Firefox.

openshift_web_console

@spadgett spadgett force-pushed the routing branch 3 times, most recently from 9aac434 to dc72b64 Compare November 23, 2015 14:38
@jwforres
Copy link
Member

Hmm if its not consistently styled, is there any benefit we are getting
from it?

On Mon, Nov 23, 2015 at 8:29 AM, Sam Padgett notifications@github.com
wrote:

I guess it's not too bad in Firefox.

[image: openshift_web_console]
https://cloud.githubusercontent.com/assets/1167259/11337615/4168687c-91bc-11e5-9d59-a3faa0e07362.png


Reply to this email directly or view it on GitHub
#5997 (comment).

@spadgett
Copy link
Member Author

It's similarly sized to the other inputs and looks OK positioned under the label. I think it makes the form look more consistent.

@spadgett
Copy link
Member Author

I feel like our default should be to use the bootstrap/patternfly classes unless there's a good reason not to.

@jwforres
Copy link
Member

So just following up here on the discussion we had in person. So we will go with the bootstrap style for the selects. We will add the ability to add routes from 1) a service page (service will be pre-set in the route's to field), 2) from the overview if the service does not have a route already, and 3) from the Routes page (we'll give you a list of services to route to, should we make this a typeahead list?)

What we will not attempt to do now is routes that don't route to a service.

@spadgett
Copy link
Member Author

Added some support for creating routes outside of the create flow.

openshift_web_console

openshift_web_console

@spadgett
Copy link
Member Author

Add route link on overview:

openshift_web_console

@spadgett spadgett force-pushed the routing branch 7 times, most recently from 00bfeee to 5caae01 Compare November 24, 2015 15:08
@spadgett
Copy link
Member Author

@Kargakis FYI

@spadgett spadgett force-pushed the routing branch 2 times, most recently from 20df6aa to 19d1ddc Compare November 24, 2015 17:03
<!-- Use shorter Termination title for table-mobile to avoid overlapping text. -->
<td data-title="Termination">
{{route.spec.tls.termination}}
<span ng-if="!route.spce.tls.termination">&nbsp;</span>
Copy link
Member

Choose a reason for hiding this comment

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

typo on spec

@jwforres
Copy link
Member

jwforres commented Dec 1, 2015

One other thought, in the ports dropdown, should we be showing the name in addition to the port number when its a named port? http (8080) or 8080 (http)

@spadgett
Copy link
Member Author

spadgett commented Dec 1, 2015

One other thought, in the ports dropdown, should we be showing the name in addition to the port number when its a named port? http (8080) or 8080 (http)

Hm, I think it will be used since that is the only value we have for targetPort. If the service points to a named port, we don't even know the number.

@jwforres
Copy link
Member

jwforres commented Dec 1, 2015

ah yep, was thinking of how the port stuff is structured on the pod template

On Tue, Dec 1, 2015 at 3:45 PM, Sam Padgett notifications@github.com
wrote:

One other thought, in the ports dropdown, should we be showing the name in
addition to the port number when its a named port? http (8080) or 8080
(http)

Hm, I think it will be used since that is the only value we have for
targetPort. If the service points to a named port, we don't even know the
number.


Reply to this email directly or view it on GitHub
#5997 (comment).

@jwforres
Copy link
Member

jwforres commented Dec 1, 2015

LGTM, can merge after the rebase against ben's stuff

@pweil-
Copy link
Contributor

pweil- commented Dec 1, 2015

If the service points to a named port, we don't even know the number.

Since @Kargakis just went through an issue with expose when dealing with ports I want to make sure we don't run into the same thing here. To be clear, the port on the route is matching to a port in the endpoints - not the service. The router bypasses the service completely and routes straight to the endpoints.

Couldn't really tell where it was pulling the ports to choose from so I thought I'd mention it just in case. @Kargakis has an upstream PR that is extracting the utility he proposed to use in this origin pr https://github.com/openshift/origin/pull/5578/files#diff-ec75dfa091437a7b426570ffde9d4cb4

@spadgett
Copy link
Member Author

spadgett commented Dec 1, 2015

Thanks @pweil-. We're using the endpoint port.

@pweil-
Copy link
Contributor

pweil- commented Dec 1, 2015

Thanks @pweil-. We're using the endpoint port.

👍 👍 And also, the screenshots look great!

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2015
@spadgett
Copy link
Member Author

spadgett commented Dec 2, 2015

@jwforres Rebased. No real changes except picking up the ProjectsService rename and resolving some minor conflicts.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 318b0b5

@jwforres
Copy link
Member

jwforres commented Dec 2, 2015

[merge]

@jwforres jwforres added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2015
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4219/) (Image: devenv-rhel7_2858)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 318b0b5

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2015
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7592/)

@jwforres
Copy link
Member

jwforres commented Dec 2, 2015

[merge]

openshift-bot pushed a commit that referenced this pull request Dec 2, 2015
@openshift-bot openshift-bot merged commit 663b358 into openshift:master Dec 2, 2015
@kurktchiev
Copy link

So i know this got merged in, but I will leave my 2c here as I did last night at MeetUp at the Tower. In just about all my work flows I find myself needing to do multiple routes (say http/https) at the same time. It would be nice if you could, queue these up and add on another route and have all of the ones created in one swoop rather than having to go in and out of the creation process multiple times

@jwforres
Copy link
Member

jwforres commented Dec 2, 2015

In your workflow are you creating routes primarily during the "Add to
Project" create flow, or after the fact when you want to expose an existing
service

On Wed, Dec 2, 2015 at 2:24 PM, Boris Kurktchiev notifications@github.com
wrote:

So i know this got merged in, but I will leave my 2c here as I did last
night at MeetUp at the Tower. In just about all my work flows I find myself
needing to do multiple routes (say http/https) at the same time. It would
be nice if you could, queue these up and add on another route and have all
of the ones created in one swoop rather than having to go in and out of the
creation process multiple times


Reply to this email directly or view it on GitHub
#5997 (comment).

@spadgett
Copy link
Member Author

spadgett commented Dec 2, 2015

@ebalsumgo Thanks for your feedback. For the http/https case specifically, is there a reason you don't create one edge route that allows insecure traffic?

@kurktchiev
Copy link

@jwforres @spadgett it wasn't working in 3.0.2 (http/https) so thats why I got in the habit of creating multiple routes I suppose, but yes the secure/nonsecure for sure should be fine (though based on the mockup I saw last night, I would say it needs to be clearer what enabling EDGE termination actually accomplishes for you). for @jwforres specific question, I would love to have it exposed during the Add to Project phase as it would make it the easiest/simplest for my user base (read non tech savvy people).

@jwforres
Copy link
Member

jwforres commented Dec 2, 2015

Ok i opened an issue, we can continue discussion there #6174

@spadgett spadgett deleted the routing branch December 4, 2015 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/web lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants