Add/Edit env vars from secrets and config maps #1090

rhamilto · 2017-01-09T14:26:59Z

TODOs:

- name: OTHER
   valueFrom:
   resourceFieldRef:
   resource: requests.cpu 
   divisor: '0'

[NICE TO HAVE] Add links to create a config map or secret (in a modal)

rhamilto · 2017-01-09T14:32:19Z

@benjaminapetersen, @jwforres, I need an assist getting KVE running. I'm not receiving any errors, but the editor isn't rendering. Not sure how to troubleshoot.

spadgett · 2017-01-09T14:42:27Z

@rhamilto did you add the JS files to index.html?

rhamilto · 2017-01-09T14:47:57Z

Thanks, @spadgett. I had overlooked the addition.

benjaminapetersen · 2017-01-16T15:03:15Z

app/scripts/directives/keyValueEditor.js

@@ -286,6 +302,36 @@
                }
              });

+              var findReferenceValue = function(entry) {


Think its possible to break this one down a bit? There are a lot of returns at various levels. Might be helpful to clarify (via function names) what is going on?

rhamilto · 2017-01-31T18:31:02Z

app/scripts/directives/keyValueEditor.js

+                  }
+                };
+                console.log('configMapObj', configMapObj);
+                entry.valueAlt = 'Set to the key ' + from.key + ' in config map <a ng-if="\'configMaps\' | canI : \'get\'" ng-href="{{configMapObj | navigateResourceURL}}">' + from.name + '</a>';


@benjaminapetersen or @jwforres, this doesn't create a valid href, and I'm not sure why. Help?

Looking into using $compile to get the filters to render correctly.

rhamilto · 2017-02-10T14:48:02Z

@jwforres, @spadgett Thoughts? We're currently checking to see if the user has access to config maps or secrets, but not if the config map or secret exists.

jwforres

did a rough pass through

jwforres · 2017-02-14T21:26:59Z

app/scripts/controllers/create/createFromImage.js

-              secretsArray.unshift("");
-            });
-          });
+          // TODO: make valueFromObjects properly


can you explain this TODO better for someone else looking at this later

^ @benjaminapetersen, I'm thinking this comment can simply be deleted?

agree... i don't think that has meaning anymore.

Comment removed.

jwforres · 2017-02-14T21:35:52Z

app/scripts/services/images.js

+      return _.isArray(input) ?
+              input :
+              _.map(input, function(value, key){
+                return {name: key, value: value};


how is this handling valueFrom? same question in applicationGenerator

^ @benjaminapetersen

This is just a switch for backwards compatibility.

if it gets a hash, this is the "old way" that needs to transform into [{name:'',value:''}, etc]

If it gets an array, return the array (input arg). We've already

Off the top of my head I'm not sure why it received a hash of {'<name>': '<value>'} to begin with.

jwforres · 2017-02-14T21:44:21Z

app/scripts/controllers/deployment.js

@@ -112,6 +124,7 @@ angular.module('openshiftConsole')
            $scope.loaded = true;
            $scope.deployment = deployment;
            updateHPAWarnings();
+            EnvironmentService.toggleReadonlyForContainerEnvsValueFrom(_.get($scope.deployment, 'spec.template.spec.containers'), envSecretAndConfigMapAreReadonly);


Is the purpose of all of these calls to toggleReadonly just to wait until secrets/configmaps are loaded before letting you touch the dropdowns? If so I want to discuss a different approach tomorrow. I'm not a fan of adding more gorp to the model data, I already don't like how much extra stuff we are adding that doesn't need to be getting sent to the server.

Good word, gorp. We have keyValueEditorUtils.cleanEntries() setup to clean off all the gorp & went through it pretty thoroughly to ensure we didn't miss anything. Agree its a bit clunky & i'd love to fix it. Thought about kve_ prefixing all the stuff we add so it would be easier to clean off, but there might be a better way.

Other than the gorp, looking @ the toggle again per our discussion to see if we can clean up.

We considered moving the DataService.list("secrets") and DataService.list("configmaps") into the directive & eliminating the controller noise... might be a good way to fix this.

jwforres · 2017-02-14T21:52:27Z

app/scripts/directives/lifecycleHook.js

@@ -10,6 +10,8 @@ angular.module("openshiftConsole")
        hookParams: "=model",
        availableVolumes: "=",
        availableContainers: "=",
+        availableSecrets: "=",
+        availableConfigmaps: "=",


availableConfigMaps

Good catch. Fixed.

jwforres · 2017-02-14T21:58:54Z

app/views/directives/key-value-editor.html

+
+        <div ng-if="(!entry.valueFrom)">
+          <input
+            ng-if="(!entry.valueAlt)"


is this ng-if still needed?

Good catch. Fixed.

jwforres · 2017-02-14T22:01:02Z

app/views/directives/key-value-editor.html

+                <span ng-switch-when="configMapKeyRef">
+                  Set to the key {{entry.valueFrom.configMapKeyRef.key}} in config map
+                  <span
+                    ng-if="entry.isReadonlyValue">


why is the appearance of the name as a link dependent on whether the value is readonly? shouldn't the appearance of the link depend on whether the user has rights to view secrets / view config maps using a canI check?

We are using canI to check if the user has rights to view config maps or secrets, we're just using a single scope parameter (entry.isReadonlyValue) to track multiple conditions as this was easier than having multiple params. The thought being it was better to keep this logic in the controller and have one less key to clean off entry. Yea/nay?

If the user can get config maps or secrets and the kve instance is NOT read only, we show the ui-selects for config maps and secrets.

If the user can get config maps or secrets and the kve instance is read only, we show the faux input with the link.

If the user can NOT get config maps or secrets and the kve instance is NOT read only, we show the faux input without the link.

If the user can NOT get config maps or secrets and the kve instance is read only, we show the faux input without the link.

Should we doc this somewhere? Comments @ the top of the directive for future reference?

jwforres · 2017-02-14T22:03:40Z

app/views/directives/key-value-editor.html

+          <div ng-if="(!isReadonlyAny) && (!entry.isReadonlyValue)">
+            <div class="ui-select">
+              <ui-select ng-model="entry.selectedValueFrom" ng-required="true" on-select="valueFromObjectSelected(entry, $select.selected)">
+                <ui-select-match placeholder="Select config map or secret">


we made the link configurable to support other options down the road, but then hardcoded this one into the editor. We made the list of objects generic to support other types, would this be better off as just "Select a resource" or something.

Good point. Changed to "Select a resource"

jwforres · 2017-02-17T21:02:57Z

No that is risky, you'll end up re-listing in some cases if the directive gets re-created for any reason (like an ng-if changing state)

…

On Fri, Feb 17, 2017 at 3:21 PM, Ben Petersen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/scripts/controllers/deployment.js <#1090 (comment)> : > @@ -112,6 +124,7 @@ angular.module('openshiftConsole') $scope.loaded = true; $scope.deployment = deployment; updateHPAWarnings(); + EnvironmentService.toggleReadonlyForContainerEnvsValueFrom(_.get($scope.deployment, 'spec.template.spec.containers'), envSecretAndConfigMapAreReadonly); We considered moving the DataService.list("secrets") and DataService.list("configmaps") into the directive & eliminating the controller noise... might be a good way to fix this. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1090 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7ZqFWFjBescyMJ1bAJ2DHPaz5G-Xks5rdgFggaJpZM4LeT7a> .

jwforres · 2017-02-17T21:10:02Z

Yeah I definitely disagree with this approach. You are lumping together multiple things onto an attribute that no longer means what the variable is called. readonly is readonly, readonly doesn't mean don't let me have a link to something. Why is the canI check not right here in the view, you have everything you need there already, you know exactly which type of resource is being referenced. The whole point of us pulling in the KVE was to stop doing hacky things to work around limitations of making stuff generic.

…

On Wed, Feb 15, 2017 at 9:14 AM, Robb Hamilton ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/views/directives/key-value-editor.html <#1090>: > + <div ng-if="entry.valueFrom"> + <div ng-if="isReadonlyAny || entry.isReadonlyValue" class="faux-input-group"> + <span + class="faux-form-control-addon {{entry.valueIcon}}" + aria-hidden="true" + data-toggle="tooltip" + data-placement="top" + data-original-title="{{entry.valueIconTooltip || valueIconTooltip}}" + title="{{entry.valueIconTooltip || valueIconTooltip}}"></span> + <div + class="faux-form-control readonly"> + <span ng-switch="entry.refType"> + <span ng-switch-when="configMapKeyRef"> + Set to the key {{entry.valueFrom.configMapKeyRef.key}} in config map + <span + ng-if="entry.isReadonlyValue"> We are using canI to check if the user has rights to view config maps or secrets <https://github.com/rhamilto/origin-web-console/blob/3940968a6e3e5ab3b881f396788b85236c5f746c/app/scripts/directives/keyValueEditor.js#L197-L198>, we're just using a single scope parameter (entry.isReadonlyValue) to track multiple conditions as this was easier than having multiple params. The thought being it was better to keep this logic in the controller and have one less key to clean off entry. Yea/nay? 1. If the user can get config maps or secrets and the kve instance is NOT read only, we show the ui-selects for config maps and secrets. [image: screen shot 2017-02-15 at 9 03 35 am] <https://cloud.githubusercontent.com/assets/895728/22977689/b72ac5d8-f35d-11e6-8587-c951c7cd9917.PNG> 1. If the user can get config maps or secrets and the kve instance is read only, we show the faux input with the link. [image: screen shot 2017-02-15 at 9 03 46 am] <https://cloud.githubusercontent.com/assets/895728/22977703/c39d5182-f35d-11e6-8fdc-c522c7b97fc7.PNG> 1. If the user can NOT get config maps or secrets and the kve instance is NOT read only, we should the faux input without the link. [image: screen shot 2017-02-15 at 9 11 42 am] <https://cloud.githubusercontent.com/assets/895728/22977982/cf8ee996-f35e-11e6-81be-dccb91047541.PNG> 1. If the user can NOT get config maps or secrets and the kve instance is read only, we should the faux input without the link. [image: screen shot 2017-02-15 at 9 11 36 am] <https://cloud.githubusercontent.com/assets/895728/22977990/d6fd22f6-f35e-11e6-9c78-151da84646df.PNG> — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1090>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7Qz447q6NPV-MPOrOUTcQ0CW_R_gks5rcwhEgaJpZM4LeT7a> .

benjaminapetersen · 2017-02-17T21:20:56Z

Everything is being cleaned, either via compact() or directly.

benjaminapetersen · 2017-02-17T21:26:36Z

@jwforres I believe we did it to reduce template logic, we began in the view file but ended up branching/copy paste more than we liked. Doing the check up front simplified the branching.

benjaminapetersen · 2017-02-17T21:30:12Z

@jwforres isReadonly still just controls wether or not you get the select pickers. In the readonly state, we use canI to render either plain text, or text with a link, based on if you have perms to work with secrets/configmaps (outlined in this comment).

rhamilto · 2017-02-17T22:07:25Z

app/views/directives/key-value-editor.html

+        role="button"
+        aria-label="Add row"
+        ng-click="onAddRow()">{{ addRowLink }}</a>
+      <span


Moar simpler: a single span wrapping both the pipe and the link with ng-if="valueFromSelectorOptions.length"

Moar simpler: a single span wrapping both the pipe and the link with ng-if="valueFromSelectorOptions.length"

Done.

rhamilto · 2017-02-17T22:08:18Z

app/views/directives/key-value-editor.html

+                <span ng-switch-when="secretKeyRef">
+                  Set to the key {{entry.valueFrom.secretKeyRef.key}} in secret
+                  <span
+                    ng-if="entry.isReadonlyValue">


canI is much clearer to anyone reading the template. Let's use it instead.

canI is much clearer to anyone reading the template. Let's use it instead.

Done

benjaminapetersen · 2017-02-17T22:11:57Z

@jwforres gonna jot down the flow here quick as well, with deployments as an example. It will explain the odd envSecretAndConfigMapAreReadonly variable. I'm open to improvements.

the DataService.get('deployments') is called initially & sets envSecretAndConfigMapAreReadonly to true since we don't have secrets/configmaps yet. Nothing should be editable.
then the DataService.watchObject('deployments') is called, along with DataService.list('configmaps') and DataService.list('secrets'). We don't know which will return first, which is why we have the quirky logic.
If the DataService.watchObject('deployments') happens to fire, envSecretAndConfigMapAreReadonly does not flip, there is no new info for us to toggle state (we already had this). This watch, if fired, needs to regenerate the state as it is, not knowing if it has secrets/configmaps or not.
But, if either DataService.list("configmaps") or DataService.list("secrets") return first, we do flip the bit, and we can now show the pickers.
Since the watch may update many times, we need it to just trust the state of envSecretAndConfigMapAreReadonly wherever it has landed based on the other 3 calls.

Hopefully that makes sense... and moves along some discussion about simplifying if we can make it happen!

jwforres · 2017-02-20T18:27:51Z

app/views/directives/key-value-editor.html

+              data-toggle="tooltip"
+              data-placement="top"
+              data-original-title="{{entry.valueIconTooltip || valueIconTooltip}}"
+              title="{{entry.valueIconTooltip || valueIconTooltip}}"></span>


while we are fixing things... can you remove the extra title attribute here, it is causing https://bugzilla.redhat.com/show_bug.cgi?id=1395079

jwforres · 2017-02-21T15:14:28Z

[merge]

jwforres · 2017-02-21T16:03:23Z

[merge]

jwforres · 2017-02-21T16:42:24Z

take three [merge]

…values from config maps or secrets

- add valueFromSelectorOptions checks in kve to detect readonly state - add isValueFromEditable fn to clean up view - remove redundant title= attrs

jwforres · 2017-02-21T20:04:54Z

[merge]

jwforres · 2017-02-21T20:39:20Z

[merge]

openshift-bot · 2017-02-21T20:47:13Z

Evaluated for origin web console merge up to 7d79245

openshift-bot · 2017-02-21T20:47:21Z

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

benjaminapetersen reviewed Jan 16, 2017

View reviewed changes

rhamilto changed the title ~~[WIP]: Adding KVE code directly to the console~~ [WIP]: Add/Edit env vars from secrets and config maps Jan 18, 2017

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

rhamilto removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 31, 2017

rhamilto commented Jan 31, 2017

View reviewed changes

rhamilto changed the title ~~[WIP]: Add/Edit env vars from secrets and config maps~~ Add/Edit env vars from secrets and config maps Feb 10, 2017

rhamilto requested review from spadgett and jwforres February 10, 2017 21:30

jwforres requested changes Feb 14, 2017

View reviewed changes

rhamilto commented Feb 17, 2017

View reviewed changes

jwforres reviewed Feb 20, 2017

View reviewed changes

jwforres approved these changes Feb 21, 2017

View reviewed changes

rhamilto and others added 2 commits February 21, 2017 14:39

Adding KVE directly to console and adding support for adding/editing …

2487128

…values from config maps or secrets

Eliminate kve bookkeeping in controllers

7d79245

- add valueFromSelectorOptions checks in kve to detect readonly state - add isValueFromEditable fn to clean up view - remove redundant title= attrs

openshift-bot merged commit 8c3bb14 into openshift:master Feb 21, 2017

rhamilto deleted the kve branch February 22, 2017 14:38

Add/Edit env vars from secrets and config maps #1090

Add/Edit env vars from secrets and config maps #1090

Conversation

rhamilto commented Jan 9, 2017 • edited Loading

rhamilto commented Jan 9, 2017

spadgett commented Jan 9, 2017

rhamilto commented Jan 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhamilto commented Feb 10, 2017

jwforres left a 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

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

rhamilto Feb 15, 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

jwforres commented Feb 17, 2017 via email

jwforres commented Feb 17, 2017 via email

benjaminapetersen commented Feb 17, 2017

benjaminapetersen commented Feb 17, 2017 • edited Loading

benjaminapetersen commented Feb 17, 2017

rhamilto Feb 17, 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

benjaminapetersen commented Feb 17, 2017

Choose a reason for hiding this comment

jwforres commented Feb 21, 2017

jwforres commented Feb 21, 2017

jwforres commented Feb 21, 2017

jwforres commented Feb 21, 2017

jwforres commented Feb 21, 2017

openshift-bot commented Feb 21, 2017

openshift-bot commented Feb 21, 2017 • edited Loading

rhamilto commented Jan 9, 2017 •

edited

Loading

rhamilto Feb 15, 2017 •

edited

Loading

benjaminapetersen commented Feb 17, 2017 •

edited

Loading

rhamilto Feb 17, 2017 •

edited

Loading

openshift-bot commented Feb 21, 2017 •

edited

Loading