Repository: redhat-developer/service-binding-operator
Pull request #959 (2021-04-30 17:15:00)

Check if unready binding can be removed
* added missing acceptance tests
* in the reconcile loop, add finalizer only if deletion timestamp is unset
* unbind handler: stop processing and do not retry in case of any error when reading the application resource

openshift-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **pedjak** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["pedjak"]} -->
openshift-ci[bot]: @pedjak: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.7-acceptance | 5fd9a838fdf72b232b950a1358c52bae98a85e79 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/959/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1388180112293236736) | `/test 4.7-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=959). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:pedjak).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @pedjak: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.7-acceptance | 5fd9a838fdf72b232b950a1358c52bae98a85e79 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/959/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1388180112293236736) | `/test 4.7-acceptance`
ci/prow/4.6-acceptance | 5fd9a838fdf72b232b950a1358c52bae98a85e79 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/959/pull-ci-redhat-developer-service-binding-operator-master-4.6-acceptance/1388180112259682304) | `/test 4.6-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=959). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:pedjak).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
openshift-ci[bot]: @pedjak: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/4.6-acceptance | 5fd9a838fdf72b232b950a1358c52bae98a85e79 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/959/pull-ci-redhat-developer-service-binding-operator-master-4.6-acceptance/1388180112259682304) | `/test 4.6-acceptance`
ci/prow/4.7-acceptance | 08a03220e4187e9decf6c567ca4dd9f3113ba5e8 | [link](https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/959/pull-ci-redhat-developer-service-binding-operator-master-4.7-acceptance/1388186941958131712) | `/test 4.7-acceptance`

[Full PR test history](https://prow.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=959). [Your PR dashboard](https://prow.ci.openshift.org/pr?query=is:pr+state:open+author:pedjak).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
pedjak: /retest
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) Report
> Merging [#959](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) (08a0322) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/c256ac60d43b29f5096fd88e77696a9179f53e08?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) (c256ac6) will **increase** coverage by `0.13%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959/graphs/tree.svg?width=650&height=150&src=pr&token=CICBeVWlrC&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer)](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer)

```diff
@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
+ Coverage   53.77%   53.90%   +0.13%     
==========================================
  Files          23       23              
  Lines        1233     1230       -3     
==========================================
  Hits          663      663              
+ Misses        480      477       -3     
  Partials       90       90              
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer) | Coverage Δ | |
|---|---|---|
| [pkg/reconcile/pipeline/handler/project/impl.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer#diff-cGtnL3JlY29uY2lsZS9waXBlbGluZS9oYW5kbGVyL3Byb2plY3QvaW1wbC5nbw==) | `54.27% <0.00%> (+0.68%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer). Last update [c256ac6...08a0322](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/959?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=redhat-developer).

DhritiShikhar: LGTM
DhritiShikhar: /approve
openshift-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-developer/service-binding-operator/pull/959#issuecomment-831137145" title="Approved">DhritiShikhar</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/OWNERS)~~ [DhritiShikhar]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
DhritiShikhar: /lgtm

Patch:
diff --git a/controllers/servicebinding_controller.go b/controllers/servicebinding_controller.go
index 9691aa5a..ae5d0856 100644
--- a/controllers/servicebinding_controller.go
+++ b/controllers/servicebinding_controller.go
@@ -79,12 +79,16 @@ func (r *ServiceBindingReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err
 		log.Error(err, "Failed to get ServiceBinding", "name", req.NamespacedName, "err", err)
 		return ctrl.Result{}, err
 	}
-	if serviceBinding.MaybeAddFinalizer() {
+	if serviceBinding.DeletionTimestamp.IsZero() && serviceBinding.MaybeAddFinalizer() {
 		if err = r.Update(ctx, serviceBinding); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
-	log.Info("Reconciling", "sb", serviceBinding)
+	if serviceBinding.DeletionTimestamp.IsZero() {
+		log.Info("Reconciling")
+	} else {
+		log.Info("Deleted, unbind the application")
+	}
 	retry, err := r.pipeline.Process(serviceBinding)
 	if !retry && err == nil {
 		if !serviceBinding.DeletionTimestamp.IsZero() {
diff --git a/pkg/reconcile/pipeline/handler/project/impl.go b/pkg/reconcile/pipeline/handler/project/impl.go
index db9df85a..2cbada98 100644
--- a/pkg/reconcile/pipeline/handler/project/impl.go
+++ b/pkg/reconcile/pipeline/handler/project/impl.go
@@ -6,7 +6,6 @@ import (
 	"github.com/redhat-developer/service-binding-operator/api/v1alpha1"
 	"github.com/redhat-developer/service-binding-operator/pkg/reconcile/pipeline"
 	corev1 "k8s.io/api/core/v1"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"path"
@@ -224,15 +223,7 @@ func Unbind(ctx pipeline.Context) {
 		return
 	}
 	applications, err := ctx.Applications()
-	if err != nil {
-		if k8serrors.IsNotFound(err) {
-			ctx.StopProcessing()
-			return
-		}
-		ctx.RetryProcessing(err)
-		return
-	}
-	if len(applications) == 0 {
+	if err != nil || len(applications) == 0 {
 		ctx.StopProcessing()
 		return
 	}


Test patch:
diff --git a/pkg/reconcile/pipeline/handler/project/impl_test.go b/pkg/reconcile/pipeline/handler/project/impl_test.go
index 5a3e337d..eaa6b0cf 100644
--- a/pkg/reconcile/pipeline/handler/project/impl_test.go
+++ b/pkg/reconcile/pipeline/handler/project/impl_test.go
@@ -507,13 +507,20 @@ var _ = Describe("Unbind handler", func() {
 		ctx = mocks.NewMockContext(mockCtrl)
 		ctx.EXPECT().UnbindRequested().Return(true)
 		ctx.EXPECT().StopProcessing()
-
 	})
 
 	AfterEach(func() {
 		mockCtrl.Finish()
 	})
 
+	It("should stop processing when there is error getting applications", func() {
+		ctx.EXPECT().Applications().Return(nil, errors.New("foo"))
+		project.Unbind(ctx)
+	})
+	It("should stop processing when there are no applications", func() {
+		ctx.EXPECT().Applications().Return([]pipeline.Application{}, nil)
+		project.Unbind(ctx)
+	})
 	Context("successful processing", func() {
 		var (
 			deploymentsUnstructured    []*unstructured.Unstructured
diff --git a/test/acceptance/features/steps/steps.py b/test/acceptance/features/steps/steps.py
index 121e7cf4..fa9dbcc1 100644
--- a/test/acceptance/features/steps/steps.py
+++ b/test/acceptance/features/steps/steps.py
@@ -501,8 +501,8 @@ def validate_error(context, err_msg=None):
 @then(u'Service Binding "{sb_name}" is not persistent in the cluster')
 def validate_absent_sb(context, sb_name):
     openshift = Openshift()
-    output = openshift.search_resource_in_namespace("servicebindings", sb_name, context.namespace.name)
-    assert output is None, f"Service Binding {sb_name} is present in namespace '{context.namespace.name}'"
+    polling2.poll(lambda: openshift.search_resource_in_namespace("servicebindings", sb_name, context.namespace.name),
+                  step=5, timeout=400, check_success=lambda v: v is None)
 
 
 @then(u'Service Binding "{sb_name}" is not updated')
diff --git a/test/acceptance/features/unbindAppToService.feature b/test/acceptance/features/unbindAppToService.feature
index 79824d19..bf9506da 100644
--- a/test/acceptance/features/unbindAppToService.feature
+++ b/test/acceptance/features/unbindAppToService.feature
@@ -154,4 +154,41 @@ Feature: Unbind an application from a service
         Then The application got redeployed 2 times so far
         * File "/bindings/remove-bindings-as-files-app-sb/host" is unavailable in application pod
         * File "/bindings/remove-bindings-as-files-app-sb/port" is unavailable in application pod
-        * Service Binding secret is not present
\ No newline at end of file
+        * Service Binding secret is not present
+
+    Scenario: Remove not ready binding
+        Given The Custom Resource is present
+            """
+            apiVersion: "stable.example.com/v1"
+            kind: Backend
+            metadata:
+                name: example-backend
+                annotations:
+                    service.binding/host: path={.spec.host}
+                    service.binding/username: path={.spec.username}
+            spec:
+                host: example.com
+                username: foo
+            """
+        * Service Binding is applied
+            """
+            apiVersion: binding.operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: unready-binding
+            spec:
+                bindAsFiles: false
+                application:
+                    name: not-found-app
+                    group: apps
+                    version: v1
+                    resource: deployments
+                services:
+                -   group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: example-backend
+            """
+        * jq ".status.conditions[] | select(.type=="Ready").status" of Service Binding "unready-binding" should be changed to "False"
+        When Service binding "unready-binding" is deleted
+        Then Service Binding "unready-binding" is not persistent in the cluster