EnvFrom Issue Updates #2198

cdcabrera · 2017-10-02T14:12:05Z

EnvFrom issue fixes for CSS, copy, and UX. Updates parts of #2182

Drag & Drop for single entries
Copy
CSS highlighting on disabled options

spadgett · 2017-10-02T14:17:29Z

app/views/directives/edit-environment-variables.html

@@ -46,7 +46,7 @@ <h4 class="section-label">
      entries="container.envFrom"
      selector-placeholder="Secret/Config Map"
      env-from-selector-options="$ctrl.valueFromObjects"
-      add-row-link="Add ALL Values from a Resource"
+      add-row-link="Add ALL Variables from Secret or Config Map"


I think "Add ALL Values..." is more accurate since secrets and config maps don't have "variables."

@beanh66 Are you OK with that change?

@spadgett That works for me. So it would be "Add ALL Values from Secret or Config Map" correct? In that case we should also change the top link to match. Would the top links be: "Add Variable | Add Value from Secret or Config Map" ?

So it would be "Add ALL Values from Secret or Config Map" correct?

@beanh66 Yup! That's what I was thinking

spadgett · 2017-10-02T15:00:24Z

app/scripts/directives/editEnvironmentFrom.js

@@ -134,7 +134,11 @@
      return referenceValue;
    };

-    ctrl.checkEntries = function(option) {
+    ctrl.checkEntries = function(option, entrySelectedEnvFrom) {
+      if(_.isEqual(option, entrySelectedEnvFrom)) {


Does option === entrySelectedEnvFrom work or is it a copy?

So it might... but the data within "entrySelectedEnvFrom" is "moved" over so kind of a copy ... I had thought about placing a check for option.kind === entrySelectedEnvFrom.kind to avoid firing the _.isEqual every time

You could check the UIDs, something like (untested)

if (entrySelectedEnvFrom.metadata.uid === option.metadata.uid) { return false; }

assuming neither is ever null.

Secrets and config maps can contain megabytes of data, so would be good to avoid a deep comparison if possible.

That works, and understood on the object complexity... Went ahead and checked the === comparison, it does appear to function as expected, they evaluate as equal. Was a little unsure since one of the parameters is being pulled through a nested ng-repeat

benjaminapetersen · 2017-10-02T15:00:49Z

app/views/directives/edit-environment-variables.html

@@ -46,7 +46,7 @@ <h4 class="section-label">
      entries="container.envFrom"
      selector-placeholder="Secret/Config Map"
      env-from-selector-options="$ctrl.valueFromObjects"
-      add-row-link="Add ALL Values from a Resource"
+      add-row-link="Add ALL Values from Secret or Config Map"


+1 for clarity here.

spadgett · 2017-10-02T15:36:03Z

@beanh66 Opinion on these proposed changes? I feel like we're repeating ourselves with some of the labels and headings.

Before:

After:

EnvFrom issue fixes for CSS, copy, and UX

benjaminapetersen · 2017-10-02T15:42:52Z

Hmm. True, that does feel a touch overkill...

spadgett · 2017-10-02T16:11:04Z

I'm going to go ahead and merge this since it fixes several problems. Thanks @cdcabrera

@beanh66 @ncameronbritt I still wouldn't mind your thoughts on my screenshots just above, though: #2198 (comment)

[merge]

spadgett · 2017-10-02T16:48:17Z

Looks like a webdriver flake

[merge]

beanh66 · 2017-10-02T17:03:20Z

@cdcabrera @spadgett see my comments below:

It might be best to have the two dropdowns say "Select a resource" for simplicity.
I am fine with removing the top "Variables" label as it doesn't add much.
In terms of the links, whether you are adding a single or multiple values form a config map/secret, the language should be consistent. If you are changing the last link to be "Add ALL Values from Secret or Config Map" then I think the one above should also be "Add Value from Secret or Config Map." I'm fine with the word Value or Variable here but I do think they should match.
I also noticed the links say "Config Map or Secret" but in the actual dropdown the secrets are listed first so we should be consistent. Either change the order in the dropdown or change the link to "Secret or Config Map"

Thoughts?

spadgett · 2017-10-02T17:13:59Z

In terms of the links, whether you are adding a single or multiple values form a config map/secret, the language should be consistent.

Yeah, I see your point. I prefer "Add Value from Secret or Config Map"

It also has the nice property of matching the field you see (valueFrom) when editing YAML.

I also noticed the links say "Config Map or Secret" but in the actual dropdown the secrets are listed first so we should be consistent. Either change the order in the dropdown or change the link to "Secret or Config Map"

Yeah, we should fix the group ordering. It might be a random order now.

spadgett · 2017-10-02T17:18:30Z

[merge]

spadgett · 2017-10-02T17:48:33Z

[test]

spadgett · 2017-10-02T18:24:46Z

Opened flake #2200

spadgett · 2017-10-02T19:53:00Z

[merge]

spadgett · 2017-10-02T19:53:04Z

[test]

spadgett · 2017-10-03T11:36:05Z

[merge]

spadgett · 2017-10-03T11:36:10Z

[test]

openshift-bot · 2017-10-03T11:39:16Z

Evaluated for origin web console test up to 544127e

openshift-bot · 2017-10-03T11:39:24Z

Evaluated for origin web console merge up to 544127e

openshift-bot · 2017-10-03T12:07:10Z

Origin Web Console Test Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/250/) (Base Commit: 9c144f3) (PR Branch Commit: 544127e)

openshift-bot · 2017-10-03T12:07:11Z

Origin Web Console Merge Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/308/) (Base Commit: 9c144f3) (PR Branch Commit: 544127e)

spadgett reviewed Oct 2, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom branch from 9e1b927 to 77d5889 Compare October 2, 2017 14:42

dtaylor113 approved these changes Oct 2, 2017

View reviewed changes

cdcabrera changed the title ~~[WIP] EnvFrom Issue Updates~~ EnvFrom Issue Updates Oct 2, 2017

spadgett reviewed Oct 2, 2017

View reviewed changes

benjaminapetersen reviewed Oct 2, 2017

View reviewed changes

cdcabrera force-pushed the issue-envfrom branch from 77d5889 to 9aa346e Compare October 2, 2017 15:35

EnvFrom Issue Updates

544127e

EnvFrom issue fixes for CSS, copy, and UX

cdcabrera force-pushed the issue-envfrom branch from 9aa346e to 544127e Compare October 2, 2017 15:39

spadgett approved these changes Oct 2, 2017

View reviewed changes

cdcabrera mentioned this pull request Oct 3, 2017

EnvFrom in the Env Editors Interface Issues #2182

Closed

6 tasks

spadgett merged commit 32e45b8 into openshift:master Oct 3, 2017

cdcabrera mentioned this pull request Oct 3, 2017

EnvFrom Copy Fixes #2211

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EnvFrom Issue Updates #2198

EnvFrom Issue Updates #2198

cdcabrera commented Oct 2, 2017 •

edited

Loading

spadgett Oct 2, 2017

beanh66 Oct 2, 2017 •

edited

Loading

spadgett Oct 2, 2017

spadgett Oct 2, 2017 •

edited

Loading

cdcabrera Oct 2, 2017

spadgett Oct 2, 2017 •

edited

Loading

spadgett Oct 2, 2017 •

edited

Loading

cdcabrera Oct 2, 2017

benjaminapetersen Oct 2, 2017

spadgett commented Oct 2, 2017

benjaminapetersen commented Oct 2, 2017

spadgett commented Oct 2, 2017 •

edited

Loading

spadgett commented Oct 2, 2017

beanh66 commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 3, 2017

spadgett commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

EnvFrom Issue Updates #2198

EnvFrom Issue Updates #2198

Conversation

cdcabrera commented Oct 2, 2017 • edited Loading

spadgett Oct 2, 2017

Choose a reason for hiding this comment

beanh66 Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

spadgett Oct 2, 2017

Choose a reason for hiding this comment

spadgett Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

cdcabrera Oct 2, 2017

Choose a reason for hiding this comment

spadgett Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

spadgett Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

cdcabrera Oct 2, 2017

Choose a reason for hiding this comment

benjaminapetersen Oct 2, 2017

Choose a reason for hiding this comment

spadgett commented Oct 2, 2017

benjaminapetersen commented Oct 2, 2017

spadgett commented Oct 2, 2017 • edited Loading

spadgett commented Oct 2, 2017

beanh66 commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 2, 2017

spadgett commented Oct 3, 2017

spadgett commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

openshift-bot commented Oct 3, 2017

cdcabrera commented Oct 2, 2017 •

edited

Loading

beanh66 Oct 2, 2017 •

edited

Loading

spadgett Oct 2, 2017 •

edited

Loading

spadgett Oct 2, 2017 •

edited

Loading

spadgett Oct 2, 2017 •

edited

Loading

spadgett commented Oct 2, 2017 •

edited

Loading