Support EnvFrom in the Env Editors #2133

cdcabrera · 2017-09-21T15:08:18Z

Add dropdown support to give the ability to add bulk additions within the environment editors. Design documentation located here

Notes:

~~Comment clean up still required~~
~~CSS/Styling~~
~~Some awkward behavior around validation and allowing a save when removing the last "envFrom" entry~~

spadgett

@cdcabrera Great start. One general comment on names containing valueFrom.

Can you look at the logic here?

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/editEnvironmentVariables.js#L50-L57

We have some code to check if the environment has changed on watch updates. We'll want to check envFrom as well.

spadgett · 2017-09-21T15:14:56Z

app/scripts/directives/editEnvironmentFrom.js

+    bindings: {
+      addRowLink: '@',
+      entries: '=',
+      valueFromSelectorOptions: '<',


envFromOptions ?

np, can change. Just reused the valueFromSelectorOptions since it helps generate the same list... can change the name no problem

spadgett · 2017-09-21T17:17:05Z

app/scripts/directives/editEnvironmentFrom.js

+      return _.includes(cannotDeleteSome, name);
+    };
+
+    ctrl.isValueFromReadonly = function() {


We should avoid valueFrom in names throughout this control. That refers specifically to choosing an individual key from a secret. It's going to get confusing if we use that in names for envFrom.

cdcabrera · 2017-09-22T20:12:38Z

@openshift/team-ux-review need some input on the tooltip copy for the Environment From section...

The original tooltip copy that shows up in the designs...

All keys from these config maps and secrets will be added as environment variables.

And a possible alternative...

Environment From lets you add all key value pairs from a config map or secret.

cdcabrera · 2017-09-22T20:35:27Z

@spadgett looks like updating the Environment Service requires an update to the associated unit test, or some additional checks within the service.

I could update the test to avoid this failure by placing an empty array for envFrom: [] within the expected results?

spadgett · 2017-09-23T13:17:17Z

I could update the test to avoid this failure by placing an empty array for envFrom: [] within the expected results?

This is fine as long as we handle the case where envFrom is missing from the resource we pass in. It's an optional field.

cdcabrera · 2017-09-25T15:45:32Z

@spadgett verified the "env" vs "envFrom" aspects of the unit tests. Behavior appears to be the same with the addition of an empty array vs the missing resource. Going to push the updated unit test change.

Edit: there is a minor difference where "env" comes back as a literal undefined and envFrom won't appear at all in the testing... there may be an additional process running on "env" vs "envFrom" or vice versa

spadgett · 2017-09-26T13:10:37Z

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

+    ng-if="$ctrl.showHeader"
+    class="key-value-editor-entry key-value-editor-entry-header">
+    <div class="form-group key-value-editor-header value-header">
+      <div class="input-group">


Why wrap in a form-group and input-group?

Part of the classes copied over from the variables component (heads up... may be some clean up there too), also part of the CSS clean up we've been going through, lots of extras to remove.

OK to leave if it's reusing the styles and needed

spadgett · 2017-09-26T13:11:17Z

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

@@ -0,0 +1,94 @@
+<ng-form name="forms.editEnvironmentFrom" novalidate>


Should this be $ctrl.editEnvironmentFrom here and below?

Is that a vote to remove the ng-form?

I may be able to, but was under the impression we were trying to maintain the parent-child relationship with the parent form?

You should be able to keep ng-form and use $ctrl.editEnvironmentForm I think. Is this sharing the same form object as the parent? I don't think that should be necessary.

spadgett · 2017-09-26T13:12:40Z

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

+          <div class="faux-form-control readonly">
+            Set to values in {{entry.selectedEnvFrom.kind | humanizeKind : true | lowercase}}
+            <span
+              ng-if="!('configmaps' | canI : 'get') || !('secrets' | canI : 'get')">


This doesn't look right. If the value is a config map, we should be able to link to that even if I can't view secrets.

Can adjust the logic, look at separating it out

spadgett · 2017-09-26T13:15:22Z

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

+        href=""
+        class="add-row-link"
+        role="button"
+        aria-label="Add row"


Probably don't want aria-label here so that the screen reader uses addRowLink

I can remove sure thing, just heads up... it's copied from the variables display

Let's fix it there, too.

spadgett · 2017-09-26T13:16:19Z

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

+      <span class="pficon pficon-info"
+            aria-hidden="true"
+            data-toggle="tooltip"
+            data-original-title="Environment From lets you add all key value pairs from a config map or secret."></span>


"Environment From lets you add all key-value pairs from a config map or secret as environment variables."

Copy update, np

spadgett · 2017-09-26T13:23:04Z

We might rethink the "View Secret" link since it navigates away from the page, losing your edits.

cc @beanh66

spadgett · 2017-09-26T13:23:55Z

I noticed the highlighted item is not very legible:

spadgett · 2017-09-26T13:28:53Z

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

+
+    <h4>
+      Environment From
+      <span class="pficon pficon-info"


We usually use a ?(pficon-help) for these tooltips, for instance:

@beanh66 Any objection to switching for consistency?

spadgett · 2017-09-26T13:31:22Z

These labels should be updated as well as in the design:

spadgett · 2017-09-26T13:32:24Z

I think the "Variables" heading is self-evident. I wonder if we should just remove that heading?

spadgett · 2017-09-26T13:39:51Z

Mobile styles are off:

spadgett · 2017-09-26T14:11:56Z

Need to fix experience when logged in as a project viewer. You can add a user with view role using the Resources -> Membership page.

beanh66 · 2017-09-26T19:47:06Z

@spadgett agreed on the view secret link! I talked to @cdcabrera about having the link be called "View Details" or "View Variables" instead and clicking the link would pop a modal to display the secret or config map variables (rather than navigating away). I also mentioned that if we go this route, the link should be available right away after a selection is made. Previously we were only showing that link after the save button was clicked. Thoughts?

benjaminapetersen · 2017-09-26T20:38:53Z

Seeing some premature collapse of the "x" to a new column @ tablet-ish size:

benjaminapetersen · 2017-09-26T20:48:18Z

app/scripts/directives/editEnvironmentFrom.js

+    ctrl.$onInit = function() {
+      ctrl.updateEnvFromEntries(ctrl.entries);
+      findReferenceValueForEntries(ctrl.envFromEntries, ctrl.envFromSelectorOptions);
+


Are you wanting to continue supporting cannotAdd, cannotSort, isReadonly, showHeader as the <key-value-editor> had them? If so, I'd prob suggest including them in bindings above. I believe this chunk of if statements was so you could put the attrib on the DOM node w/o a value:

<edit-environment-from cannot-add></edit-environment-from>  <edit-environment-from cannot-add="'true'"></edit-environment-from>

benjaminapetersen · 2017-09-26T20:50:27Z

app/scripts/directives/editEnvironmentFrom.js

+        }
+      });
+    };
+


nit: for consistency, prob can drop the angular.extend and just do ctrl.dragControlListeners = since the rest of the file uses this style.

benjaminapetersen · 2017-09-26T20:51:46Z

app/scripts/directives/editEnvironmentFrom.js

+
+    var canI = $filter('canI');
+    var humanizeKind = $filter('humanizeKind');
+


I don't think you are using ctrl.$id in the view, so can prob change this to var id.

benjaminapetersen · 2017-09-26T20:59:35Z

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

+
+      <div
+        class="form-group key-value-editor-input"
+        ng-class="{ 'has-error': (forms.editEnvironmentFrom[uniqueForValue(unique, $index)].$invalid && forms.editEnvironmentFrom[uniqueForValue(unique, $index)].$touched) }">


I don't think the has-error class will ever be applied, as uniqueForValue is a function that doesn't exist. It is added to $scope in key-value-editor, if you want to use it prob need to add it as $ctrl.uniqueForValue.

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/keyValueEditor.js#L207-L208

Also in uniqueForValue(unique, $index), unique is $scope.unique, which is also missing here (would be ctrl.unique).
https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/keyValueEditor.js#L202

Btw, the uniqueFor helpers are to generate uniqueIds for the inputs, and match up the labels, has-error classes, etc.

https://github.com/openshift/origin-web-console/blob/master/app/views/directives/key-value-editor.html#L37

The ctrl.unique is used in key-value-editor to ensure that you can have more than one editor on a page w/o collisions. Basically forms.editEnvironmentFrom[uniqueForValue(unique, $index)] returns the matching id of one of the <input id="???"> elements.

Agreed! Removing the forms reference from the earlier review and the lack of the uniqueForValue method seemed to build/accumulate, looks like an adjustment for the readonly section needs to take place

benjaminapetersen · 2017-09-26T21:14:59Z

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

+            ng-href="{{entry.selectedEnvFrom | navigateResourceURL}}"
+            class="pficon"
+            ng-show="entry.selectedEnvFrom"
+            ng-click="$ctrl.viewDetail(entry)">View {{entry.selectedEnvFrom.kind | humanizeKind : true}}</a>


I think $ctrl.viewDetail doesn't exist? the href works, tho.

Yeahhhhhhhh =), removing appears superfluous looks like it was from a previous iteration

benjaminapetersen · 2017-09-26T21:18:47Z

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

+    <edit-environment-from
+      entries="container.envFrom"
+      selector-placeholder="Secret/Config Map"
+      env-from-selector-options="$ctrl.valueFromObjects"


See above about env-from-selector-options.

benjaminapetersen · 2017-09-26T21:20:59Z

app/scripts/directives/editEnvironmentFrom.js

+    ],
+    bindings: {
+      addRowLink: '@',
+      entries: '=',


I'm not sure I'm crazy about envFromSelectorOptions...I'd at least like a comment about what it means. Its config for something, but at a glance I'm not sure what (might be my ignorance re: EnvironmentFrom).

An annotation couldn't hurt, we'll add one in

cdcabrera · 2017-09-27T16:55:24Z

@spadgett @benjaminapetersen there's still a missing method around edit-environment-from.html and uniqueForValue to help generate unique select Ids...

the Read Only, in general, logic probably needs additional adjustment...
and the "has-error" class, which feels like it should be removed from the Read Only section

ng-class="{ 'has-error': ($ctrl.editEnvironmentFromForm[uniqueForValue(unique, $index)].$invalid && $ctrl.editEnvironmentFromForm[uniqueForValue(unique, $index)].$touched) }">

spadgett · 2017-09-27T17:17:20Z

Let's fix the readonly state

spadgett · 2017-09-27T17:02:06Z

app/scripts/directives/editEnvironmentFrom.js

+
+    var canI = $filter('canI');
+    var humanizeKind = $filter('humanizeKind');
+    var namespace = $routeParams.project;


Better to pass the project in than read it from route params. But why is it needed?

For the comment chain, related to the below dummy values referenced in editEnvironmentFrom.js

spadgett · 2017-09-27T17:03:25Z

app/scripts/directives/editEnvironmentFrom.js

+              namespace: namespace
+            }
+          });
+        });


I'd rather not set dummy values. Is there another way to handle this? Is this for handling secrets that have been deleted?

For the comment chain, we're removing this as discussed

spadgett · 2017-09-27T17:06:22Z

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

+          ng-hide="$ctrl.cannotDeleteAny"
+          ng-click="$ctrl.deleteEntry($index, 1)"></a>
+        <a
+          ng-if="!$ctrl.editEnvironmentFromForm.$dirty"


We can't just check this form because the normal env var form above might have changed.

I suggest we handle "View Secret" as a follow on issue and remove it for now.

spadgett · 2017-09-27T17:07:56Z

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

@@ -32,6 +34,21 @@
      add-row-with-selectors-link="Add Environment Variable Using a Config Map or Secret"
      show-header>


Just above this line:

add-row-link="Add Variable" add-row-with-selectors-link="Add Variable from Config Map or Secret"

beanh66 · 2017-09-27T17:44:27Z

@spadgett Do we want to change the Environment From title? @cdcabrera the field level help next to Environment From also doesn't appear to be using the PatternFly styling here, can you take a look? I liked your suggestion for the wording change regarding the tooltip though.

spadgett · 2017-09-27T18:11:34Z

@cdcabrera the field level help next to Environment From also doesn't appear to be using the PatternFly styling here, can you take a look?

@beanh66 It's using the style we use for help tooltips throughout the console. I'm not opposed to changing, but I'd like to change them everywhere to be consistent (and probably out of scope for this PR).

That said, the icon is larger here than it is in other places. For instance, this is the build config editor with a similar help tooltip:

spadgett · 2017-09-27T18:45:16Z

spadgett · 2017-09-27T18:46:59Z

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

+              No secrets or config maps have been added as Environment From.
+          </div>
+          <div ng-if="entry.configMapRef.name || entry.secretRef.name" class="faux-form-control readonly">
+              Set to values in {{entry.configMapRef.name || entry.secretRef.name}}


Probably should say Use all keys and values from <kind> <name>.

Use all keys and values from <span ng-if="entry.configMapRef.name">config map {{entry.configMapRef.name}}</span> <span ng-if="entry.secretRef.name">secret {{entry.secretRef.name}}</span>

spadgett · 2017-09-27T19:59:33Z

[merge]

spadgett · 2017-09-27T20:24:45Z

@cdcabrera

Built /dist does not match what is committed, run 'grunt build' and include the results in your commit.

Add dropdown support to give the ability to add bulk additions within the environment editors

spadgett · 2017-09-27T20:52:54Z

[merge]

openshift-bot · 2017-09-27T20:55:31Z

Evaluated for origin web console merge up to 7165cef

openshift-bot · 2017-09-27T20:55:40Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/284/) (Base Commit: ac5246b) (PR Branch Commit: 7165cef)

cdcabrera requested a review from spadgett September 21, 2017 15:08

spadgett reviewed Sep 21, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2017

spadgett added this to the 3.7.0 milestone Sep 22, 2017

cdcabrera force-pushed the envfrom-enveditors branch from ae6f733 to 71ac457 Compare September 22, 2017 19:51

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2017

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2017

cdcabrera force-pushed the envfrom-enveditors branch 2 times, most recently from ffb97c1 to 24e22d2 Compare September 25, 2017 19:07

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 25, 2017

spadgett requested changes Sep 26, 2017

View reviewed changes

spadgett reviewed Sep 26, 2017

View reviewed changes

cdcabrera force-pushed the envfrom-enveditors branch from 24e22d2 to 69e10c9 Compare September 26, 2017 15:25

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2017

cdcabrera force-pushed the envfrom-enveditors branch from 69e10c9 to 3ab3d9a Compare September 26, 2017 19:32

cdcabrera requested a review from benjaminapetersen September 26, 2017 19:33

benjaminapetersen reviewed Sep 26, 2017

View reviewed changes

benjaminapetersen suggested changes Sep 26, 2017

View reviewed changes

cdcabrera force-pushed the envfrom-enveditors branch from ed143ef to 1ba99ba Compare September 27, 2017 16:45

spadgett reviewed Sep 27, 2017

View reviewed changes

cdcabrera force-pushed the envfrom-enveditors branch from 18eeb46 to 42ee449 Compare September 27, 2017 18:24

spadgett reviewed Sep 27, 2017

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2017

cdcabrera force-pushed the envfrom-enveditors branch from 42ee449 to 034cf29 Compare September 27, 2017 19:45

spadgett approved these changes Sep 27, 2017

View reviewed changes

spadgett changed the title ~~[WIP] Support EnvFrom in the Env Editors~~ Support EnvFrom in the Env Editors Sep 27, 2017

Support EnvFrom in the Env Editors

7165cef

Add dropdown support to give the ability to add bulk additions within the environment editors

cdcabrera force-pushed the envfrom-enveditors branch from 034cf29 to 7165cef Compare September 27, 2017 20:44

openshift-bot merged commit 18330d5 into openshift:master Sep 27, 2017

spadgett removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2017

		@@ -0,0 +1,94 @@
		<ng-form name="forms.editEnvironmentFrom" novalidate>


		var canI = $filter('canI');
		var humanizeKind = $filter('humanizeKind');

		@@ -32,6 +34,21 @@
		add-row-with-selectors-link="Add Environment Variable Using a Config Map or Secret"
		show-header>

Support EnvFrom in the Env Editors #2133

Support EnvFrom in the Env Editors #2133

Conversation

cdcabrera commented Sep 21, 2017 • edited Loading

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcabrera Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcabrera commented Sep 22, 2017

cdcabrera commented Sep 22, 2017

spadgett commented Sep 23, 2017

cdcabrera commented Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

cdcabrera Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

spadgett Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Sep 26, 2017

spadgett commented Sep 26, 2017

Choose a reason for hiding this comment

spadgett commented Sep 26, 2017 • edited Loading

spadgett commented Sep 26, 2017

spadgett commented Sep 26, 2017

spadgett commented Sep 26, 2017

beanh66 commented Sep 26, 2017

benjaminapetersen commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcabrera Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdcabrera commented Sep 27, 2017

spadgett commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beanh66 commented Sep 27, 2017

spadgett commented Sep 27, 2017

spadgett commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Sep 27, 2017

spadgett commented Sep 27, 2017

spadgett commented Sep 27, 2017

openshift-bot commented Sep 27, 2017

openshift-bot commented Sep 27, 2017 • edited Loading

cdcabrera commented Sep 21, 2017 •

edited

Loading

cdcabrera Sep 21, 2017 •

edited

Loading

cdcabrera commented Sep 25, 2017 •

edited

Loading

cdcabrera Sep 26, 2017 •

edited

Loading

spadgett Sep 26, 2017 •

edited

Loading

spadgett commented Sep 26, 2017 •

edited

Loading

cdcabrera Sep 26, 2017 •

edited

Loading

openshift-bot commented Sep 27, 2017 •

edited

Loading