Deleting a component also unlinks it from components it might be linked to

[dharmit]

What kind of PR is this?

/kind bug

What does does this PR do / why we need it:
This PR unlinks a component requested for deletion from all the components it might be linked to in the OpenShift cluster

Which issue(s) this PR fixes:

Fixes #2355

How to test changes / Special notes to the reviewer:

1. odo create nodejs node
2. odo create java java
3. From component node's context directory do odo link java --port 8080
4. From component java's context directory do odo delete -f
5. From component node's context directory do odo delete -f

[kadel]

/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

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:

• OWNERS [kadel]

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

[dharmit]

@amitkrout @kadel do you think I should add integration tests here? The scenario I can think of is same as what I've described in the "How to test changes" section of this PR.

@amitkrout if yes, which file do you recommend I add it to?

[kadel]

@amitkrout @kadel do you think I should add integration tests here? The scenario I can think of is same as what I've described in the "How to test changes" section of this PR.

yes that would be great!

@amitkrout if yes, which file do you recommend I add it to?

I would categorize it as link test.

[dharmit]

@mik-dass Thanks for the tip! It makes sense. But I'll need some help. Can you see if you can help me?

Here's the git diff:

$ git diff
diff --git a/pkg/odo/cli/component/delete.go b/pkg/odo/cli/component/delete.go
index 932df0f3e..9a0fdadae 100644
--- a/pkg/odo/cli/component/delete.go
+++ b/pkg/odo/cli/component/delete.go
@@ -99,13 +99,18 @@ func (do *DeleteOptions) Run() (err error) {
                        componentSecrets := component.UnlinkComponents(parentComponent, compoList)
 
                        for component, secret := range componentSecrets {
+                               spinner := log.NewStatus(log.GetStdout())
+                               defer spinner.End(true)
                                for _, secretName := range secret {
+                                       spinner.Start(fmt.Sprintf("Unlinking component %q from component %q for secret %q\n", parentComponent.Name, component, secretName), false)
                                        err = do.Client.UnlinkSecret(secretName, component, do.Context.Application)
                                        if err != nil {
+                                               log.Errorf("Unlinking failed")
                                                return err
                                        }
-                                       log.Successf("Unlinked component %q from component %q for secret %q", parentComponent.Name, component, secretName)
+                                       log.Successf("Unlink successful")
                                }
+                               spinner.End(true)
                        }
                        err = component.Delete(do.Client, do.componentName, do.Application)
                        if err != nil {

And here's the output which is clearly messed up!

 odo delete -f
 ◐  Unlinking component "backend" from component "frontend" for secret "backend-app-8778"
 ◓  Unlinking component "backend" from component "frontend" for secret "backend-app-8778"
 ◑  Unlinking component "backend" from component "frontend" for secret "backend-app-8778"
 ✓  Unlink successful
 ✓  Unlinking component "backend" from component "frontend" for secret "backend-app-8778"
 [614ms]
 ◐  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ◓  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ◑  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ◒  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ◐  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ◓  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ◑  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 ✓  Unlink successful
 ✓  Unlinking component "backend" from component "frontend" for secret "backend-app-8080"
 [2s]
 ◐  Unlinking component "backend" from component "node" for secret "backend-app-8080"
 ◓  Unlinking component "backend" from component "node" for secret "backend-app-8080"
 ◑  Unlinking component "backend" from component "node" for secret "backend-app-8080"
 ◒  Unlinking component "backend" from component "node" for secret "backend-app-8080"
 ✓  Unlink successful
 ✓  Unlinking component "backend" from component "node" for secret "backend-app-8080"
 [921ms]
 ◐  Unlinking component "backend" from component "node" for secret "backend-app-8778"
 ◓  Unlinking component "backend" from component "node" for secret "backend-app-8778"
 ◑  Unlinking component "backend" from component "node" for secret "backend-app-8778"
 ✓  Unlink successful
 ✓  Unlinking component "backend" from component "node" for secret "backend-app-8778"
 [2s]
 ✓  Deleting component backend [920ms]
 ✓  Component backend from application app has been deleted

Do you think you can suggest where I can improve here?

[dharmit]

Do you think you can suggest where I can improve here?

Think I managed to have a sane output:

odo delete -f
 ✓  Unlinking components [614ms]
 ✓  Unlinking component "backend" from component "node" for secret "backend-app-8080"

 ✓  Unlinking component "backend" from component "node" for secret "backend-app-8778"

 ✓  Deleting component backend [1s]
 ✓  Component backend from application app has been deleted

with this git diff:

$ git diff
diff --git a/pkg/odo/cli/component/delete.go b/pkg/odo/cli/component/delete.go
index 932df0f3e..617d98c92 100644
--- a/pkg/odo/cli/component/delete.go
+++ b/pkg/odo/cli/component/delete.go
@@ -99,12 +99,19 @@ func (do *DeleteOptions) Run() (err error) {
                        componentSecrets := component.UnlinkComponents(parentComponent, compoList)
 
                        for component, secret := range componentSecrets {
+                               spinner := log.Spinner("Unlinking components")
                                for _, secretName := range secret {
+
+                                       defer spinner.End(false)
+
                                        err = do.Client.UnlinkSecret(secretName, component, do.Context.Application)
                                        if err != nil {
+                                               log.Errorf("Unlinking failed")
                                                return err
                                        }
-                                       log.Successf("Unlinked component %q from component %q for secret %q", parentComponent.Name, component, secretName)
+
+                                       spinner.End(true)
+                                       log.Successf(fmt.Sprintf("Unlinking component %q from component %q for secret %q\n", parentComponent.Name, component, secretName))
                                }
                        }
                        err = component.Delete(do.Client, do.componentName, do.Application)

[mik-dass]

Think I managed to have a sane output:

@dharmit The new output looks good to me 👍

[dharmit]

@amitkrout if yes, which file do you recommend I add it to?

I would categorize it as link test.

Done. PR's ready for another round of review @kadel @mik-dass @cdrage.

[mik-dass]

/lgtm

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.