Add delete binding (unbind) action to provisioned services #1505

benjaminapetersen · 2017-05-03T17:24:44Z

Depends on #1578

WIP, still some things to do. Based on mock-ups from here. Progress thus far:

TODO:

<service> has been unbound from <application>. Application name is missing.
factor bindService and unbindService duplicate code into separate service, OR collapse unbindService into bindService & toggle.

benjaminapetersen · 2017-05-03T20:28:22Z

app/scripts/directives/bindService.js

@@ -16,10 +17,16 @@ angular.module('openshiftConsole').component('bindService', {
 });

 function BindService($filter,
+                     BindingModalUtils,
                     DataService,
                     DNS1123_SUBDOMAIN_VALIDATION) {
  var ctrl = this;



@jwforres thoughts here, pulled some things from bindService to share w/both components.

spadgett · 2017-05-03T20:30:47Z

app/scripts/services/bindingModalUtils.js

+'use strict';
+
+angular.module('openshiftConsole')
+  .factory('BindingModalUtils', function() {


@benjaminapetersen We're planning to switch to the angular-patternfly pf-wizard component, which should handle most of this for us.

Sounds good, keep the service till then so don't have to duplicate code?

Going to probably push up sortApplications as well, as I need that for unbindService to be able to get the name of the associated app.

spadgett · 2017-05-04T17:10:21Z

I feel like this should be either create binding / delete binding OR add binding / remove binding. Create / remove seems wrong.

@ncameronbritt @jwforres Opinion?

benjaminapetersen · 2017-05-04T18:54:15Z

I tend toward create/delete, it implies you are manipulating a type of resource.

ncameronbritt · 2017-05-05T14:29:39Z

I agree with @spadgett about choosing the right pair of verbs. I think I like create/delete better as well, but I also want to make sure that we're choosing words that reflect how we want users to understand what they're doing. Ignoring what happens technically, is there a metaphor we would use to describe the concept?

benjaminapetersen · 2017-05-09T15:41:01Z

benjaminapetersen · 2017-05-30T15:54:24Z

btw, rebased

benjaminapetersen · 2017-06-06T19:34:58Z

[TEST] rebased / resolved conflicts

benjaminapetersen · 2017-06-06T19:41:51Z

ah, meh, another rebase todo.

benjaminapetersen · 2017-06-06T19:59:36Z

@jwforres rebased

ServiceInstancesService... ftw?

benjaminapetersen · 2017-06-07T19:17:01Z

Will rebase again after #1599

jwforres · 2017-06-06T14:36:39Z

app/scripts/services/bindingModalUtils.js

+                                              .concat(statefulSets);
+            return _.sortByAll(apiObjects, ['metadata.name', 'kind']);
+          }
+        }


I would prefer to see an explicit return null here, that makes it clear to someone scanning the function that there is a case where the utility will return a null instead of an array

I don't think _.sortByAll will ever return a null. Even _.sortByAll(null, ['metadata.name', 'kind']) will return [ ].

Ahh... my bad. Got it. Will do.

jwforres · 2017-06-06T14:37:36Z

app/scripts/directives/unbindService.js

+      group: 'servicecatalog.k8s.io',
+      resource: 'bindings'
+    }, ctrl.selectedBinding, context).then(function(result) {
+      console.log('did we succeed?', result);


remove these two console.log stmts

jwforres · 2017-06-06T14:43:23Z

app/scripts/directives/unbindService.js

+  };
+
+  ctrl.$onInit = function() {
+    // loadApps();


I know its not implemented yet, but you will only want to loadApps if the target of unbind is a service instance object, should update your comment here with a TODO that is clear.

jwforres · 2017-06-12T17:41:28Z

app/views/directives/bind-service/remove-binding-confirm.html

+    </p>
+  </div>
+  <div ng-if="ctrl.error">
+    <div class="title">Removal of Binding Failed <span class="fa fa-times text-danger"></span></div>


if we called the action Delete Binding, then it shouldnt say Removal here

jwforres · 2017-06-12T17:41:40Z

app/views/directives/bind-service/remove-binding-confirm.html

+        {{ctrl.error.data.message | upperFirst}}
+      </span>
+      <span ng-if="!ctrl.error.data.message">
+        An error occurred removing the binding.


same here, delete not remove

jwforres · 2017-06-12T17:42:08Z

app/views/directives/bind-service/select-binding.html

@@ -0,0 +1,13 @@
+<h3 class="mar-top-none">
+  Select a service to unbind from <strong>{{ctrl.displayName}}</strong>


we agreed on friday we didnt want to use the term "unbind"

jwforres · 2017-06-13T15:51:48Z

no i mean the fact that you have an if check here, in the case where the if doesn't pass the function returns nothing, effectively an undefined result wherever it was called from

…

On Tue, Jun 13, 2017 at 11:47 AM, Ben Petersen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/scripts/services/bindingModalUtils.js <#1505 (comment)> : > + function() { + return { + sortApplications: function(deploymentConfigs, + deployments, + replicationControllers, + replicaSets, + statefulSets) { + // Don't waste time sorting on each data load, just sort when we have them all + if (deploymentConfigs && deployments && replicationControllers && replicaSets && statefulSets) { + var apiObjects = deploymentConfigs.concat(deployments) + .concat(replicationControllers) + .concat(replicaSets) + .concat(statefulSets); + return _.sortByAll(apiObjects, ['metadata.name', 'kind']); + } + } I don't think _.sortByAll will ever return a null. Even _.sortByAll(null, ['metadata.name', 'kind']) will return [ ]. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1505 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7QrLt1gjvJe-vXGM5kkp4fhj94laks5sDq8jgaJpZM4NPtX5> .

openshift-bot · 2017-06-13T20:07:25Z

Evaluated for origin web console test up to 3fe6155

openshift-bot · 2017-06-13T20:42:09Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1517/) (Base Commit: 54262a2)

openshift-bot · 2017-06-14T01:31:08Z

Origin Web Console Action Required: Pull request cannot be automatically merged, please rebase your branch from latest HEAD and push again

benjaminapetersen · 2017-06-19T13:47:02Z

Fixed by PR #1705

benjaminapetersen force-pushed the unbind-service branch from de7650a to 2b2fa4a Compare May 3, 2017 17:25

benjaminapetersen commented May 3, 2017

View reviewed changes

spadgett reviewed May 3, 2017

View reviewed changes

benjaminapetersen force-pushed the unbind-service branch from 442a118 to 9cab1ec Compare May 3, 2017 20:30

benjaminapetersen force-pushed the unbind-service branch 2 times, most recently from 92f24b9 to c9c49e3 Compare May 4, 2017 18:51

benjaminapetersen mentioned this pull request May 4, 2017

[WIP] Use common components for bind pages. #1514

Closed

benjaminapetersen force-pushed the unbind-service branch from c9c49e3 to 0d47703 Compare May 4, 2017 18:53

benjaminapetersen force-pushed the unbind-service branch 2 times, most recently from de0ef50 to a74e2ca Compare May 4, 2017 20:00

benjaminapetersen force-pushed the unbind-service branch from a74e2ca to 5e63120 Compare May 9, 2017 15:40

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

benjaminapetersen force-pushed the unbind-service branch 4 times, most recently from 947c04c to 20f9c25 Compare May 30, 2017 15:54

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

benjaminapetersen force-pushed the unbind-service branch from 20f9c25 to 8ce21f9 Compare May 30, 2017 18:07

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

benjaminapetersen force-pushed the unbind-service branch from 8ce21f9 to 8f5b82c Compare June 6, 2017 19:07

benjaminapetersen changed the title ~~[WIP] Add unbind action to provisioned services~~ Add unbind action to provisioned services Jun 6, 2017

benjaminapetersen force-pushed the unbind-service branch from 8f5b82c to 56c2952 Compare June 6, 2017 19:33

benjaminapetersen force-pushed the unbind-service branch from 56c2952 to 2b32e1f Compare June 6, 2017 19:46

benjaminapetersen force-pushed the unbind-service branch from 2b32e1f to ab2d17e Compare June 6, 2017 19:46

benjaminapetersen force-pushed the unbind-service branch 2 times, most recently from 3a9b70a to 05e515e Compare June 6, 2017 20:23

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

benjaminapetersen changed the title ~~Add unbind action to provisioned services~~ Add delete binding (unbind) action to provisioned services Jun 9, 2017

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

jwforres requested changes Jun 12, 2017

View reviewed changes

benjaminapetersen force-pushed the unbind-service branch 3 times, most recently from a486461 to f35afb3 Compare June 13, 2017 16:11

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

Add unbind action to provisioned services

3fe6155

benjaminapetersen force-pushed the unbind-service branch from f35afb3 to 3fe6155 Compare June 13, 2017 19:56

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

benjaminapetersen mentioned this pull request Jun 14, 2017

Add delete binding action to provisioned services #1705

Merged

benjaminapetersen closed this Jun 19, 2017

benjaminapetersen deleted the unbind-service branch June 19, 2017 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add delete binding (unbind) action to provisioned services #1505

Add delete binding (unbind) action to provisioned services #1505

benjaminapetersen commented May 3, 2017 •

edited

Loading

benjaminapetersen May 3, 2017

spadgett May 3, 2017

benjaminapetersen May 3, 2017

benjaminapetersen May 3, 2017

spadgett commented May 4, 2017

benjaminapetersen commented May 4, 2017 •

edited

Loading

ncameronbritt commented May 5, 2017

benjaminapetersen commented May 9, 2017

benjaminapetersen commented May 30, 2017

benjaminapetersen commented Jun 6, 2017 •

edited

Loading

benjaminapetersen commented Jun 6, 2017

benjaminapetersen commented Jun 6, 2017

benjaminapetersen commented Jun 7, 2017

jwforres Jun 6, 2017

benjaminapetersen Jun 13, 2017

benjaminapetersen Jun 13, 2017

jwforres Jun 6, 2017

jwforres Jun 6, 2017

jwforres Jun 12, 2017

jwforres Jun 12, 2017

jwforres Jun 12, 2017

jwforres commented Jun 13, 2017 via email

openshift-bot commented Jun 13, 2017

openshift-bot commented Jun 13, 2017

openshift-bot commented Jun 14, 2017

benjaminapetersen commented Jun 19, 2017

		@@ -0,0 +1,13 @@
		<h3 class="mar-top-none">
		Select a service to unbind from <strong>{{ctrl.displayName}}</strong>

Add delete binding (unbind) action to provisioned services #1505

Add delete binding (unbind) action to provisioned services #1505

Conversation

benjaminapetersen commented May 3, 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

spadgett commented May 4, 2017

benjaminapetersen commented May 4, 2017 • edited Loading

ncameronbritt commented May 5, 2017

benjaminapetersen commented May 9, 2017

benjaminapetersen commented May 30, 2017

benjaminapetersen commented Jun 6, 2017 • edited Loading

benjaminapetersen commented Jun 6, 2017

benjaminapetersen commented Jun 6, 2017

benjaminapetersen commented Jun 7, 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

Choose a reason for hiding this comment

jwforres commented Jun 13, 2017 via email

openshift-bot commented Jun 13, 2017

openshift-bot commented Jun 13, 2017

openshift-bot commented Jun 14, 2017

benjaminapetersen commented Jun 19, 2017

benjaminapetersen commented May 3, 2017 •

edited

Loading

benjaminapetersen commented May 4, 2017 •

edited

Loading

benjaminapetersen commented Jun 6, 2017 •

edited

Loading