Repository: redhat-developer/service-binding-operator
Pull request #741 (2020-10-29 13:25:45)

[acceptance-tests] Fix remote SBO namespace checking.
### Motivation

When using `TEST_ACCEPTANCE_START_SBO=remote` option, `SBO_NAMESPACE` is required to be set to a non-empty value, as well. Currently the check for that is missing so when the acceptance tests are executed without `SBO_NAMESPACE` the tests attempt to run with empty name for SBO namespace which is invalid.

### Changes

This PR:
* Enhances the check for `SBO_NAMESPACE` env variable value in `before_all` hook.
* Fixes: https://issues.redhat.com/browse/APPSVC-744

### Testing

Running `TEST_ACCEPTANCE_TESTS=remote make test-acceptance` should correctly fail with:
```
HOOK-ERROR in before_all: AssertionError: SBO_NAMESPACE is required but it is not set or it is empty: 
```

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 **pmacik** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @pmacik` 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:

- **[test/OWNERS](https://github.com/redhat-developer/service-binding-operator/blob/master/test/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":["pmacik"]} -->

Patch:
diff --git a/OWNERS b/OWNERS
index dcd9d954..b3b0e4e5 100644
--- a/OWNERS
+++ b/OWNERS
@@ -20,3 +20,4 @@ reviewers:
 - pratikjagrut
 - isutton
 - pedjak
+- qibobo
diff --git a/examples/java_postgresql_customvar/README.md b/examples/java_postgresql_customvar/README.md
index 9e388528..ec8e7054 100644
--- a/examples/java_postgresql_customvar/README.md
+++ b/examples/java_postgresql_customvar/README.md
@@ -100,7 +100,7 @@ kubectl edit crd databases.postgresql.baiju.dev
 Add these annotations under `metadata.annotations` along with other annotations.
 
 ```yaml
-service.binding/tags: path={.spec.tags}
+service.binding/tags: path={.spec.tags},elementType=sliceOfStrings
 service.binding/userLabels: path={.spec.userLabels},elementType=map
 service.binding/secretName: path={.spec.secretName},elementType=sliceOfMaps,sourceKey=type,sourceValue=secret
 ```
@@ -324,7 +324,8 @@ DATABASE_PASSWORD=password
 DATABASE_SECRETNAME_PRIMARYSECRETNAME=example-primaryuser
 DATABASE_SECRETNAME_ROOTSECRETNAME=example-rootuser
 DATABASE_SECRETNAME_SECONDARYSECRETNAME=example-secondaryuser
-DATABASE_TAGS=[centos7-12.3 centos7-12.4]
+DATABASE_TAGS_0=centos7-12.3
+DATABASE_TAGS_1=centos7-12.4
 DATABASE_USER=postgres
 DATABASE_USERLABELS_ARCHIVE=false
 DATABASE_USERLABELS_ENVIRONMENT=demo
diff --git a/pkg/controller/servicebinding/binding/definition.go b/pkg/controller/servicebinding/binding/definition.go
index 1548b607..822aecdb 100644
--- a/pkg/controller/servicebinding/binding/definition.go
+++ b/pkg/controller/servicebinding/binding/definition.go
@@ -286,12 +286,19 @@ func (d *sliceOfStringsFromPathDefinition) Apply(u *unstructured.Unstructured) (
 		return nil, errors.New("not found")
 	}
 
-	v := make([]string, 0, len(val))
+	v := make([]interface{}, 0, len(val))
 	for _, e := range val {
-		if mm, ok := e.(map[string]interface{}); ok {
-			sourceValue := mm[d.sourceValue].(string)
-			v = append(v, sourceValue)
+		if d.sourceValue != "" {
+			if mm, ok := e.(map[string]interface{}); ok {
+				sourceValue := mm[d.sourceValue].(string)
+				v = append(v, sourceValue)
+			}
+		} else {
+			if x, ok := e.(string); ok {
+				v = append(v, x)
+			}
 		}
+
 	}
 
 	return &value{v: map[string]interface{}{d.outputName: v}}, nil
diff --git a/pkg/controller/servicebinding/binding/model.go b/pkg/controller/servicebinding/binding/model.go
index deb7f228..1a692f94 100644
--- a/pkg/controller/servicebinding/binding/model.go
+++ b/pkg/controller/servicebinding/binding/model.go
@@ -13,7 +13,7 @@ type model struct {
 	objectType  objectType
 	sourceKey   string
 	sourceValue string
-	bindAs		BindingType
+	bindAs      BindingType
 }
 
 func (m *model) isStringElementType() bool {
diff --git a/pkg/controller/servicebinding/envvars/envvars.go b/pkg/controller/servicebinding/envvars/envvars.go
index fe54a6d7..cc874512 100644
--- a/pkg/controller/servicebinding/envvars/envvars.go
+++ b/pkg/controller/servicebinding/envvars/envvars.go
@@ -2,6 +2,7 @@ package envvars
 
 import (
 	"errors"
+	"fmt"
 	"sort"
 	"strconv"
 	"strings"
@@ -66,12 +67,14 @@ func Build(obj interface{}, path ...string) (map[string]string, error) {
 		return buildString(strconv.FormatInt(val, 10), path), nil
 	case float64:
 		return buildString(strconv.FormatFloat(val, 'f', -1, 64), path), nil
+	case []string:
+		return buildSliceOfStrings(val, path)
 	case []interface{}:
 		return buildSliceOfInterface(val, path)
 	case bool:
 		return buildString(strconv.FormatBool(val), path), nil
 	default:
-		return nil, errUnsupportedType
+		return nil, fmt.Errorf("%v: %v", errUnsupportedType, val)
 	}
 }
 
@@ -116,6 +119,19 @@ func buildMap(obj map[string]interface{}, path []string) (map[string]string, err
 	return envVars, nil
 }
 
+// buildSliceOfStrings returns a slice containing environment variables for
+// all the leaves present in the given 'obj' slice of strings.
+func buildSliceOfStrings(obj []string, acc []string) (map[string]string, error) {
+	envVars := make(map[string]string)
+	for i, v := range obj {
+		k := strconv.Itoa(i)
+		if err := buildInner(acc, k, v, envVars); err != nil {
+			return nil, err
+		}
+	}
+	return envVars, nil
+}
+
 // buildSliceOfInterface retrurns a slice containing environment variables for
 // all the leaves present in the given 'obj' slice.
 func buildSliceOfInterface(obj []interface{}, acc []string) (map[string]string, error) {
diff --git a/pkg/controller/servicebinding/nested/nested.go b/pkg/controller/servicebinding/nested/nested.go
index 8d0aa237..f9f07626 100644
--- a/pkg/controller/servicebinding/nested/nested.go
+++ b/pkg/controller/servicebinding/nested/nested.go
@@ -68,7 +68,7 @@ func convertToSlice(src interface{}) ([]interface{}, error) {
 			obj[i] = e
 		}
 	default:
-		return nil, unsupportedTypeErr
+		return nil, fmt.Errorf("%v: %v", unsupportedTypeErr, t)
 	}
 	return obj, nil
 }


Test patch:
diff --git a/pkg/controller/servicebinding/binding/definition_test.go b/pkg/controller/servicebinding/binding/definition_test.go
index 3df7e396..e0c19074 100644
--- a/pkg/controller/servicebinding/binding/definition_test.go
+++ b/pkg/controller/servicebinding/binding/definition_test.go
@@ -149,7 +149,7 @@ func TestSliceOfStringsFromPath(t *testing.T) {
 	})
 	require.NoError(t, err)
 	v := map[string]interface{}{
-		"bootstrap": []string{"www.example.com", "secure.example.com"},
+		"bootstrap": []interface{}{"www.example.com", "secure.example.com"},
 	}
 	require.Equal(t, v, val.Get())
 }
diff --git a/pkg/controller/servicebinding/binding/spec_test.go b/pkg/controller/servicebinding/binding/spec_test.go
index e674437a..69dfdd8c 100644
--- a/pkg/controller/servicebinding/binding/spec_test.go
+++ b/pkg/controller/servicebinding/binding/spec_test.go
@@ -353,11 +353,11 @@ func TestSpecHandler(t *testing.T) {
 			},
 		},
 		expectedData: map[string]interface{}{
-			"bootstrap": []string{"secure.example.com", "www.example.com"},
+			"bootstrap": []interface{}{"secure.example.com", "www.example.com"},
 		},
 		expectedRawData: map[string]interface{}{
 			"status": map[string]interface{}{
-				"bootstrap": []string{"secure.example.com", "www.example.com"},
+				"bootstrap": []interface{}{"secure.example.com", "www.example.com"},
 			},
 		},
 	}))
@@ -377,11 +377,11 @@ func TestSpecHandler(t *testing.T) {
 			},
 		},
 		expectedData: map[string]interface{}{
-			"urls": []string{"secure.example.com", "www.example.com"},
+			"urls": []interface{}{"secure.example.com", "www.example.com"},
 		},
 		expectedRawData: map[string]interface{}{
 			"status": map[string]interface{}{
-				"urls": []string{"secure.example.com", "www.example.com"},
+				"urls": []interface{}{"secure.example.com", "www.example.com"},
 			},
 		},
 	}))
diff --git a/pkg/controller/servicebinding/envvars/envvars_test.go b/pkg/controller/servicebinding/envvars/envvars_test.go
index 6acef446..14a368d0 100644
--- a/pkg/controller/servicebinding/envvars/envvars_test.go
+++ b/pkg/controller/servicebinding/envvars/envvars_test.go
@@ -136,6 +136,21 @@ func TestBuild(t *testing.T) {
 			},
 			src: "",
 		},
+		{
+			name: "should create envvars for each string in slice",
+			expected: map[string]string{
+				"TAGS_0": "knowledge",
+				"TAGS_1": "is",
+				"TAGS_2": "power",
+			},
+			src: map[string]interface{}{
+				"tags": []string{
+					"knowledge",
+					"is",
+					"power",
+				},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/test/acceptance/features/__init__.py b/test/acceptance/features/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/test/acceptance/features/bindAppToServiceAnnotations.feature b/test/acceptance/features/bindAppToServiceAnnotations.feature
index 3e7a6e28..ecf0fa71 100644
--- a/test/acceptance/features/bindAppToServiceAnnotations.feature
+++ b/test/acceptance/features/bindAppToServiceAnnotations.feature
@@ -135,6 +135,133 @@ Feature: Bind an application to a service using annotations
         And jq ".status.conditions[] | select(.type=="CollectionReady").status" of Service Binding "rsa-2" should be changed to "True"
         And jq ".status.conditions[] | select(.type=="InjectionReady").status" of Service Binding "rsa-2" should be changed to "True"
 
+    Scenario: Each value in referred slice of strings from service resource gets injected into app as separate env variable
+        Given Generic test application "slos-app" is running
+        And The Custom Resource Definition is present
+            """
+            apiVersion: apiextensions.k8s.io/v1beta1
+            kind: CustomResourceDefinition
+            metadata:
+                name: backends.stable.example.com
+                annotations:
+                    service.binding/tags: path={.spec.tags},elementType=sliceOfStrings
+            spec:
+                group: stable.example.com
+                versions:
+                  - name: v1
+                    served: true
+                    storage: true
+                scope: Namespaced
+                names:
+                    plural: backends
+                    singular: backend
+                    kind: Backend
+                    shortNames:
+                      - bk
+            """
+        And The Custom Resource is present
+            """
+            apiVersion: stable.example.com/v1
+            kind: Backend
+            metadata:
+                name: slos-service
+            spec:
+                tags:
+                  - knowledge
+                  - is
+                  - power
+            status:
+                somestatus: good
+            """
+        When Service Binding is applied
+            """
+            apiVersion: operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: slos-binding
+            spec:
+                services:
+                  - group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: slos-service
+                application:
+                    name: slos-app
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        Then jq ".status.conditions[] | select(.type=="CollectionReady").status" of Service Binding "slos-binding" should be changed to "True"
+        And jq ".status.conditions[] | select(.type=="InjectionReady").status" of Service Binding "slos-binding" should be changed to "True"
+        And The application env var "BACKEND_TAGS_0" has value "knowledge"
+        And The application env var "BACKEND_TAGS_1" has value "is"
+        And The application env var "BACKEND_TAGS_2" has value "power"
+
+    Scenario: Values extracted from each map by a given key in referred slice of maps from service resource gets injected into app as separate env variable
+        Given Generic test application "slom-to-slos-app" is running
+        And The Custom Resource Definition is present
+            """
+            apiVersion: apiextensions.k8s.io/v1beta1
+            kind: CustomResourceDefinition
+            metadata:
+                name: backends.stable.example.com
+                annotations:
+                    service.binding/url: path={.spec.connections},elementType=sliceOfStrings,sourceValue=url
+            spec:
+                group: stable.example.com
+                versions:
+                  - name: v1
+                    served: true
+                    storage: true
+                scope: Namespaced
+                names:
+                    plural: backends
+                    singular: backend
+                    kind: Backend
+                    shortNames:
+                      - bk
+            """
+        And The Custom Resource is present
+            """
+            apiVersion: stable.example.com/v1
+            kind: Backend
+            metadata:
+                name: slom-to-slos-service
+            spec:
+                connections:
+                  - type: primary
+                    url: primary.example.com
+                  - type: secondary
+                    url: secondary.example.com
+                  - type: '404'
+                    url: black-hole.example.com
+            status:
+                somestatus: good
+            """
+        When Service Binding is applied
+            """
+            apiVersion: operators.coreos.com/v1alpha1
+            kind: ServiceBinding
+            metadata:
+                name: slom-to-slos-binding
+            spec:
+                services:
+                  - group: stable.example.com
+                    version: v1
+                    kind: Backend
+                    name: slom-to-slos-service
+                application:
+                    name: slom-to-slos-app
+                    group: apps
+                    version: v1
+                    resource: deployments
+            """
+        Then jq ".status.conditions[] | select(.type=="CollectionReady").status" of Service Binding "slom-to-slos-binding" should be changed to "True"
+        And jq ".status.conditions[] | select(.type=="InjectionReady").status" of Service Binding "slom-to-slos-binding" should be changed to "True"
+        And The application env var "BACKEND_URL_0" has value "primary.example.com"
+        And The application env var "BACKEND_URL_1" has value "secondary.example.com"
+        And The application env var "BACKEND_URL_2" has value "black-hole.example.com"
+
     Scenario: Each value in referred slice of maps from service resource gets injected into app as separate env variable
         Given Generic test application "slom-app" is running
         And The Custom Resource Definition is present
diff --git a/test/acceptance/features/environment.py b/test/acceptance/features/environment.py
index 40db44f3..28b14158 100644
--- a/test/acceptance/features/environment.py
+++ b/test/acceptance/features/environment.py
@@ -12,11 +12,20 @@ before_all(context), after_all(context)
     These run before and after the whole shooting match.
 """
 
-import subprocess
+from steps.command import Command
+
 import os
 
+cmd = Command()
+
 
 def before_all(_context):
+    output, code = cmd.run("oc version | grep Client")
+    assert code == 0, f"Checking oc version failed: {output}"
+
+    oc_ver = output.split()[2]
+    assert oc_ver >= "4.5", f"oc version is required 4.5+, but is {oc_ver}."
+
     start_sbo = os.getenv("TEST_ACCEPTANCE_START_SBO")
     assert start_sbo is not None, "TEST_ACCEPTANCE_START_SBO is not set. It should be one of local, remote or operator-hub"
     assert start_sbo in {"local", "remote", "operator-hub"}, "TEST_ACCEPTANCE_START_SBO should be one of local, remote or operator-hub"
@@ -25,12 +34,12 @@ def before_all(_context):
         assert not os.getenv("TEST_ACCEPTANCE_SBO_STARTED").startswith("FAILED"), "TEST_ACCEPTANCE_SBO_STARTED shoud not be FAILED."
     elif start_sbo == "remote":
         sbo_namespace = os.getenv("SBO_NAMESPACE")
-        assert sbo_namespace is not None, "SBO_NAMESPACE is required but not set."
+        assert (sbo_namespace is not None) and (sbo_namespace != ""), f"SBO_NAMESPACE is required but it is not set or it is empty: {sbo_namespace}"
         _context.sbo_namespace = sbo_namespace
     else:
         assert False, f"TEST_ACCEPTANCE_START_SBO={start_sbo} is currently unsupported."
 
 
 def before_scenario(_context, _scenario):
-    code, output = subprocess.getstatusoutput('kubectl get project default -o jsonpath="{.metadata.name}"')
+    output, code = cmd.run('kubectl get project default -o jsonpath="{.metadata.name}"')
     assert code == 0, f"Checking connection to OS cluster by getting the 'default' project failed: {output}"