Should not be able to add the same secret or config map to an application twice #2249

jhadvig · 2017-10-11T13:54:26Z

The Add to Application modal will only show applications which DC's won't contain the secret/configMap that user wants to add. In case one of the containers won't contain the secret/configMap the application will be shown in the dropbox, but the ref will be added only to the containers that don't contain the it.

Unfortunately if, in case of multiple container DC, user chooses to specify a specific container, the checkbox of the container that already contains the ref wont be pre-selected, cause that could mislead the user that by unchecking the checkbox he could remove the ref from that specific container. Other option would be to pre-select the checkbox and also disable it, but that would require add the disable logic to the select-container directive. Wasn't sure if we need that ATM...

@jeff-phillips-18 @spadgett PTAL

Closes #2226

benjaminapetersen · 2017-10-11T15:33:20Z

app/scripts/directives/addConfigToApplication.js

@@ -26,12 +26,35 @@
    var ctrl = this;
    var humanizeKind = $filter('humanizeKind');

+    var checkContainerForRef = function(container) {


Tested, I think we might need an additional message option:

It says "There are no applications in this project", but that isn't true in this case. I've just already added this secret to both of the apps in my project.

@benjaminapetersen what about "There is not any application with containers to add secret to"

We might want to show all apps in the list, but show an error with a disabled save button when the user tries to pick one that already has the secret. Otherwise it might seem like a bug that their app isn't showing up, and it's not obvious why.

jhadvig · 2017-10-12T13:29:31Z

@benjaminapetersen as suggested by @spadgett I've reworked the logic so the selectbox will list all the applications but we will throw an warning and disable the Save button if the ref is already present in the application containers.

Screen:

PTAL

benjaminapetersen · 2017-10-12T15:24:34Z

Nice, I'm good with that!

spadgett

Thanks @jhadvig, a few comments

spadgett · 2017-10-13T13:24:34Z

app/scripts/directives/addConfigToApplication.js

+    var checkContainerForRef = function(container) {
+      var addRefName = ctrl.apiObject.metadata.name;
+      if (ctrl.apiObject.kind === "ConfigMap") {
+        return _.find(container.envFrom, {configMapRef: {name: addRefName}});


You can use _.some instead of _.find if you are just checking if it exists.

spadgett · 2017-10-13T13:25:09Z

app/scripts/directives/addConfigToApplication.js

+      if (ctrl.apiObject.kind === "ConfigMap") {
+        return _.find(container.envFrom, {configMapRef: {name: addRefName}});
+      } else {
+        return _.find(container.envFrom, {secretRef: {name: addRefName}});


spadgett · 2017-10-13T13:27:31Z

app/scripts/directives/addConfigToApplication.js

@@ -25,6 +25,26 @@
  function AddConfigToApplication($filter, $scope, APIService, ApplicationsService, DataService, Navigate, NotificationsService, StorageService) {
    var ctrl = this;
    var humanizeKind = $filter('humanizeKind');
+    ctrl.canAddRefToApplication = false;
+
+    var checkContainerForRef = function(container) {


Suggest conatinerHasRef

spadgett · 2017-10-13T13:29:53Z

app/scripts/directives/addConfigToApplication.js

+          ctrl.canAddRefToApplication = true;
+          return false;
+        }
+      });


I think this could be shortened to.

var containers = _.get(application, 'spec.template.spec.containers'); ctrl.canAddRefToApplication = !_.every(containers, checkContainerForRef);

spadgett · 2017-10-13T13:32:04Z

app/views/directives/add-config-to-application.html

@@ -5,10 +5,14 @@
  <div class="dialog-body">
    <form name="addToApplicationForm" novalidate>
      <fieldset ng-disabled="disableInputs">
+        <div class="alert alert-warning" ng-if="ctrl.addType === 'env' && ctrl.application && !ctrl.canAddRefToApplication">
+          <span class="pficon pficon-warning-triangle-o" aria-hidden="true"></span>
+          <strong>Containers of the selected application already contain this {{ctrl.apiObject.kind | humanizeKind}}.</strong>


Maybe just

<strong>The {{ctrl.apiObject.kind | humanizeKind}} has already been added to this application.</strong>

Suggest using the help-block / has-error style (instead of an alert) and moving it below the select.

jhadvig · 2017-10-16T09:50:07Z

@spadgett I've update the PR based on your comments :)
PTAL

spadgett · 2017-10-16T12:47:31Z

Looks good. A couple small comments:

There should be more margin between the error message and the "Add secret as:" label below.
The select should be highlighted red as well. If you have a has-error class on the form group, the entire select is highlighted. See https://github.com/openshift/origin-web-catalog/blob/master/src/components/select-project/selectProject.html#L2
We should probably use a consistent style for the "Some keys for..." warning as well. It looks odd to have two error styles. I'd suggest switching the env warning to the help-block / has-warning style.

jhadvig · 2017-10-16T16:41:53Z

@spadgett comments addressed

screen:

PTAL

spadgett · 2017-10-16T22:00:10Z

app/views/directives/add-config-to-application.html

@@ -108,7 +111,7 @@
                  class="btn btn-primary"
                  ng-class="{'dialog-btn': isDialog}"
                  ng-click="ctrl.addToApplication()"
-                  ng-disabled="ctrl.addType === 'volume' && addToApplicationForm.$invalid || !ctrl.application"
+                  ng-disabled="(ctrl.addType === 'volume' && addToApplicationForm.$invalid) || (ctrl.addType === 'env' && !ctrl.canAddRefToApplication)"


It looks to me like you might be able to submit the form now while "applications" is still loading? canAddRefToApplication is initialized to true and the invalid check is skipped.

I'm guessing we're only checking addToApplicationForm.$invalid to keep the form submittable when mount path is invalid, but addType === 'env'. It would be better to disable the mount path input, however, and just check

ng-disabled="addToApplicationForm.$invalid || (ctrl.addType === 'env' && !ctrl.canAddRefToApplication)"

cc @jeff-phillips-18

I think you still need to ensure an application is selected either way.

@jeff-phillips-18 I've update the mount path input with the check if the application was selected as @spadgett suggested. This will force user to pick the application in order to make the form valid. Added separate commit so the changes are obvious.
PTAL

spadgett · 2017-10-17T12:19:44Z

@jhadvig Looks like a dist mismatch

jhadvig · 2017-10-17T12:33:25Z

@spadgett will rebase and regenerate dist

…tion twice

spadgett · 2017-10-17T13:06:24Z

/lgtm

openshift-merge-robot · 2017-10-17T13:33:45Z

Automatic merge from submit-queue.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 11, 2017

benjaminapetersen reviewed Oct 11, 2017

View reviewed changes

jhadvig force-pushed the twice branch from 748b6a6 to c3e279c Compare October 12, 2017 13:26

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 12, 2017

spadgett requested changes Oct 13, 2017

View reviewed changes

jhadvig force-pushed the twice branch from c3e279c to d6ce337 Compare October 16, 2017 09:48

jhadvig force-pushed the twice branch from d6ce337 to cb67bfb Compare October 16, 2017 16:37

spadgett reviewed Oct 16, 2017

View reviewed changes

jeff-phillips-18 approved these changes Oct 17, 2017

View reviewed changes

spadgett approved these changes Oct 17, 2017

View reviewed changes

Should not be able to add the same secret or config map to an applica…

3d5ae02

…tion twice

jhadvig force-pushed the twice branch from 09714d8 to 3d5ae02 Compare October 17, 2017 12:35

openshift-ci-robot assigned spadgett Oct 17, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2017

openshift-merge-robot merged commit aeaf248 into openshift:master Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should not be able to add the same secret or config map to an application twice #2249

Should not be able to add the same secret or config map to an application twice #2249

jhadvig commented Oct 11, 2017

benjaminapetersen Oct 11, 2017

jhadvig Oct 11, 2017

spadgett Oct 11, 2017

jhadvig commented Oct 12, 2017

benjaminapetersen commented Oct 12, 2017

spadgett left a comment

spadgett Oct 13, 2017

spadgett Oct 13, 2017

spadgett Oct 13, 2017

spadgett Oct 13, 2017

spadgett Oct 13, 2017

spadgett Oct 13, 2017

jhadvig commented Oct 16, 2017

spadgett commented Oct 16, 2017

jhadvig commented Oct 16, 2017

spadgett Oct 16, 2017

jeff-phillips-18 Oct 17, 2017

jhadvig Oct 17, 2017

spadgett commented Oct 17, 2017

jhadvig commented Oct 17, 2017

spadgett commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017

Should not be able to add the same secret or config map to an application twice #2249

Should not be able to add the same secret or config map to an application twice #2249

Conversation

jhadvig commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig commented Oct 12, 2017

benjaminapetersen commented Oct 12, 2017

spadgett 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

jhadvig commented Oct 16, 2017

spadgett commented Oct 16, 2017

jhadvig commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Oct 17, 2017

jhadvig commented Oct 17, 2017

spadgett commented Oct 17, 2017

openshift-merge-robot commented Oct 17, 2017