Skip to content

EnvFrom Issue Updates#2198

Merged
spadgett merged 1 commit intoopenshift:masterfrom
cdcabrera:issue-envfrom
Oct 3, 2017
Merged

EnvFrom Issue Updates#2198
spadgett merged 1 commit intoopenshift:masterfrom
cdcabrera:issue-envfrom

Conversation

@cdcabrera
Copy link
Contributor

@cdcabrera cdcabrera commented Oct 2, 2017 •
edited
Loading

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

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

spadgett
spadgett reviewed Oct 2, 2017
View reviewed changes
app/views/directives/edit-environment-variables.html Outdated
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"
Copy link
Member

@spadgett spadgett Oct 2, 2017

Choose a reason for hiding this comment

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

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

@beanh66 Are you OK with that change?

Copy link

@beanh66 beanh66 Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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" ?

Copy link
Member

@spadgett spadgett Oct 2, 2017

Choose a reason for hiding this comment

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

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

@beanh66 Yup! That's what I was thinking

@cdcabrera cdcabrera changed the title [WIP] EnvFrom Issue Updates EnvFrom Issue Updates Oct 2, 2017
spadgett
spadgett reviewed Oct 2, 2017
View reviewed changes
app/scripts/directives/editEnvironmentFrom.js Outdated

ctrl.checkEntries = function(option) {
ctrl.checkEntries = function(option, entrySelectedEnvFrom) {
if(_.isEqual(option, entrySelectedEnvFrom)) {
Copy link
Member

@spadgett spadgett Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cdcabrera cdcabrera Oct 2, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@spadgett spadgett Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You could check the UIDs, something like (untested)

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

assuming neither is ever null.

Copy link
Member

@spadgett spadgett Oct 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cdcabrera cdcabrera Oct 2, 2017

Choose a reason for hiding this comment

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

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
benjaminapetersen reviewed Oct 2, 2017
View reviewed changes
app/views/directives/edit-environment-variables.html
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"
Copy link
Contributor

@benjaminapetersen benjaminapetersen Oct 2, 2017

Choose a reason for hiding this comment

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

+1 for clarity here.

@spadgett
Copy link
Member

spadgett commented Oct 2, 2017

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

Before:

openshift web console 2017-10-02 11-32-32

After:

openshift web console 2017-10-02 11-33-55

@cdcabrera
EnvFrom Issue Updates
544127e 
EnvFrom issue fixes for CSS, copy, and UX
@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Oct 2, 2017

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

@spadgett
Copy link
Member

spadgett commented Oct 2, 2017 •
edited
Loading

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
Copy link
Member

spadgett commented Oct 2, 2017

Looks like a webdriver flake

[merge]

@beanh66
Copy link

beanh66 commented Oct 2, 2017

@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
Copy link
Member

spadgett commented Oct 2, 2017

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
Copy link
Member

spadgett commented Oct 2, 2017

[merge]

@spadgett
Copy link
Member

spadgett commented Oct 2, 2017

[test]

@spadgett
Copy link
Member

spadgett commented Oct 2, 2017

Opened flake #2200

@spadgett
Copy link
Member

spadgett commented Oct 2, 2017

[merge]

@spadgett
Copy link
Member

spadgett commented Oct 2, 2017

[test]

@spadgett
Copy link
Member

spadgett commented Oct 3, 2017

[merge]

@spadgett
Copy link
Member

spadgett commented Oct 3, 2017

[test]

@openshift-bot
Copy link

openshift-bot commented Oct 3, 2017

Evaluated for origin web console test up to 544127e

@openshift-bot
Copy link

openshift-bot commented Oct 3, 2017

Evaluated for origin web console merge up to 544127e

@openshift-bot
Copy link

openshift-bot commented Oct 3, 2017

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
Copy link

openshift-bot commented Oct 3, 2017

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)

@cdcabrera cdcabrera mentioned this pull request Oct 3, 2017
Closed
6 tasks
@spadgett spadgett merged commit 32e45b8 into openshift:master Oct 3, 2017
@cdcabrera cdcabrera mentioned this pull request Oct 3, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spadgett spadgett spadgett approved these changes

+3 more reviewers

@benjaminapetersen benjaminapetersen benjaminapetersen left review comments

@beanh66 beanh66 beanh66 left review comments

@dtaylor113 dtaylor113 dtaylor113 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cdcabrera @spadgett @benjaminapetersen @beanh66 @openshift-bot @dtaylor113