add "overwrite" option to attachPVC view #2176

juanvallejo · 2017-09-26T23:12:37Z

Depends on: openshift/origin-web-common#203
Fixes #2045

Work in progress. Just wanted to begin gathering feedback.
Will post images.

juanvallejo · 2017-09-26T23:13:53Z

app/scripts/controllers/attachPVC.js

@@ -118,6 +118,51 @@ angular.module('openshiftConsole')
        $scope.$watchGroup(['attach.resource', 'attach.allContainers'], updateMountPaths);
        $scope.$watch('attach.containers', updateMountPaths, true);

+        var setVolumeMount = function(podTemplate, name, mountPath, subPath, readOnly) {
+          for (var i = 0; i < podTemplate.spec.containers.length; i++) {


Will switch to an angular.forEach.

We usually use Lodash _.each

https://lodash.com/docs/4.17.4#forEach

fwiw, var isn't block scoped (inside if statements), switching to _.each(function() { }) will fix. It wouldn't error, but its a bit sketch. yay js 😄

juanvallejo · 2017-09-26T23:14:47Z

app/scripts/controllers/attachPVC.js

+              containers[i].volumeMounts = [];
+            }
+
+            for (var idx = 0; idx < containers[i].volumeMounts.length; idx++) {


Will switch to angular.forEach

juanvallejo · 2017-09-26T23:14:58Z

app/scripts/controllers/attachPVC.js

+              }
+            }
+
+            for (var mountIdx = 0; mountIdx < containers[i].volumeMounts.length; mountIdx++) {


Will also switch to angular.forEach

spadgett · 2017-09-27T12:10:20Z

app/views/attach-pvc.html

@@ -211,7 +225,7 @@ <h2 class="text-center">No persistent volume claims.</h2>
                  <button type="submit"
                      class="btn btn-primary btn-lg"
                      ng-click="attachPVC()"
-                      ng-disabled="attachPVCForm.$invalid || disableInputs || !attachPVC">Add</button>
+                      ng-disabled="(attachPVCForm.$invalid) || disableInputs || !attachPVC">Add</button>


nit: shouldn't need the parens

spadgett · 2017-09-27T12:11:53Z

app/scripts/controllers/attachPVC.js

+                volumeExists = true;
+                break;
+              }
+            }


You should use _.some to make this a one-liner:

var volumeExists = _.some(podTemplate.spec.volumes, { name: newVolume.name });

https://lodash.com/docs/4.17.4#some

benjaminapetersen · 2017-09-27T14:10:47Z

app/views/attach-pvc.html

@@ -83,6 +83,7 @@ <h2 class="text-center">No persistent volume claims.</h2>
                      ng-model="attach.mountPath"
                      ng-pattern="/^\/.*$/"
                      osc-unique="existingMountPaths"
+                      osc-unique-disabled="attach.overwrite"


juanvallejo · 2017-09-27T20:52:58Z

@spadgett thanks for the feedback, review comments addressed

benjaminapetersen · 2017-09-29T14:40:21Z

Verifying that checked in built files under dist match the source...
Built /dist does not match what is committed, run 'grunt build' and include the results in your commit.
diff --git a/dist/scripts/vendor.js b/dist/scripts/vendor.js

prob just a rebase & a bower install & a grunt build

juanvallejo · 2017-10-02T16:53:18Z

@benjaminapetersen thanks, PR re-built and rebased

juanvallejo · 2017-10-03T14:15:16Z

re[test]

benjaminapetersen · 2017-10-03T15:04:33Z

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

😄
try:

hack/clean-deps.sh
hack/install-deps.sh
grunt build 
// commit this stuff

juanvallejo · 2017-10-03T17:12:12Z

@benjaminapetersen thanks! that seems to have done the trick

benjaminapetersen · 2017-10-03T18:28:52Z

Yup, now you just have the same e2e test errors that all of our other PRs are struggling with. Woohoo!

benjaminapetersen · 2017-10-05T15:46:01Z

[test] since Jenkins file changed

juanvallejo · 2017-10-06T18:09:27Z

@spadgett friendly ping

juanvallejo · 2017-10-09T16:10:00Z

@spadgett per our conversation, went ahead and broke setVolumeMount into smaller functions f4f32b0

spadgett · 2017-10-09T19:54:27Z

app/scripts/controllers/attachPVC.js

+              success = false;
+              return success;
+            }
+          });


I think this reads a little better and avoids needing a success flag.

var duplicateMount = _.find(container.volumeMounts, function(mount) { return mount.mountPath === newVolumeMount.mountPath && mount.name !== newVolumeMount.name; }); if (duplicateMount) { displayError('The volume mount "' + duplicateMount.mountPath + '" with name "' + duplicateMount.name +'" already exists for container "' + container.name + '"'); return false; } return true;

spadgett · 2017-10-09T19:56:54Z

app/scripts/controllers/attachPVC.js

+            }
+          });
+
+          return didReplace;


var index = _.findIndex(container.volumeMounts, { name: newVolumeMount.name }); if (index === -1) { return false; } container.volumeMounts[index] = newVolumeMount; return true;

spadgett · 2017-10-09T20:00:29Z

app/views/attach-pvc.html

+                      Overwrite
+                    </label>
+                    <div id="overwrite-help" class="help-block">
+                      Overwrite the volume mount config (if it already exists).


I'd just say "Overwrite a volume mount if it already exists."

spadgett · 2017-10-09T20:04:52Z

app/scripts/controllers/attachPVC.js

+
+            if (!$scope.attach.overwrite || ($scope.attach.overwrite && !volumeExists)) {
+              podTemplate.spec.volumes.push(newVolume);
+            }


Is it correct to continue if the volume exists and overwrite is not set?

Is it correct to continue if the volume exists and overwrite is not set?

Ah, no pushing a volume containing a duplicate name field would cause an error, however I think I was relying on the client-side checks that we currently have to stop a user from submitting the form if a duplicate name or mount path was found.

Will go ahead and update this to just be

if (!volumeExists) { podTemplate.spec.volumes.push(newVolume); }

spadgett · 2017-10-09T20:05:07Z

app/scripts/controllers/attachPVC.js

-                  container.volumeMounts.push(newVolumeMount);
-                }
-              });
+              // angular.forEach(podTemplate.spec.containers, function(container) {


Remove commented out code

juanvallejo · 2017-10-10T17:03:18Z

@spadgett thanks for the feedback, review comments addressed

openshift-bot · 2017-10-10T17:07:17Z

Evaluated for origin web console test up to 9fe84ee

openshift-bot · 2017-10-10T17:28:59Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/327/) (Base Commit: 45d17ff) (PR Branch Commit: 9fe84ee)

spadgett · 2017-10-13T13:37:16Z

/test

spadgett · 2017-10-13T13:44:28Z

/lgtm

spadgett · 2017-10-13T14:27:39Z

/retest

openshift-merge-robot · 2017-10-13T15:14:02Z

Automatic merge from submit-queue.

…volume-overwrite-option Automatic merge from submit-queue. Revert "add "overwrite" option to attachPVC view" Reverts #2176 The change has introduced some bugs. Reverting for now. I've reopened the original issue.

juanvallejo mentioned this pull request Sep 26, 2017

add ability to "disable" form validity openshift/origin-web-common#203

Merged

juanvallejo commented Sep 26, 2017

View reviewed changes

spadgett reviewed Sep 27, 2017

View reviewed changes

benjaminapetersen reviewed Sep 27, 2017

View reviewed changes

juanvallejo force-pushed the jvallejo/add-set-volume-overwrite-option branch from 2baea1f to d6cd77c Compare September 27, 2017 21:29

juanvallejo force-pushed the jvallejo/add-set-volume-overwrite-option branch 2 times, most recently from 8e44b7f to afc011f Compare October 2, 2017 16:52

juanvallejo changed the title ~~[WIP] add "overwrite" option to attachPVC view~~ add "overwrite" option to attachPVC view Oct 2, 2017

juanvallejo force-pushed the jvallejo/add-set-volume-overwrite-option branch from afc011f to f118eb1 Compare October 3, 2017 15:42

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 9, 2017

juanvallejo force-pushed the jvallejo/add-set-volume-overwrite-option branch from f4f32b0 to bb9a408 Compare October 9, 2017 18:16

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 9, 2017

juanvallejo force-pushed the jvallejo/add-set-volume-overwrite-option branch from bb9a408 to 9d5d868 Compare October 9, 2017 18:50

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 9, 2017

spadgett reviewed Oct 9, 2017

View reviewed changes

add "overwrite" option to attachPVC view

9fe84ee

juanvallejo force-pushed the jvallejo/add-set-volume-overwrite-option branch from 9d5d868 to 9fe84ee Compare October 10, 2017 17:03

openshift-ci-robot assigned spadgett Oct 13, 2017

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

openshift-merge-robot merged commit ce05a60 into openshift:master Oct 13, 2017

juanvallejo deleted the jvallejo/add-set-volume-overwrite-option branch October 13, 2017 15:31

spadgett mentioned this pull request Oct 13, 2017

Revert "add "overwrite" option to attachPVC view" #2263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "overwrite" option to attachPVC view #2176

add "overwrite" option to attachPVC view #2176

juanvallejo commented Sep 26, 2017

juanvallejo Sep 26, 2017

spadgett Sep 27, 2017

benjaminapetersen Sep 27, 2017

juanvallejo Sep 26, 2017

juanvallejo Sep 26, 2017

spadgett Sep 27, 2017

spadgett Sep 27, 2017

benjaminapetersen Sep 27, 2017 •

edited

Loading

juanvallejo commented Sep 27, 2017

benjaminapetersen commented Sep 29, 2017 •

edited

Loading

juanvallejo commented Oct 2, 2017

juanvallejo commented Oct 3, 2017

benjaminapetersen commented Oct 3, 2017

juanvallejo commented Oct 3, 2017

benjaminapetersen commented Oct 3, 2017

benjaminapetersen commented Oct 5, 2017

juanvallejo commented Oct 6, 2017

juanvallejo commented Oct 9, 2017

spadgett Oct 9, 2017

spadgett Oct 9, 2017

spadgett Oct 9, 2017

spadgett Oct 9, 2017

juanvallejo Oct 10, 2017

spadgett Oct 9, 2017

juanvallejo commented Oct 10, 2017

openshift-bot commented Oct 10, 2017

openshift-bot commented Oct 10, 2017

spadgett commented Oct 13, 2017

spadgett commented Oct 13, 2017

spadgett commented Oct 13, 2017

openshift-merge-robot commented Oct 13, 2017

add "overwrite" option to attachPVC view #2176

add "overwrite" option to attachPVC view #2176

Conversation

juanvallejo 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

Choose a reason for hiding this comment

benjaminapetersen Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

juanvallejo commented Sep 27, 2017

benjaminapetersen commented Sep 29, 2017 • edited Loading

juanvallejo commented Oct 2, 2017

juanvallejo commented Oct 3, 2017

benjaminapetersen commented Oct 3, 2017

juanvallejo commented Oct 3, 2017

benjaminapetersen commented Oct 3, 2017

benjaminapetersen commented Oct 5, 2017

juanvallejo commented Oct 6, 2017

juanvallejo commented Oct 9, 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

juanvallejo commented Oct 10, 2017

openshift-bot commented Oct 10, 2017

openshift-bot commented Oct 10, 2017

spadgett commented Oct 13, 2017

spadgett commented Oct 13, 2017

spadgett commented Oct 13, 2017

openshift-merge-robot commented Oct 13, 2017

benjaminapetersen Sep 27, 2017 •

edited

Loading

benjaminapetersen commented Sep 29, 2017 •

edited

Loading