Repository: redhat-developer/service-binding-operator
Pull request #282 (2019-12-18 10:56:16)

Ensure MountPathPrefix is optional
Refer https://issues.redhat.com/browse/APPSVC-320

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 **baijum**
You can assign the PR to them by writing `/assign @baijum` 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":["baijum"]} -->
openshift-ci-robot: @baijum: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
ci/prow/e2e | 856bacaa7a37af20892a2dd830e47047a8706cdc | [link](https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/redhat-developer_service-binding-operator/282/pull-ci-redhat-developer-service-binding-operator-master-e2e/990) | `/test e2e`

[Full PR test history](https://prow.svc.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=282). [Your PR dashboard](https://prow.svc.ci.openshift.org/pr?query=is:pr+state:open+author:baijum). Please help us cut down on flakes by [linking to](https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/redhat-developer/service-binding-operator/issues?q=is:issue+is:open) when you hit one in your PR.

<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 -->
isutton: Isn't it required to run `make generate-k8s` to update the generated code?
sbose78: > Isn't it required to run make generate-k8s to update the generated code?

Yes!
sbose78: @baijum , could we address the feedback and get this in ?
codecov[bot]: # [Codecov](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282?src=pr&el=h1) Report
> Merging [#282](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282?src=pr&el=desc) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/869e38718530fee4943c8dcda58770377e8d8f2c?src=pr&el=desc) will **decrease** coverage by `0.78%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master     #282      +/-   ##
==========================================
- Coverage    59.5%   58.71%   -0.79%     
==========================================
  Files          17       16       -1     
  Lines        1284     1279       -5     
==========================================
- Hits          764      751      -13     
- Misses        390      393       +3     
- Partials      130      135       +5
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...kg/controller/servicebindingrequest/annotations.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2Fubm90YXRpb25zLmdv) | `83.78% <0%> (-2.71%)` | :arrow_down: |
| [pkg/controller/servicebindingrequest/olm.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L29sbS5nbw==) | `63.84% <0%> (-2.44%)` | :arrow_down: |
| [pkg/controller/servicebindingrequest/reconciler.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3JlY29uY2lsZXIuZ28=) | `50.31% <0%> (ø)` | :arrow_up: |
| [...kg/controller/servicebindingrequest/bindinginfo.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2JpbmRpbmdpbmZvLmdv) | | |
| [pkg/controller/servicebindingrequest/secret.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/282/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L3NlY3JldC5nbw==) | `78.94% <0%> (+1.16%)` | :arrow_up: |

------

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

baijum: I ran `make generate-k8s` and `make generate-openapi`, but it's not making any difference. If anyone can run it locally with the change in this PR, that would be helpful.
baijum: The struct tag `omitempty` was already making the field as optional. So, this change is redundant. But for consistency, we can merge this PR.

Patch:
diff --git a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go
index d80dc2fe..7e166375 100644
--- a/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go
+++ b/pkg/apis/apps/v1alpha1/servicebindingrequest_types.go
@@ -15,6 +15,7 @@ type ServiceBindingRequestSpec struct {
 	// 	https://book.kubebuilder.io/beyond_basics/generating_crd.html
 
 	// MountPathPrefix is the prefix for volume mount
+	// +optional
 	MountPathPrefix string `json:"mountPathPrefix,omitempty"`
 
 	// EnvVarPrefix is the prefix for environment variables
diff --git a/pkg/controller/servicebindingrequest/bind_custom_resources.go b/pkg/controller/servicebindingrequest/bind_custom_resources.go
index 19469e99..5520202a 100644
--- a/pkg/controller/servicebindingrequest/bind_custom_resources.go
+++ b/pkg/controller/servicebindingrequest/bind_custom_resources.go
@@ -1,6 +1,8 @@
 package servicebindingrequest
 
 import (
+	"encoding/base64"
+
 	"github.com/redhat-developer/service-binding-operator/pkg/apis/apps/v1alpha1"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -69,8 +71,8 @@ func (b DetectBindableResources) GetBindableVariables() (map[string]interface{},
 	}
 	for _, resource := range ownedResources {
 		switch resource.GetKind() {
-		// In case ConfigMap and Secret we would read data field
-		case "ConfigMap", "Secret":
+		// In case of ConfigMap we would read data field
+		case "ConfigMap":
 			for _, v := range path[resource.GetKind()] {
 				d, exist, err := unstructured.NestedMap(resource.Object, v...)
 				if err != nil {
@@ -83,6 +85,21 @@ func (b DetectBindableResources) GetBindableVariables() (map[string]interface{},
 					}
 				}
 			}
+		// In case of Secret we would decode and read data field
+		case "Secret":
+			for _, v := range path[resource.GetKind()] {
+				d, exist, err := unstructured.NestedMap(resource.Object, v...)
+				if err != nil {
+					// skipping on error
+					continue
+				}
+				if exist {
+					for k, val := range d {
+						value, _ := base64.StdEncoding.DecodeString(val.(string))
+						b.data[k] = value
+					}
+				}
+			}
 
 			// In case of Route and Service we would extract information from respective path
 		case "Route", "Service":
diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go
index bac36092..edeb98c1 100644
--- a/pkg/controller/servicebindingrequest/reconciler.go
+++ b/pkg/controller/servicebindingrequest/reconciler.go
@@ -284,9 +284,11 @@ func (r *Reconciler) bind(
 	}
 
 	// appending finalizer, should be later removed upon resource deletion
-	sbr.SetFinalizers(append(sbr.GetFinalizers(), sbrFinalizer))
-	if _, err = r.updateServiceBindingRequest(sbr); err != nil {
-		return NoRequeue(err)
+	if !containsStringSlice(sbr.GetFinalizers(), sbrFinalizer) {
+		sbr.SetFinalizers(append(sbr.GetFinalizers(), sbrFinalizer))
+		if _, err = r.updateServiceBindingRequest(sbr); err != nil {
+			return NoRequeue(err)
+		}
 	}
 
 	logger.Info("All done!")


Test patch:
diff --git a/pkg/controller/servicebindingrequest/bind_custom_resources_test.go b/pkg/controller/servicebindingrequest/bind_custom_resources_test.go
index e650f292..58209430 100644
--- a/pkg/controller/servicebindingrequest/bind_custom_resources_test.go
+++ b/pkg/controller/servicebindingrequest/bind_custom_resources_test.go
@@ -1,6 +1,8 @@
 package servicebindingrequest
 
 import (
+	"testing"
+
 	v12 "github.com/openshift/api/route/v1"
 	pgv1alpha1 "github.com/operator-backing-service-samples/postgresql-operator/pkg/apis/postgresql/v1alpha1"
 	"github.com/redhat-developer/service-binding-operator/test/mocks"
@@ -9,13 +11,13 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"testing"
 )
 
-func TestBindNonBindableResources_GetOwnedResources(t *testing.T) {
+var trueBool = true
+
+func TestBindNonBindableResources_ConfigMap_GetOwnedResources(t *testing.T) {
 	f := mocks.NewFake(t, "test")
 	cr := mocks.DatabaseCRMock("test", "test")
-	trueBool := true
 	reference := v1.OwnerReference{
 		APIVersion:         cr.APIVersion,
 		Kind:               cr.Kind,
@@ -63,12 +65,76 @@ func TestBindNonBindableResources_GetOwnedResources(t *testing.T) {
 	t.Run("Should return configmap as owned resource", func(t *testing.T) {
 		resources, err := b.GetOwnedResources()
 		require.NoError(t, err)
-		require.Equal(t, 2, len(resources), "Should return 1 owned resource")
+		require.Equal(t, 2, len(resources), "Should return 2 owned resource")
+	})
+
+	t.Run("Should return all variables exist in the configmap data section and route", func(t *testing.T) {
+		data, err := b.GetBindableVariables()
+		str1 := b.data["user"]
+		require.NoError(t, err)
+		require.Equal(t, 3, len(data), "")
+		require.Equal(t, str1, "user", "The intermediate data values are equal ")
+	})
+}
+
+func TestBindNonBindableResources_Secret_GetOwnedResources(t *testing.T) {
+	f := mocks.NewFake(t, "test")
+	cr := mocks.DatabaseCRMock("test", "test")
+	reference := v1.OwnerReference{
+		APIVersion:         cr.APIVersion,
+		Kind:               cr.Kind,
+		Name:               cr.Name,
+		UID:                cr.UID,
+		Controller:         &trueBool,
+		BlockOwnerDeletion: &trueBool,
+	}
+	secret := mocks.SecretMock("test", "test_database")
+	us := &unstructured.Unstructured{}
+	uc, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&secret)
+	require.NoError(t, err)
+	us.Object = uc
+	us.SetOwnerReferences([]v1.OwnerReference{reference})
+	route, err := runtime.DefaultUnstructuredConverter.ToUnstructured(mocks.RouteCRMock("test", "test"))
+	require.NoError(t, err)
+	usRoute := &unstructured.Unstructured{Object: route}
+	usRoute.SetOwnerReferences([]v1.OwnerReference{reference})
+	f.S.AddKnownTypes(pgv1alpha1.SchemeGroupVersion, &pgv1alpha1.Database{})
+	f.S.AddKnownTypes(v12.SchemeGroupVersion, &v12.Route{})
+	f.AddMockResource(cr)
+	f.AddMockResource(us)
+	f.AddMockResource(&unstructured.Unstructured{Object: route})
+
+	unstructuredCr, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(cr)
+	u := &unstructured.Unstructured{Object: unstructuredCr}
+	b := NewDetectBindableResources(
+		nil,
+		u,
+		[]schema.GroupVersionResource{
+			{
+				Group:    "",
+				Version:  "v1",
+				Resource: "secrets",
+			},
+			{
+				Group:    "",
+				Version:  "v1",
+				Resource: "routes",
+			},
+		},
+		f.FakeDynClient(),
+	)
+
+	t.Run("Should return secret as owned resource", func(t *testing.T) {
+		resources, err := b.GetOwnedResources()
+		require.NoError(t, err)
+		require.Equal(t, 2, len(resources), "Should return 2 owned resource")
 	})
 
-	t.Run("Should return all variables exist in the configmap data section", func(t *testing.T) {
+	t.Run("Should return all variables exist in the secret data section and route", func(t *testing.T) {
 		data, err := b.GetBindableVariables()
+		str2 := (b.data["user"]).([]byte)
 		require.NoError(t, err)
 		require.Equal(t, 3, len(data), "")
+		require.Equal(t, str2, []byte("user"), "The intermediate data values are equal ")
 	})
 }