Repository: redhat-developer/service-binding-operator
Pull request #369 (2020-03-09 05:17:13)

Avoid appearance of empty fields
### Motivation

If the following SBR is created with a populated `BackingServiceSelectors`... 

```
apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  name: nodejs-rest-http-crud-d-demo-database-postgresql-d
  namespace: sbose
spec:
  applicationSelector:
    group: apps
    resource: deployments
    resourceRef: nodejs-rest-http-crud
    version: v1
  backingServiceSelectors:
    - group: postgresql.baiju.dev
      kind: Database
      resourceRef: demo-database
      version: v1alpha1
```
Post reconciliation, magically, the empty `BackingServiceSelector` also shows up:
```
apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  creationTimestamp: '2020-03-09T04:44:50Z'
  finalizers:
    - finalizer.servicebindingrequest.openshift.io
  generation: 3
  name: nodejs-rest-http-crud-d-demo-database-postgresql-d
  namespace: sbose
  resourceVersion: '47774'
  selfLink: >-
    /apis/apps.openshift.io/v1alpha1/namespaces/sbose/servicebindingrequests/nodejs-rest-http-crud-d-demo-database-postgresql-d
  uid: cb7a2db1-3d88-4308-9224-893058a1cad4
spec:
  applicationSelector:
    group: apps
    resource: deployments
    resourceRef: nodejs-rest-http-crud
    version: v1
  backingServiceSelector:    # <--- This should not show up
    group: ''
    kind: ''
    resourceRef: ''
    version: ''
  backingServiceSelectors:
    - group: postgresql.baiju.dev
      kind: Database
      resourceRef: demo-database
      version: v1alpha1
  detectBindingResources: false
status:
  bindingStatus: BindingSuccess
  conditions:
    - lastHeartbeatTime: '2020-03-09T04:49:41Z'
      lastTransitionTime: '2020-03-09T04:44:51Z'
      status: 'True'
      type: Ready
  secret: nodejs-rest-http-crud-d-demo-database-postgresql-d

```

### Changes

- This PR changes both the `BackingServiceSelector` and `BackingServiceSelectors` as nil-able fields to avoid this from happening. This changes fixes the issue.

- Mark `backingServiceSelectors` as `+optional`. This doesn't impact the current `required` list which anyway didn't have `backingServiceSelector` & `backingServiceSelectors`. Disclosure: This change was not required to fix this.


### Testing

Validated, the empty `backingServiceSelector` field does not show up

```
apiVersion: apps.openshift.io/v1alpha1
kind: ServiceBindingRequest
metadata:
  creationTimestamp: '2020-03-09T05:03:59Z'
  finalizers:
    - finalizer.servicebindingrequest.openshift.io
  generation: 2
  name: nodejs-rest-http-crud-d-demo-database-postgresql-d
  namespace: sbose
  resourceVersion: '54218'
  selfLink: >-
    /apis/apps.openshift.io/v1alpha1/namespaces/sbose/servicebindingrequests/nodejs-rest-http-crud-d-demo-database-postgresql-d
  uid: a922ff8e-f298-4f8a-a38b-e6491384fc4a
spec:
  applicationSelector:
    group: apps
    resource: deployments
    resourceRef: nodejs-rest-http-crud
    version: v1
  backingServiceSelectors:
    - group: postgresql.baiju.dev
      kind: Database
      resourceRef: demo-database
      version: v1alpha1
  detectBindingResources: true
status:
 ...
 ...
  secret: nodejs-rest-http-crud-d-demo-database-postgresql-d
```

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 **sbose78**
You can assign the PR to them by writing `/assign @sbose78` 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":["sbose78"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/369?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@cbb876e`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff            @@
##             master     #369   +/-   ##
=========================================
  Coverage          ?   63.44%           
=========================================
  Files             ?       21           
  Lines             ?     1387           
  Branches          ?        0           
=========================================
  Hits              ?      880           
  Misses            ?      378           
  Partials          ?      129
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/369?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/binder.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/369/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRlci5nbw==) | `58.53% <ø> (ø)` | |
| [pkg/controller/servicebindingrequest/planner.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/369/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3BsYW5uZXIuZ28=) | `89.28% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/369?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/369?src=pr&el=footer). Last update [cbb876e...dca3a13](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/369?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Avni-Sharma: /lgtm
/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/369#issuecomment-596477145" title="Approved">Avni-Sharma</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)~~ [Avni-Sharma]

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":[]} -->

Patch:
diff --git a/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml b/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml
index 676c52d3..8337c8ea 100644
--- a/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml
+++ b/deploy/crds/apps.openshift.io_servicebindingrequests_crd.yaml
@@ -100,8 +100,9 @@ spec:
               - version
               type: object
             backingServiceSelector:
-              description: BackingServiceSelector is used to identify the backing
-                service operator.
+              description: 'BackingServiceSelector is used to identify the backing
+                service operator. Deprecation Notice: In the upcoming release, this
+                field would be depcreated. It would be mandatory to set "backingServiceSelectors".'
               properties:
                 group:
                   type: string
@@ -121,7 +122,8 @@ spec:
               type: object
             backingServiceSelectors:
               description: BackingServiceSelectors is used to identify multiple backing
-                services.
+                services. This would be made a required field after 'BackingServiceSelector'
+                is removed.
               items:
                 description: BackingServiceSelector defines the selector based on
                   resource name, version, and resource kind
diff --git a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go
index 6115f66a..4a5c7743 100644
--- a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go
+++ b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go
@@ -29,11 +29,17 @@ type ServiceBindingRequestSpec struct {
 	CustomEnvVar []corev1.EnvVar `json:"customEnvVar,omitempty"`
 
 	// BackingServiceSelector is used to identify the backing service operator.
+	// Deprecation Notice:
+	// In the upcoming release, this field would be depcreated. It would be mandatory
+	// to set "backingServiceSelectors".
 	// +optional
-	BackingServiceSelector BackingServiceSelector `json:"backingServiceSelector,omitempty"`
+	BackingServiceSelector *BackingServiceSelector `json:"backingServiceSelector,omitempty"`
 
 	// BackingServiceSelectors is used to identify multiple backing services.
-	BackingServiceSelectors []BackingServiceSelector `json:"backingServiceSelectors,omitempty"`
+	// This would be made a required field after 'BackingServiceSelector'
+	// is removed.
+	// +optional
+	BackingServiceSelectors *[]BackingServiceSelector `json:"backingServiceSelectors,omitempty"`
 
 	// ApplicationSelector is used to identify the application connecting to the
 	// backing service operator.
diff --git a/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go
index a2cb15f6..17338809 100644
--- a/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go
+++ b/pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go
@@ -144,12 +144,20 @@ func (in *ServiceBindingRequestSpec) DeepCopyInto(out *ServiceBindingRequestSpec
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
-	in.BackingServiceSelector.DeepCopyInto(&out.BackingServiceSelector)
+	if in.BackingServiceSelector != nil {
+		in, out := &in.BackingServiceSelector, &out.BackingServiceSelector
+		*out = new(BackingServiceSelector)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.BackingServiceSelectors != nil {
 		in, out := &in.BackingServiceSelectors, &out.BackingServiceSelectors
-		*out = make([]BackingServiceSelector, len(*in))
-		for i := range *in {
-			(*in)[i].DeepCopyInto(&(*out)[i])
+		*out = new([]BackingServiceSelector)
+		if **in != nil {
+			in, out := *in, *out
+			*out = make([]BackingServiceSelector, len(*in))
+			for i := range *in {
+				(*in)[i].DeepCopyInto(&(*out)[i])
+			}
 		}
 	}
 	in.ApplicationSelector.DeepCopyInto(&out.ApplicationSelector)
diff --git a/pkg/apis/apps/v1alpha1/zz_generated.openapi.go b/pkg/apis/apps/v1alpha1/zz_generated.openapi.go
index eb3656c8..b667c6c1 100644
--- a/pkg/apis/apps/v1alpha1/zz_generated.openapi.go
+++ b/pkg/apis/apps/v1alpha1/zz_generated.openapi.go
@@ -188,13 +188,13 @@ func schema_pkg_apis_apps_v1alpha1_ServiceBindingRequestSpec(ref common.Referenc
 					},
 					"backingServiceSelector": {
 						SchemaProps: spec.SchemaProps{
-							Description: "BackingServiceSelector is used to identify the backing service operator.",
+							Description: "BackingServiceSelector is used to identify the backing service operator. Deprecation Notice: In the upcoming release, this field would be depcreated. It would be mandatory to set \"backingServiceSelectors\".",
 							Ref:         ref("./pkg/apis/apps/v1alpha1.BackingServiceSelector"),
 						},
 					},
 					"backingServiceSelectors": {
 						SchemaProps: spec.SchemaProps{
-							Description: "BackingServiceSelectors is used to identify multiple backing services.",
+							Description: "BackingServiceSelectors is used to identify multiple backing services. This would be made a required field after 'BackingServiceSelector' is removed.",
 							Type:        []string{"array"},
 							Items: &spec.SchemaOrArray{
 								Schema: &spec.Schema{
diff --git a/pkg/controller/servicebindingrequest/binder.go b/pkg/controller/servicebindingrequest/binder.go
index 6f939772..4811aafa 100644
--- a/pkg/controller/servicebindingrequest/binder.go
+++ b/pkg/controller/servicebindingrequest/binder.go
@@ -4,10 +4,11 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"time"
+
 	"k8s.io/apimachinery/pkg/fields"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"time"
 
 	"gotest.tools/assert/cmp"
 	corev1 "k8s.io/api/core/v1"
diff --git a/pkg/controller/servicebindingrequest/planner.go b/pkg/controller/servicebindingrequest/planner.go
index 75882a32..7428fe5d 100644
--- a/pkg/controller/servicebindingrequest/planner.go
+++ b/pkg/controller/servicebindingrequest/planner.go
@@ -76,9 +76,12 @@ var EmptyBackingServiceSelectorsErr = errors.New("backing service selectors are
 // Plan by retrieving the necessary resources related to binding a service backend.
 func (p *Planner) Plan() (*Plan, error) {
 	ns := p.sbr.GetNamespace()
-	selectors := append([]v1alpha1.BackingServiceSelector{}, p.sbr.Spec.BackingServiceSelectors...)
-	if (p.sbr.Spec.BackingServiceSelector != v1alpha1.BackingServiceSelector{}) {
-		selectors = append(selectors, p.sbr.Spec.BackingServiceSelector)
+	var selectors []v1alpha1.BackingServiceSelector
+	if p.sbr.Spec.BackingServiceSelector != nil {
+		selectors = append(selectors, *p.sbr.Spec.BackingServiceSelector)
+	}
+	if p.sbr.Spec.BackingServiceSelectors != nil {
+		selectors = append(selectors, *p.sbr.Spec.BackingServiceSelectors...)
 	}
 
 	if len(selectors) == 0 {


Test patch:
diff --git a/pkg/controller/servicebindingrequest/binding_test.go b/pkg/controller/servicebindingrequest/binding_test.go
index b72fa958..d71ae355 100644
--- a/pkg/controller/servicebindingrequest/binding_test.go
+++ b/pkg/controller/servicebindingrequest/binding_test.go
@@ -2,11 +2,12 @@ package servicebindingrequest
 
 import (
 	"encoding/base64"
+	"strings"
+	"testing"
+
 	conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
 	"github.com/redhat-developer/service-binding-operator/pkg/conditions"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
-	"strings"
-	"testing"
 
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
@@ -250,7 +251,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 				},
 				ResourceRef: d.GetName(),
 			},
-			BackingServiceSelectors: []v1alpha1.BackingServiceSelector{
+			BackingServiceSelectors: &[]v1alpha1.BackingServiceSelector{
 				{
 					GroupVersionKind: metav1.GroupVersionKind{
 						Group:   db1.GetObjectKind().GroupVersionKind().Group,
@@ -284,7 +285,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 				},
 				ResourceRef: d.GetName(),
 			},
-			BackingServiceSelectors: []v1alpha1.BackingServiceSelector{
+			BackingServiceSelectors: &[]v1alpha1.BackingServiceSelector{
 				{
 					GroupVersionKind: metav1.GroupVersionKind{
 						Group:   db1.GetObjectKind().GroupVersionKind().Group,
@@ -330,7 +331,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 				},
 				ResourceRef: d.GetName(),
 			},
-			BackingServiceSelectors: []v1alpha1.BackingServiceSelector{
+			BackingServiceSelectors: &[]v1alpha1.BackingServiceSelector{
 				{
 					GroupVersionKind: metav1.GroupVersionKind{
 						Group:   db1.GetObjectKind().GroupVersionKind().Group,
@@ -363,7 +364,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 		},
 		Spec: v1alpha1.ServiceBindingRequestSpec{
 			ApplicationSelector: v1alpha1.ApplicationSelector{},
-			BackingServiceSelectors: []v1alpha1.BackingServiceSelector{
+			BackingServiceSelectors: &[]v1alpha1.BackingServiceSelector{
 				{
 					GroupVersionKind: metav1.GroupVersionKind{
 						Group:   db1.GetObjectKind().GroupVersionKind().Group,
@@ -398,7 +399,7 @@ func TestServiceBinder_Bind(t *testing.T) {
 				},
 				ResourceRef: d.GetName(),
 			},
-			BackingServiceSelectors: []v1alpha1.BackingServiceSelector{},
+			BackingServiceSelectors: &[]v1alpha1.BackingServiceSelector{},
 		},
 		Status: v1alpha1.ServiceBindingRequestStatus{},
 	}
diff --git a/pkg/controller/servicebindingrequest/planner_test.go b/pkg/controller/servicebindingrequest/planner_test.go
index 4f7d7cb1..67d22daf 100644
--- a/pkg/controller/servicebindingrequest/planner_test.go
+++ b/pkg/controller/servicebindingrequest/planner_test.go
@@ -27,8 +27,8 @@ func TestPlanner(t *testing.T) {
 	}
 	f := mocks.NewFake(t, ns)
 	sbr := f.AddMockedServiceBindingRequest(name, nil, resourceRef, "", deploymentsGVR, matchLabels)
-	sbr.Spec.BackingServiceSelectors = []v1alpha1.BackingServiceSelector{
-		sbr.Spec.BackingServiceSelector,
+	sbr.Spec.BackingServiceSelectors = &[]v1alpha1.BackingServiceSelector{
+		*sbr.Spec.BackingServiceSelector,
 	}
 	f.AddMockedUnstructuredCSV("cluster-service-version")
 	f.AddMockedDatabaseCR(resourceRef, ns)
@@ -40,7 +40,7 @@ func TestPlanner(t *testing.T) {
 	// Out of the box, our mocks don't set the namespace
 	// ensure SearchCR fails.
 	t.Run("search CR with namespace not set", func(t *testing.T) {
-		cr, err := planner.searchCR(sbr.Spec.BackingServiceSelector)
+		cr, err := planner.searchCR(*sbr.Spec.BackingServiceSelector)
 		require.Error(t, err)
 		require.Nil(t, cr)
 	})
@@ -60,7 +60,7 @@ func TestPlanner(t *testing.T) {
 	// The searchCR contract only cares about the backingServiceNamespace
 	sbr.Spec.BackingServiceSelector.Namespace = &ns
 	t.Run("searchCR", func(t *testing.T) {
-		cr, err := planner.searchCR(sbr.Spec.BackingServiceSelector)
+		cr, err := planner.searchCR(*sbr.Spec.BackingServiceSelector)
 		require.NoError(t, err)
 		require.NotNil(t, cr)
 	})
@@ -88,7 +88,7 @@ func TestPlannerWithExplicitBackingServiceNamespace(t *testing.T) {
 	require.NotNil(t, planner)
 
 	t.Run("searchCR", func(t *testing.T) {
-		cr, err := planner.searchCR(sbr.Spec.BackingServiceSelector)
+		cr, err := planner.searchCR(*sbr.Spec.BackingServiceSelector)
 		require.NoError(t, err)
 		require.NotNil(t, cr)
 	})
diff --git a/pkg/controller/servicebindingrequest/sbrcontroller_test.go b/pkg/controller/servicebindingrequest/sbrcontroller_test.go
index 9b43e365..73f37f94 100644
--- a/pkg/controller/servicebindingrequest/sbrcontroller_test.go
+++ b/pkg/controller/servicebindingrequest/sbrcontroller_test.go
@@ -28,7 +28,7 @@ func TestSBRControllerBuildSBRPredicate(t *testing.T) {
 	t.Run("update", func(t *testing.T) {
 		sbrA := &v1alpha1.ServiceBindingRequest{
 			Spec: v1alpha1.ServiceBindingRequestSpec{
-				BackingServiceSelector: v1alpha1.BackingServiceSelector{
+				BackingServiceSelector: &v1alpha1.BackingServiceSelector{
 					GroupVersionKind: metav1.GroupVersionKind{Group: "test", Version: "v1alpha1", Kind: "TestHost"},
 					ResourceRef:      "",
 				},
@@ -36,7 +36,7 @@ func TestSBRControllerBuildSBRPredicate(t *testing.T) {
 		}
 		sbrB := &v1alpha1.ServiceBindingRequest{
 			Spec: v1alpha1.ServiceBindingRequestSpec{
-				BackingServiceSelector: v1alpha1.BackingServiceSelector{
+				BackingServiceSelector: &v1alpha1.BackingServiceSelector{
 					GroupVersionKind: metav1.GroupVersionKind{Group: "test", Version: "v1", Kind: "TestHost"},
 					ResourceRef:      "",
 				},
diff --git a/test/e2e/servicebindingrequest_test.go b/test/e2e/servicebindingrequest_test.go
index 69dcabb9..d386414d 100644
--- a/test/e2e/servicebindingrequest_test.go
+++ b/test/e2e/servicebindingrequest_test.go
@@ -377,7 +377,7 @@ func CreateSBR(
 
 // setSBRBackendGVK sets backend service selector
 func setSBRBackendGVK(sbr *v1alpha1.ServiceBindingRequest, resourceRef string, backendGVK schema.GroupVersionKind) {
-	sbr.Spec.BackingServiceSelector = v1alpha1.BackingServiceSelector{
+	sbr.Spec.BackingServiceSelector = &v1alpha1.BackingServiceSelector{
 		GroupVersionKind: metav1.GroupVersionKind{Group: backendGVK.Group, Version: backendGVK.Version, Kind: backendGVK.Kind},
 		ResourceRef:      resourceRef,
 	}
diff --git a/test/mocks/mocks.go b/test/mocks/mocks.go
index 9f052d65..0cfe6ce1 100644
--- a/test/mocks/mocks.go
+++ b/test/mocks/mocks.go
@@ -416,7 +416,7 @@ func MultiNamespaceServiceBindingRequestMock(
 				LabelSelector:        &metav1.LabelSelector{MatchLabels: matchLabels},
 			},
 			DetectBindingResources: false,
-			BackingServiceSelector: v1alpha1.BackingServiceSelector{
+			BackingServiceSelector: &v1alpha1.BackingServiceSelector{
 				GroupVersionKind: metav1.GroupVersionKind{Group: CRDName, Version: CRDVersion, Kind: CRDKind},
 				ResourceRef:      backingServiceResourceRef,
 				Namespace:        &backingServiceNamespace,
@@ -455,7 +455,7 @@ func ServiceBindingRequestMock(
 				LabelSelector:        &metav1.LabelSelector{MatchLabels: matchLabels},
 			},
 			DetectBindingResources: false,
-			BackingServiceSelector: v1alpha1.BackingServiceSelector{
+			BackingServiceSelector: &v1alpha1.BackingServiceSelector{
 				GroupVersionKind: metav1.GroupVersionKind{Group: CRDName, Version: CRDVersion, Kind: CRDKind},
 				ResourceRef:      backingServiceResourceRef,
 				Namespace:        backingServiceNamespace,