Add svcat unbind --abandon command #2579

taragu · 2019-03-10T16:57:43Z

This PR is a

Feature Implementation
Bug Fix
Documentation

What this PR does / why we need it: Add --abandon and --yes flags for svcat unbind

Which issue(s) this PR fixes

#2268

Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

Merge Checklist:

New feature
- Tests
- Documentation
SVCat CLI flag

taragu · 2019-03-10T16:58:33Z

/assign @carolynvs
/cc @jberkhahn

jberkhahn · 2019-03-11T17:50:07Z

cmd/svcat/binding/unbind_cmd.go

@@ -35,6 +41,9 @@ type unbindCmd struct {

 	instanceName string
 	bindingNames []string
+	abandon      bool
+	stdin        io.Reader


I don't understand why this is defined as a field on the command struct. Does COBRA do something special to populate this for you? where is it getting instantiated?

jberkhahn · 2019-03-11T17:55:33Z

cmd/svcat/binding/unbind_cmd.go

+		if err != nil {
+			return err
+		}
+		for _, binding := range instanceBindings {


I don't think it's a good idea to reach in and do this sort of stuff in the command layer. I think I would add some sort of RemoveFinalizers() method to the interface in pkg/svcat/service-catalog, and implement this there, like the rest of the methods like RetrieveBindingsByInstance or Unbind

jberkhahn · 2019-03-11T17:58:03Z

cmd/svcat/binding/unbind_cmd.go

+			}
+		}
+
+		if c.instanceName == "" {


It's actually possible for there to be multiple bindings to remove, see the bit on line 145 where it calls Unbind or DeleteBindings based on whether instanceName is set or not. I think we also need to handle both cases.

jberkhahn · 2019-03-11T18:04:57Z

In addition to the comments on the code, I also think your commit message could be a bit more descriptive. "Add --abandon flag to svcat unbind command" or something.

taragu · 2019-03-16T13:26:39Z

@jberkhahn thanks for the feedback! I've updated the PR and it's ready to be reviewed again

taragu · 2019-03-16T16:38:30Z

/test pull-service-catalog-integration

jberkhahn · 2019-03-18T21:21:36Z

cmd/svcat/binding/unbind_cmd.go

+	if c.abandon {
+		fmt.Fprintln(c.Output, "This action is not reversible and may cause you to be charged for the broker resources that are abandoned.")
+		if !c.skipPrompt {
+			fmt.Fprintln(c.Output, "Are you sure? [y|N]: ")


shouldn't these both be upper or lower case?

jberkhahn · 2019-03-18T21:30:07Z

cmd/svcat/binding/unbind_cmd.go

+			si := &v1beta1.ServiceInstance{ObjectMeta: metav1.ObjectMeta{Name: c.instanceName, Namespace: c.Namespace}}
+			removedBindings, err = c.App.RemoveFinalizersByInstance(si)
+		} else {
+			retrievedBindings, err := c.App.RetrieveBindings(c.Namespace)


This will remove the finalizers on all the bindings in the whole namespace, which I don't think is what we want. I think you could replace this with a call to getBindingsToDelete(), like the part that actually delete the bindings does. Then you would have to either turn that array of names into an array of v1beta1 binding objects, or refactor the pkg methods to take names/namespaces instead of v1beta1.bindings. I think I would do the second, but you could make the case for either.

jberkhahn · 2019-03-18T21:35:40Z

pkg/svcat/service-catalog/binding_test.go

+			actions := svcCatClient.Actions()
+			Expect(actions[0].Matches("update", "servicebindings")).To(BeTrue())
+			Expect(actions[0].(testing.UpdateActionImpl).Object.(*v1beta1.ServiceBinding).ObjectMeta.Name).To(Equal(sb.ObjectMeta.Name))
+			Expect(actions[0].(testing.UpdateActionImpl).Object.(*v1beta1.ServiceBinding).ObjectMeta.Namespace).To(Equal(sb.ObjectMeta.Namespace))


These tests are great! Only other thing I could ask for us somehow asserting that the update action is removing the finalizers, but I don't actually know how to do that.

jberkhahn · 2019-03-18T21:36:38Z

pkg/svcat/service-catalog/sdk.go

@@ -81,6 +81,10 @@ type SvcatClient interface {
 	RetrieveSecretByBinding(*apiv1beta1.ServiceBinding) (*apicorev1.Secret, error)

 	ServerVersion() (*version.Info, error)
+
+	RemoveFinalizersByInstance(*apiv1beta1.ServiceInstance) ([]types.NamespacedName, error)


these should be in the block at the top with the other methods for bindings

jberkhahn · 2019-03-25T23:53:00Z

cmd/svcat/binding/unbind_cmd.go

+			return err
+		}
+		for _, binding := range removedBindings {
+			fmt.Fprintln(c.Output, fmt.Sprintf("deleted %s", binding.Name))


its possible for both these to print, but the removing finalizers is an if/else, so shouldn't this be as well?

actually, looking further, neither of these print statements is needed, we say "deleted foobar" down below

jberkhahn · 2019-03-25T23:57:51Z

cmd/svcat/binding/unbind_cmd.go

+		if c.instanceName != "" {
+			fmt.Fprintln(c.Output, fmt.Sprintf("deleted %s", c.instanceName))
+		}
+		return nil


this return statement needs to be removed - otherwise we just remove the finalizers and don't actually delete the binding

jberkhahn · 2019-03-25T23:58:19Z

@taragu just a few more small changes, and then this LGTM

taragu · 2019-03-29T12:54:16Z

/test pull-service-catalog-xbuild

jberkhahn · 2019-03-29T18:16:56Z

/lgtm

jboyd01

The code is right on, lgtm. Some comments and function names are misleading (all finalizers) and in one case, it sounds like removing the finalizer from an instance (RemoveFinalizersByInstance)

jboyd01 · 2019-03-29T18:19:01Z

pkg/svcat/service-catalog/binding.go

@@ -264,3 +265,69 @@ func (sdk *SDK) bindingHasStatus(binding *v1beta1.ServiceBinding, status v1beta1

 	return false
 }
+
+// RemoveFinalizersForBinding removes all finalizers for a single binding


nit: RemoveFinalizerForBinding (singular finalizer)
here and other places: "all finalizers" but we're actually only deleting the v1beta1.FinalizerServiceCatalog finalizer (not all finalizers). Code is correct.

jboyd01 · 2019-03-29T18:20:50Z

pkg/svcat/service-catalog/binding.go

+	return deleted, bindErr.ErrorOrNil()
+}
+
+// RemoveFinalizersByInstance removes all finalizers for an instance.


Remove v1beta1.FinalizerServiceCatalog from all bindings for the specified instance

jboyd01 · 2019-03-29T18:25:59Z

pkg/svcat/service-catalog/binding.go

+}
+
+// RemoveFinalizersByInstance removes all finalizers for an instance.
+func (sdk *SDK) RemoveFinalizersByInstance(instance *v1beta1.ServiceInstance) ([]types.NamespacedName, error) {


nit: I'd prefer to call it RemoveBindingFinalizerByInstance
(its the only the service catalog finalizer for all bindings that are for a specific instance)

jboyd01 · 2019-03-29T18:26:26Z

pkg/svcat/service-catalog/binding.go

+}
+
+// RemoveFinalizersForBindings removes all finalizers for the provided list of bindings
+func (sdk *SDK) RemoveFinalizersForBindings(bindings []types.NamespacedName) ([]types.NamespacedName, error) {


nit: RemoveFinalizerForBindings (singular on the finalizer)

jboyd01 · 2019-03-29T18:36:19Z

@taragu - I neglected to say thanks for the PR!! Great job on what I believe to be your first SIG Catalog contribution!

jberkhahn · 2019-03-29T20:08:22Z

/lgtm

jboyd01 · 2019-03-29T20:10:35Z

Thanks @taragu
/approve

k8s-ci-robot · 2019-03-29T20:10:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jboyd01

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/svcat/OWNERS~~ [jboyd01]
~~pkg/svcat/OWNERS~~ [jboyd01]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

taragu · 2019-03-29T20:26:35Z

Thanks @jberkhahn @jboyd01 !

jberkhahn · 2019-03-29T20:34:35Z

/test pull-service-catalog-xbuild

cforce · 2019-04-13T22:13:04Z

Still can not remove abandon bindings.

C:\Users\user\Downloads>svcat get bindings -ndev
NAME NAMESPACE INSTANCE STATUS
+------------------------------+-----------+------------------------------+-------------------------------+
push-adapter-db.dev.db dev push-adapter-db.dev.db ReferencesNonexistentInstance

C:\Users\user\Downloads>svcatc unbind push-adapter-db.dev.db -ndev --abandon
This action is not reversible and may cause you to be charged for the broker resources that are abandoned.
Are you sure? [y|n]:
y

unable to get instance 'dev.push-adapter-db.dev.db' (serviceinstances.servicecatalog.k8s.io "push-adapter-db.dev.db" not found)
Error: could not remove all bindings

C:\Users\user\Downloads>svcatc unbind push-adapter-db.dev.db -ndev --abandon --yes
This action is not reversible and may cause you to be charged for the broker resources that are abandoned.
unable to get instance 'dev.push-adapter-db.dev.db' (serviceinstances.servicecatalog.k8s.io "push-adapter-db.dev.db" not found)
Error: could not remove all bindings

taragu · 2019-04-13T23:24:05Z

@cforce thank you for reporting this problem. I created #2610 to track this.

* Add --abandon flag to svcat unbind command; add remove finalizer methods * Add tests for svcat remove finalizer methods * Update yaml file for svcat unbind --abandon command * Generate fake client methods for remove finalizer methods

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 10, 2019

k8s-ci-robot requested review from eriknelson and kikisdeliveryservice March 10, 2019 16:57

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 10, 2019

k8s-ci-robot assigned carolynvs Mar 10, 2019

k8s-ci-robot requested a review from jberkhahn March 10, 2019 16:58

taragu force-pushed the svcat-abandon branch 3 times, most recently from 9db404a to 61ec955 Compare March 10, 2019 23:27

jberkhahn reviewed Mar 11, 2019

View reviewed changes

taragu force-pushed the svcat-abandon branch from 61ec955 to dae51cc Compare March 16, 2019 13:19

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2019

taragu changed the title ~~Unbind abandon~~ Add svcat unbind --abandon command Mar 16, 2019

taragu force-pushed the svcat-abandon branch from dae51cc to b36be04 Compare March 16, 2019 13:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 16, 2019

taragu force-pushed the svcat-abandon branch 2 times, most recently from 1ed102d to f896543 Compare March 16, 2019 14:33

jberkhahn reviewed Mar 18, 2019

View reviewed changes

carolynvs removed their assignment Mar 19, 2019

jberkhahn reviewed Mar 25, 2019

View reviewed changes

taragu force-pushed the svcat-abandon branch from f7224e6 to f339702 Compare March 29, 2019 02:46

k8s-ci-robot assigned jberkhahn Mar 29, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2019

jboyd01 reviewed Mar 29, 2019

View reviewed changes

Tara Gu added 3 commits March 29, 2019 15:13

Add --abandon flag to svcat unbind command; add remove finalizer methods

b5b1740

Update yaml file for svcat unbind --abandon command

3b77228

Add tests for svcat remove finalizer methods

15f1a98

taragu force-pushed the svcat-abandon branch from f339702 to dd3cf78 Compare March 29, 2019 19:14

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2019

Generate fake client methods for remove finalizer methods

5cfb626

taragu force-pushed the svcat-abandon branch from dd3cf78 to 5cfb626 Compare March 29, 2019 19:48

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2019

k8s-ci-robot merged commit 05181ac into kubernetes-retired:master Mar 29, 2019

taragu deleted the svcat-abandon branch March 30, 2019 20:30

cforce mentioned this pull request Apr 8, 2019

purge DeprovisionCallFailed entries from database Azure/open-service-broker-azure#682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add svcat unbind --abandon command #2579

Add svcat unbind --abandon command #2579

taragu commented Mar 10, 2019 •

edited

Loading

taragu commented Mar 10, 2019

jberkhahn Mar 11, 2019

jberkhahn Mar 11, 2019

jberkhahn Mar 11, 2019

jberkhahn commented Mar 11, 2019

taragu commented Mar 16, 2019

taragu commented Mar 16, 2019

jberkhahn Mar 18, 2019

jberkhahn Mar 18, 2019

jberkhahn Mar 18, 2019

jberkhahn Mar 18, 2019

jberkhahn Mar 25, 2019

jberkhahn Mar 25, 2019 •

edited

Loading

jberkhahn Mar 25, 2019

jberkhahn commented Mar 25, 2019

taragu commented Mar 29, 2019

jberkhahn commented Mar 29, 2019

jboyd01 left a comment

jboyd01 Mar 29, 2019

jboyd01 Mar 29, 2019

jboyd01 Mar 29, 2019 •

edited by jberkhahn

Loading

jboyd01 Mar 29, 2019

jboyd01 commented Mar 29, 2019

jberkhahn commented Mar 29, 2019

jboyd01 commented Mar 29, 2019

k8s-ci-robot commented Mar 29, 2019

taragu commented Mar 29, 2019

jberkhahn commented Mar 29, 2019

cforce commented Apr 13, 2019

taragu commented Apr 13, 2019 •

edited

Loading

Add svcat unbind --abandon command #2579

Add svcat unbind --abandon command #2579

Conversation

taragu commented Mar 10, 2019 • edited Loading

taragu commented Mar 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Mar 11, 2019

taragu commented Mar 16, 2019

taragu commented Mar 16, 2019

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

jberkhahn Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jberkhahn commented Mar 25, 2019

taragu commented Mar 29, 2019

jberkhahn commented Mar 29, 2019

jboyd01 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jboyd01 Mar 29, 2019 • edited by jberkhahn Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jboyd01 commented Mar 29, 2019

jberkhahn commented Mar 29, 2019

jboyd01 commented Mar 29, 2019

k8s-ci-robot commented Mar 29, 2019

taragu commented Mar 29, 2019

jberkhahn commented Mar 29, 2019

cforce commented Apr 13, 2019

Still can not remove abandon bindings.

taragu commented Apr 13, 2019 • edited Loading

taragu commented Mar 10, 2019 •

edited

Loading

jberkhahn Mar 25, 2019 •

edited

Loading

jboyd01 Mar 29, 2019 •

edited by jberkhahn

Loading

taragu commented Apr 13, 2019 •

edited

Loading