Repository: redhat-developer/service-binding-operator
Pull request #99 (2019-08-06 03:22:15)

add UPDATE predicate to avoid reconcile events when status is updated
When `status` gets updated in future PRs, filter out such events to avoid making `reconcile` too chatty.

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/99?src=pr&el=h1) Report
> Merging [#99](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/99?src=pr&el=desc) into [master](https://codecov.io/gh/redhat-developer/service-binding-operator/commit/5e13c0de1532b88aa8e036232bef4b61ccc416f0?src=pr&el=desc) will **decrease** coverage by `1.35%`.
> The diff coverage is `0%`.

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

```diff
@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
- Coverage   66.56%   65.21%   -1.36%     
==========================================
  Files           5        5              
  Lines         338      345       +7     
==========================================
  Hits          225      225              
- Misses         88       95       +7     
  Partials       25       25
```


| [Impacted Files](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/99?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/controller/servicebindingrequest/controller.go](https://codecov.io/gh/redhat-developer/service-binding-operator/pull/99/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvc2VydmljZWJpbmRpbmdyZXF1ZXN0L2NvbnRyb2xsZXIuZ28=) | `0% <0%> (ø)` | :arrow_up: |

------

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

openshift-ci-robot: @sbose78: The following test **failed**, say `/retest` to rerun them all:

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

[Full PR test history](https://prow.svc.ci.openshift.org/pr-history?org=redhat-developer&repo=service-binding-operator&pr=99). [Your PR dashboard](https://prow.svc.ci.openshift.org/pr?query=is:pr+state:open+author:sbose78). 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 -->
sbose78: /test lint

Patch:
diff --git a/README.md b/README.md
index c0276fcd..22791042 100644
--- a/README.md
+++ b/README.md
@@ -187,7 +187,7 @@ and click on the `[Create]` button.
 
 Notice, that during the import no DB config was mentioned or requestd.
 
-When the application is running navigate to its route to verify that it is up. Notice that in the header it says `(DB: N/A)`. That meens that the application is not connected to a DB and so it should not work properly. Try the application's UI to add a fruit - it causes an error proving that the DB is not connected.
+When the application is running navigate to its route to verify that it is up. Notice that in the header it says `(DB: N/A)`. That means that the application is not connected to a DB and so it should not work properly. Try the application's UI to add a fruit - it causes an error proving that the DB is not connected.
 
 #### Create a DB instance for the application
 
diff --git a/pkg/controller/servicebindingrequest/controller.go b/pkg/controller/servicebindingrequest/controller.go
index ea12525e..1bf2a8f0 100644
--- a/pkg/controller/servicebindingrequest/controller.go
+++ b/pkg/controller/servicebindingrequest/controller.go
@@ -3,8 +3,10 @@ package servicebindingrequest
 import (
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
@@ -30,6 +32,17 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 		return err
 	}
 
+	pred := predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			// Ignore updates to CR status in which case metadata.Generation does not change
+			return e.MetaOld.GetGeneration() != e.MetaNew.GetGeneration()
+		},
+		DeleteFunc: func(e event.DeleteEvent) bool {
+			// Evaluates to false if the object has been confirmed deleted.
+			return !e.DeleteStateUnknown
+		},
+	}
+
 	// Watch for changes to primary resource ServiceBindingRequest
 	err = c.Watch(&source.Kind{Type: &v1alpha1.ServiceBindingRequest{}}, &handler.EnqueueRequestForObject{})
 	if err != nil {
@@ -41,7 +54,8 @@ func add(mgr manager.Manager, r reconcile.Reconciler) error {
 	err = c.Watch(&source.Kind{Type: &corev1.Pod{}}, &handler.EnqueueRequestForOwner{
 		IsController: true,
 		OwnerType:    &v1alpha1.ServiceBindingRequest{},
-	})
+	}, pred)
+
 	if err != nil {
 		return err
 	}
diff --git a/pkg/controller/servicebindingrequest/reconciler.go b/pkg/controller/servicebindingrequest/reconciler.go
index 6aa9058c..a28b17cd 100644
--- a/pkg/controller/servicebindingrequest/reconciler.go
+++ b/pkg/controller/servicebindingrequest/reconciler.go
@@ -116,7 +116,7 @@ func (r *Reconciler) Reconcile(request reconcile.Request) (reconcile.Result, err
 		return RequeueOnNotFound(err)
 	}
 
-	retriever := NewRetriever(ctx, r.client, plan)
+	retriever := NewRetriever(ctx, r.client, plan, instance.Spec.EnvVarPrefix)
 	if err = retriever.Retrieve(); err != nil {
 		return RequeueOnNotFound(err)
 	}
diff --git a/pkg/controller/servicebindingrequest/retriever.go b/pkg/controller/servicebindingrequest/retriever.go
index a0ad90ec..3225bade 100644
--- a/pkg/controller/servicebindingrequest/retriever.go
+++ b/pkg/controller/servicebindingrequest/retriever.go
@@ -17,16 +17,16 @@ import (
 
 // Retriever reads all data referred in plan instance, and store in a secret.
 type Retriever struct {
-	ctx        context.Context   // request context
-	client     client.Client     // Kubernetes API client
-	plan       *planner.Plan     // plan instance
-	logger     logr.Logger       // logger instance
-	data       map[string][]byte // data retrieved
-	volumeKeys []string
+	ctx           context.Context   // request context
+	client        client.Client     // Kubernetes API client
+	plan          *planner.Plan     // plan instance
+	logger        logr.Logger       // logger instance
+	data          map[string][]byte // data retrieved
+	volumeKeys    []string
+	bindingPrefix string
 }
 
 const (
-	bindingPrefix           = "SERVICE_BINDING"
 	basePrefix              = "urn:alm:descriptor:servicebindingrequest:env:object"
 	secretPrefix            = basePrefix + ":secret"
 	configMapPrefix         = basePrefix + ":configmap"
@@ -177,7 +177,11 @@ func (r *Retriever) readConfigMap(name string, items []string) error {
 func (r *Retriever) store(key string, value []byte) {
 	key = strings.ReplaceAll(key, ":", "_")
 	key = strings.ReplaceAll(key, ".", "_")
-	key = fmt.Sprintf("%s_%s_%s", bindingPrefix, r.plan.CR.GetKind(), key)
+	if r.bindingPrefix == "" {
+		key = fmt.Sprintf("%s_%s", r.plan.CR.GetKind(), key)
+	} else {
+		key = fmt.Sprintf("%s_%s_%s", r.bindingPrefix, r.plan.CR.GetKind(), key)
+	}
 	key = strings.ToUpper(key)
 	r.data[key] = value
 }
@@ -226,13 +230,14 @@ func (r *Retriever) Retrieve() error {
 }
 
 // NewRetriever instantiate a new retriever instance.
-func NewRetriever(ctx context.Context, client client.Client, plan *planner.Plan) *Retriever {
+func NewRetriever(ctx context.Context, client client.Client, plan *planner.Plan, bindingPrefix string) *Retriever {
 	return &Retriever{
-		ctx:        ctx,
-		client:     client,
-		logger:     logf.Log.WithName("retriever"),
-		plan:       plan,
-		data:       make(map[string][]byte),
-		volumeKeys: []string{},
+		ctx:           ctx,
+		client:        client,
+		logger:        logf.Log.WithName("retriever"),
+		plan:          plan,
+		data:          make(map[string][]byte),
+		volumeKeys:    []string{},
+		bindingPrefix: bindingPrefix,
 	}
 }


Test patch:
diff --git a/pkg/controller/servicebindingrequest/retriever_test.go b/pkg/controller/servicebindingrequest/retriever_test.go
index 8fd02bb3..ddffc379 100644
--- a/pkg/controller/servicebindingrequest/retriever_test.go
+++ b/pkg/controller/servicebindingrequest/retriever_test.go
@@ -39,7 +39,7 @@ func TestRetriever(t *testing.T) {
 	objs := []runtime.Object{&dbSecret}
 	fakeClient := fake.NewFakeClient(objs...)
 
-	retriever = NewRetriever(context.TODO(), fakeClient, plan)
+	retriever = NewRetriever(context.TODO(), fakeClient, plan, "SERVICE_BINDING")
 	require.NotNil(t, retriever)
 
 	t.Run("retrive", func(t *testing.T) {
@@ -102,6 +102,17 @@ func TestRetriever(t *testing.T) {
 		err := retriever.saveDataOnSecret()
 		assert.Nil(t, err)
 	})
+	t.Run("empty prefix", func(t *testing.T) {
+		retriever = NewRetriever(context.TODO(), fakeClient, plan, "")
+		require.NotNil(t, retriever)
+		retriever.data = make(map[string][]byte)
+
+		err := retriever.readSecret("db-credentials", []string{"user", "password"})
+		assert.Nil(t, err)
+
+		assert.Contains(t, retriever.data, "DATABASE_SECRET_USER")
+		assert.Contains(t, retriever.data, "DATABASE_SECRET_PASSWORD")
+	})
 }
 
 func TestRetrieverNestedCRDKey(t *testing.T) {
@@ -128,7 +139,7 @@ func TestRetrieverNestedCRDKey(t *testing.T) {
 	objs := []runtime.Object{&dbSecret}
 	fakeClient := fake.NewFakeClient(objs...)
 
-	retriever = NewRetriever(context.TODO(), fakeClient, plan)
+	retriever = NewRetriever(context.TODO(), fakeClient, plan, "SERVICE_BINDING")
 	require.NotNil(t, retriever)
 
 	t.Run("Second level", func(t *testing.T) {
@@ -175,7 +186,7 @@ func TestConfigMapRetriever(t *testing.T) {
 	objs := []runtime.Object{&dbConfigMap}
 	fakeClient := fake.NewFakeClient(objs...)
 
-	retriever = NewRetriever(context.TODO(), fakeClient, plan)
+	retriever = NewRetriever(context.TODO(), fakeClient, plan, "SERVICE_BINDING")
 	require.NotNil(t, retriever)
 
 	t.Run("read", func(t *testing.T) {
diff --git a/test/e2e/servicebindingrequest_test.go b/test/e2e/servicebindingrequest_test.go
index 9e24b0e2..633a205d 100644
--- a/test/e2e/servicebindingrequest_test.go
+++ b/test/e2e/servicebindingrequest_test.go
@@ -172,10 +172,10 @@ func serviceBindingRequestTest(t *testing.T, ctx *framework.TestCtx, f *framewor
 	t.Logf("Checking intermediary secret '%s'...", name)
 	sbrSecret := corev1.Secret{}
 	require.Nil(t, f.Client.Get(todoCtx, types.NamespacedName{Namespace: ns, Name: name}, &sbrSecret))
-	assert.Contains(t, sbrSecret.Data, "SERVICE_BINDING_DATABASE_SECRET_USER")
-	assert.Equal(t, []byte("user"), sbrSecret.Data["SERVICE_BINDING_DATABASE_SECRET_USER"])
-	assert.Contains(t, sbrSecret.Data, "SERVICE_BINDING_DATABASE_SECRET_PASSWORD")
-	assert.Equal(t, []byte("password"), sbrSecret.Data["SERVICE_BINDING_DATABASE_SECRET_PASSWORD"])
+	assert.Contains(t, sbrSecret.Data, "DATABASE_SECRET_USER")
+	assert.Equal(t, []byte("user"), sbrSecret.Data["DATABASE_SECRET_USER"])
+	assert.Contains(t, sbrSecret.Data, "DATABASE_SECRET_PASSWORD")
+	assert.Equal(t, []byte("password"), sbrSecret.Data["DATABASE_SECRET_PASSWORD"])
 
 	// cleaning up
 	t.Log("Cleaning all up!")