Let users paste certificate values #1068

spadgett · 2016-12-22T14:41:16Z

When creating or editing a route, let users paste in the certificate values if they need to. Previously you could only upload from a file.

Fixes #964

When creating or editing a route, let users paste in the certificate values if they need to. Previously you could only upload from a file.

benjaminapetersen · 2016-12-22T16:43:15Z

Working through this, a few thoughts, but probably not to hold up your PR.

I skimmed right past the "secure routes" checkbox a few times expecting to see a "Security" heading at the same level as "Labels" below. Perhaps it would help draw the eye?
I'm curious if we should hide the boxes that are not relevant to a selection in the TLS Termination box. For example, if "Edge" doesn't need "Destination CA Certificate", do we need to show it?
I was surprised to see 4 sets, expected one option with just 2. Clearly I need to read up more on secure routes. 😄

spadgett · 2016-12-22T16:53:53Z

I skimmed right past the "secure routes" checkbox a few times expecting to see a "Security" heading at the same level as "Labels" below. Perhaps it would help draw the eye?

Agree on this, although we'd have to decide how to make it look on the add to project page, which uses those heading for major sections. Maybe a smaller header?

I'm curious if we should hide the boxes that are not relevant to a selection in the TLS Termination box. For example, if "Edge" doesn't need "Destination CA Certificate", do we need to show it?

Yeah, maybe not. Trying to decide if we should show it and keep the existing warning if a value will be removed.

Maybe a stronger warning there saying the value will be removed from the route.

spadgett · 2016-12-22T17:54:24Z

@benjaminapetersen I'm glad you made these comments because it got me testing. There's some stuff broken with routes (although they're fine in our 1.4 release). Working on fixes.

benjaminapetersen · 2016-12-22T18:30:42Z

Ah, that totally makes sense if it has a value already but the type is changed so the value is no longer needed.

spadgett · 2016-12-22T18:31:18Z

@benjaminapetersen I have showing and hiding the fields working, but it feels strange. The different termination types all support different certs. It's a bit disconcerting to see things appear and disappear when changing types. Since the certs are the last thing on the page, it can cause the page to jump/scroll when removed. Thinking we should table that.

benjaminapetersen · 2016-12-22T18:34:35Z

Yeah, that works for me. Figured it was a note/discussion, but prob not holding up the PR if it makes things quirky.

benjaminapetersen

On that note, LGTM!

benjaminapetersen · 2016-12-22T18:36:32Z

Oh wait, just saw your comment about broken stuff with routes. Not a concern now, but will be?

spadgett · 2016-12-22T18:37:40Z

Oh wait, just saw your comment about broken stuff with routes. Not a concern now, but will be?

About to push an update. There's an error creating edge routes currently. Also includes some improvement to the form (section headers).

spadgett · 2016-12-22T18:48:06Z

@benjaminapetersen updated, PTAL at the second commit

* Make "Split traffic across multiple services" a checkbox like "Secure route" * Add consistent section headings for "Alernate Services" and "Security" * Fix bug where `insecureEdgeTerminationPolicy` was initialized to an invalid value, causing an error creating edge terminated routes * Fix a bug where the range slider was not properly initialized for existing values

benjaminapetersen · 2016-12-22T19:42:37Z

Checking it now

benjaminapetersen · 2016-12-22T19:58:08Z

Couple observations:

I definitely like the headings for "Alternate Services" and "Security".
Checking "split traffic" is a bit jumpy. Couple things I struggled with here:
- The slider works well, but once I click "edit weights as integers" I couldn't go back to the slider without refreshing the page.
- When I switch to "edit weights as integers" it took me a while to figure out why I had a dropdown with only one service, even though the slider shows 2 services:
  
  I think its because the first service shows up above and the "weight" input now appears next to it. I had to think about that a bit as I expected everything I was interacting with to appear below the heading. I wonder if we could leave the above selector to be exclusive to "service to route to" and just list them all again under "split traffic" with the weight inputs (somehow show the primary service linked to the above "service to route to").
- Once I added additional services beyond 2, it was a little easier to understand.

spadgett · 2016-12-22T20:01:13Z

Yeah I don't disagree on the a/b services usability. I think we should revisit that (although probably a follow on at this point). We changed it to use the slider very recently.

spadgett · 2016-12-22T20:01:54Z

Related PR #1053

benjaminapetersen · 2016-12-22T20:02:02Z

Was just typing the same thing--I don't think that these observations relate to "paste cert value" so can def pull that out. I'll create an issue.

benjaminapetersen

Only one nit in the code besides our discussion about the experience. Good to go!

benjaminapetersen · 2016-12-22T20:13:47Z

app/scripts/directives/oscFileInput.js

@@ -24,7 +23,7 @@ angular.module('openshiftConsole')
        var dropMessageSelector = "#" + scope.dropMessageID,
            highlightDropZone = false,
            showDropZone = false,
-            inputFileField = element.find('input[type=file]')[0];
+            inputFileField = element.find('input[type=file]');


nit: we could do inputFileField = element.find('input[type=file]')[0]; or inputFileField = _.first(element.find()) here & not have to repeat the [0] a few places below.

Unfortunately I can't because I need the jQuery object in one place :/

Ah, true, because of the .change on 73, no worries!

spadgett · 2016-12-22T20:35:59Z

thanks @benjaminapetersen !

[merge]

openshift-bot · 2016-12-22T20:39:05Z

Evaluated for origin web console merge up to 7816a26

openshift-bot · 2016-12-22T20:39:15Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/891/) (Base Commit: ab61c6f)

Let users paste certificate values

ddb780e

When creating or editing a route, let users paste in the certificate values if they need to. Previously you could only upload from a file.

spadgett requested a review from benjaminapetersen December 22, 2016 14:41

benjaminapetersen approved these changes Dec 22, 2016

View reviewed changes

spadgett force-pushed the paste-certs branch from 00736c9 to b139986 Compare December 22, 2016 18:47

spadgett force-pushed the paste-certs branch from b139986 to 7816a26 Compare December 22, 2016 19:10

benjaminapetersen mentioned this pull request Dec 22, 2016

Splitting traffic under routes is a bit difficult to understand #1070

Closed

benjaminapetersen approved these changes Dec 22, 2016

View reviewed changes

openshift-bot merged commit 6bd8443 into openshift:master Dec 22, 2016

spadgett deleted the paste-certs branch December 22, 2016 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let users paste certificate values #1068

Let users paste certificate values #1068

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen left a comment

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen left a comment

benjaminapetersen Dec 22, 2016

spadgett Dec 22, 2016

benjaminapetersen Dec 22, 2016

spadgett commented Dec 22, 2016

openshift-bot commented Dec 22, 2016

openshift-bot commented Dec 22, 2016 •

edited

Loading

Let users paste certificate values #1068

Let users paste certificate values #1068

Conversation

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen left a comment

Choose a reason for hiding this comment

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

spadgett commented Dec 22, 2016

spadgett commented Dec 22, 2016

benjaminapetersen commented Dec 22, 2016

benjaminapetersen left a comment

Choose a reason for hiding this comment

benjaminapetersen Dec 22, 2016

Choose a reason for hiding this comment

spadgett Dec 22, 2016

Choose a reason for hiding this comment

benjaminapetersen Dec 22, 2016

Choose a reason for hiding this comment

spadgett commented Dec 22, 2016

openshift-bot commented Dec 22, 2016

openshift-bot commented Dec 22, 2016 • edited Loading

openshift-bot commented Dec 22, 2016 •

edited

Loading