Repository: redhat-developer/service-binding-operator
Pull request #433 (2020-04-22 09:23:10)

fix csv resource checking
### Motivation
Bug introduced in PR https://github.com/redhat-developer/service-binding-operator/pull/397.
Should check CSVs using resource `clusterserviceversions` instead of kind `ClusterServiceVersion`


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 assign **otaviof**
You can assign the PR to them by writing `/assign @otaviof` in a comment when ready.

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":["otaviof"]} -->
openshift-ci-robot: Hi @int8dev. Thanks for your PR.

I'm waiting for a [redhat-developer](https://github.com/orgs/redhat-developer/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/redhat-developer/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=redhat-developer%2Fservice-binding-operator).

<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.
</details>

sbose78: /ok-to-test
sbose78: Hi, how have you validated this?
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433?src=pr&el=h1) Report
> Merging [#433](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433?src=pr&el=desc) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/a60d54a2851383ab643b5098f9b08624136cdd30&el=desc) will **increase** coverage by `0.83%`.
> The diff coverage is `90.90%`.

[![Impacted file tree graph](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433/graphs/tree.svg?width=650&height=150&src=pr&token=CICBeVWlrC)](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
+ Coverage   65.14%   65.97%   +0.83%     
==========================================
  Files          21       21              
  Lines        1423     1446      +23     
==========================================
+ Hits          927      954      +27     
+ Misses        373      370       -3     
+ Partials      123      122       -1     
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../controller/servicebindingrequest/sbrcontroller.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3NicmNvbnRyb2xsZXIuZ28=) | `24.00% <0.00%> (ø)` | |
| [pkg/controller/servicebindingrequest/binder.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRlci5nbw==) | `63.87% <91.66%> (+4.28%)` | :arrow_up: |
| [pkg/controller/servicebindingrequest/binding.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/433/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRpbmcuZ28=) | `71.42% <100.00%> (ø)` | |

------

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


Patch:
diff --git a/pkg/controller/servicebindingrequest/binder.go b/pkg/controller/servicebindingrequest/binder.go
index e733e4c2..9840bb6f 100644
--- a/pkg/controller/servicebindingrequest/binder.go
+++ b/pkg/controller/servicebindingrequest/binder.go
@@ -13,7 +13,6 @@ import (
 
 	"gotest.tools/assert/cmp"
 	corev1 "k8s.io/api/core/v1"
-	k8serror "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -65,6 +64,7 @@ func (f ExtraFieldsModifierFunc) ModifyExtraFields(u *unstructured.Unstructured)
 }
 
 var EmptyApplicationSelectorErr = errors.New("application ResourceRef or MatchLabel not found")
+var ApplicationNotFound = errors.New("Application is already deleted")
 
 // search objects based in Kind/APIVersion, which contain the labels defined in ApplicationSelector.
 func (b *Binder) search() (*unstructured.UnstructuredList, error) {
@@ -95,15 +95,13 @@ func (b *Binder) search() (*unstructured.UnstructuredList, error) {
 
 	objList, err := b.dynClient.Resource(gvr).Namespace(ns).List(opts)
 	if err != nil {
-		return nil, err
+		return nil, ApplicationNotFound
+
 	}
 
 	// Return fake NotFound error explicitly to ensure requeue when objList(^) is empty.
 	if len(objList.Items) == 0 {
-		return nil, k8serror.NewNotFound(
-			gvr.GroupResource(),
-			b.sbr.Spec.ApplicationSelector.GroupVersionResource.Resource,
-		)
+		return nil, ApplicationNotFound
 	}
 	return objList, err
 }
@@ -555,6 +553,9 @@ func (b *Binder) remove(objs *unstructured.UnstructuredList) error {
 func (b *Binder) Unbind() error {
 	objs, err := b.search()
 	if err != nil {
+		if errors.Is(err, ApplicationNotFound) {
+			return nil
+		}
 		return err
 	}
 	return b.remove(objs)
@@ -565,6 +566,9 @@ func (b *Binder) Unbind() error {
 func (b *Binder) Bind() ([]*unstructured.Unstructured, error) {
 	objs, err := b.search()
 	if err != nil {
+		if errors.Is(err, ApplicationNotFound) {
+			return nil, nil
+		}
 		return nil, err
 	}
 	return b.update(objs)
diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go
index c6f574dd..0c6df2a8 100644
--- a/pkg/controller/servicebindingrequest/reconciler.go
+++ b/pkg/controller/servicebindingrequest/reconciler.go
@@ -1,6 +1,8 @@
 package servicebindingrequest
 
 import (
+	"errors"
+
 	v1 "github.com/openshift/custom-resource-status/conditions/v1"
 	"github.com/redhat-developer/service-binding-operator/pkg/conditions"
 	corev1 "k8s.io/api/core/v1"
@@ -114,6 +116,10 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 	// fetch and validate namespaced ServiceBindingRequest instance
 	sbr, err := r.getServiceBindingRequest(request.NamespacedName)
 	if err != nil {
+		if errors.Is(err, ApplicationNotFound) {
+			logger.Info("SBR deleted after application deletion")
+			return Done()
+		}
 		logger.Error(err, "On retrieving service-binding-request instance.")
 		return DoneOnNotFound(err)
 	}
@@ -147,8 +153,10 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 		if err == EmptyBackingServiceSelectorsErr || err == EmptyApplicationSelectorErr {
 			// TODO: find or create an error type containing suitable information to be propagated
 			var reason string
-			if err == EmptyBackingServiceSelectorsErr {
+			if errors.Is(err, EmptyBackingServiceSelectorsErr) {
 				reason = "EmptyBackingServiceSelectors"
+			} else if errors.Is(err, ApplicationNotFound) {
+				reason = "ApplicationUnavailable"
 			} else {
 				reason = "EmptyApplicationSelector"
 			}
diff --git a/pkg/controller/servicebindingrequest/sbrcontroller.go b/pkg/controller/servicebindingrequest/sbrcontroller.go
index bc61e287..a6f82b4d 100644
--- a/pkg/controller/servicebindingrequest/sbrcontroller.go
+++ b/pkg/controller/servicebindingrequest/sbrcontroller.go
@@ -147,7 +147,7 @@ func (s *SBRController) AddWatchForGVK(gvk schema.GroupVersionKind) error {
 // addCSVWatch creates a watch on ClusterServiceVersion.
 func (s *SBRController) addCSVWatch() error {
 	log := s.logger
-	gvr := olmv1alpha1.SchemeGroupVersion.WithResource(ClusterServiceVersionKind)
+	gvr := olmv1alpha1.SchemeGroupVersion.WithResource(csvResource)
 	resourceClient := s.Client.Resource(gvr).Namespace(os.Getenv("WATCH_NAMESPACE"))
 	_, err := resourceClient.List(metav1.ListOptions{})
 	if err != nil && errors.IsNotFound(err) {


Test patch:
diff --git a/pkg/controller/servicebindingrequest/reconciler_test.go b/pkg/controller/servicebindingrequest/reconciler_test.go
index 525503ed..929dae0a 100644
--- a/pkg/controller/servicebindingrequest/reconciler_test.go
+++ b/pkg/controller/servicebindingrequest/reconciler_test.go
@@ -229,14 +229,14 @@ func TestReconcilerGenericBinding(t *testing.T) {
 	// Reconcile without deployment
 	res, err := reconciler.Reconcile(reconcileRequest())
 	require.NoError(t, err)
-	require.True(t, res.Requeue)
+	require.False(t, res.Requeue)
 
 	namespacedName := types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
 	sbrOutput, err := reconciler.getServiceBindingRequest(namespacedName)
 	require.NoError(t, err)
 
 	require.Equal(t, conditions.BindingReady, sbrOutput.Status.Conditions[0].Type)
-	require.Equal(t, corev1.ConditionFalse, sbrOutput.Status.Conditions[0].Status)
+	require.Equal(t, corev1.ConditionTrue, sbrOutput.Status.Conditions[0].Status)
 	require.Equal(t, 0, len(sbrOutput.Status.Applications))
 
 	// Reconcile with deployment
@@ -347,13 +347,15 @@ func TestEmptyApplicationSelector(t *testing.T) {
 
 	res, err := reconciler.Reconcile(reconcileRequest())
 	require.NoError(t, err)
-	require.True(t, res.Requeue)
+	require.False(t, res.Requeue)
 
 	namespacedName := types.NamespacedName{Namespace: reconcilerNs, Name: reconcilerName}
 	sbrOutput, err := reconciler.getServiceBindingRequest(namespacedName)
 	require.NoError(t, err)
 
 	require.Equal(t, conditions.BindingReady, sbrOutput.Status.Conditions[0].Type)
-	require.Equal(t, corev1.ConditionFalse, sbrOutput.Status.Conditions[0].Status)
+	// Currently the Conditions[0].Status would be true as application's absence won't cause error
+	// TODO New steps to conditions to be introduced - InjectionReady, CollectionReady
+	require.Equal(t, corev1.ConditionTrue, sbrOutput.Status.Conditions[0].Status)
 	require.Equal(t, 0, len(sbrOutput.Status.Applications))
 }