Fix support for label selectors #971

baijum · 2021-06-04T10:01:02Z

Fix .spec.application.name field becoming mandatory
If more than one application is selected through label selectors, all will be considered for binding

Fixes #965

acceptance test demonstrating how to bind 2 applications having the same label.
Remove InvalidApplicationReference reason and corresponding code and tests
- Added issue A name and selector MUST NOT be defined in the same application reference #974
Wrong assertion for the test: should return an error if application list returns error

pedjak

a few more unit tests are missing:

when both application name and selector are set
when label selector is used but no application is annotated so the result is empty
there is an error at performing List call

and acceptance tests demonstrating the usage of the label selector and the situation when both name and label selector are set.

pedjak · 2021-06-04T10:17:06Z

pkg/reconcile/pipeline/context/impl.go

+			if i.serviceBinding.Spec.Application.Name != "" &&
+				i.serviceBinding.Spec.Application.LabelSelector != nil &&
+				i.serviceBinding.Spec.Application.LabelSelector.MatchLabels != nil {
+				return nil, errs.New("Both label selector and name for application should be used")


you mean - "should not be used"?

Done. 5326c6f

pedjak · 2021-06-04T10:22:33Z

pkg/reconcile/pipeline/context/impl.go

+
+			}
+			if i.serviceBinding.Spec.Application.LabelSelector != nil && i.serviceBinding.Spec.Application.LabelSelector.MatchLabels != nil {
+				gvr, err := i.typeLookup.ResourceForReferable(ref)


this lines repeats also in line 130 - so it could be moved before back to the original place.

Done. 5326c6f

pedjak · 2021-06-04T10:26:21Z

pkg/reconcile/pipeline/context/impl.go

+				}
+
+				if len(objList.Items) == 0 {
+					return nil, errs.New("application not found")


the contract of this method is to return empty array, not array if not applications are not found, see the doc:

service-binding-operator/pkg/reconcile/pipeline/api.go

Line 102 in d680987

Applications() ([]Application, error)

Returning nil returns an empty slice of Application objects.
See this example: https://play.golang.org/p/QvQLT9CH3Fg

Thanks, nice. However, according to the contract, we should not return error in this place, just empty slice.

Fixed and added a test case for the same: 588535c

pedjak · 2021-06-04T10:27:40Z

pkg/reconcile/pipeline/context/impl_test.go

@@ -91,6 +92,51 @@ var _ = Describe("Context", func() {
 			Entry("no binding path specified", nil, defaultContainerPath),
 			Entry("binding path specified", &v1alpha1.BindingPath{ContainersPath: "foo.bar"}, "foo.bar"),
 		)
+		DescribeTable("should return slice of size 1 if application is specifiedt through label seclector", func(bindingPath *v1alpha1.BindingPath, expectedContainerPath string) {


typo "specifiedt"

it would be also more interesting to cover the case when there at least two applications with the same label - with that we check if the loop in https://github.com/redhat-developer/service-binding-operator/pull/971/files#diff-0168dc7ac8aa540873af78cf38f886f6a5ace89fb75dce9880490cedbed1d46eR157 really works.

Done. 5326c6f

codecov · 2021-06-04T13:19:07Z

Codecov Report

Merging #971 (5e91f6d) into master (d4fd4b7) will increase coverage by 0.44%.
The diff coverage is 100.00%.

❗ Current head 5e91f6d differs from pull request most recent head 5446f72. Consider uploading reports for the commit 5446f72 to get more accurate results

@@            Coverage Diff             @@
##           master     #971      +/-   ##
==========================================
+ Coverage   54.27%   54.72%   +0.44%     
==========================================
  Files          23       23              
  Lines        1227     1239      +12     
==========================================
+ Hits          666      678      +12     
  Misses        473      473              
  Partials       88       88

Impacted Files	Coverage Δ
pkg/reconcile/pipeline/context/impl.go	`70.67% <100.00%> (+2.90%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4fd4b7...5446f72. Read the comment docs.

pedjak · 2021-06-04T21:08:33Z

@baijum can you think about some better commit message/PR summary describing what was done and how?

isutton · 2021-06-05T07:18:58Z

@baijum commented on this pull request. Returning `nil` returns an empty slice of `Application` objects. See this example: https://play.golang.org/p/QvQLT9CH3Fg

In this particular case, 'nil' is an un-initialized slice of a Application items, not an empty initialized slice of Application items. ***@***.***/golang-nil-vs-empty-slice-87fd51c0a4d I've found a good summary on why this matters.

baijum · 2021-06-07T06:56:19Z

@baijum can you think about some better commit message/PR summary describing what was done and how?

I have updated the PR title and description. It can be used for the commit message after squashing the commits.

baijum · 2021-06-07T07:15:44Z

In this particular case, 'nil' is an un-initialized slice of a Application items, not an empty initialized slice of Application items.

Done. 538772c

baijum · 2021-06-07T11:43:15Z

and acceptance tests demonstrating the usage of the label selector and the situation when both name and label selector are set.

I missed this. I will work on this.

baijum · 2021-06-08T03:45:35Z

and acceptance tests demonstrating the usage of the label selector and the situation when both name and label selector are set.

I missed this. I will work on this.

Done. 5e472be

baijum · 2021-06-08T07:44:15Z

when both application name and selector are set

Done. 7debb12

baijum · 2021-06-08T07:45:22Z

there is an error at performing List call

I couldn't figure out how to produce an error in the List call. Let me know if we can skip this for now.

pedjak

it would be good to have an additonal acceptance test demonstrating how to bind 2 applications having the same label.

pedjak · 2021-06-08T10:13:16Z

pkg/reconcile/pipeline/context/impl.go

+				if err != nil {
+					return nil, err


IMHO this check does not have to be guarded by i.serviceBinding.Spec.Application.Name != "" condition because in line 129 we get just GroupVersionResource struct, Name does play any role in that call.

Done. af6e990

pedjak · 2021-06-08T10:14:27Z

pkg/reconcile/pipeline/context/impl.go

+				}
+
+				if len(objList.Items) == 0 {
+					return nil, nil


should we return here also emptyResult, nill, similar to what we have in line 150?

Done. af6e990

pedjak · 2021-06-08T10:16:15Z

pkg/reconcile/pipeline/context/impl_test.go

@@ -91,7 +92,175 @@ var _ = Describe("Context", func() {
 			Entry("no binding path specified", nil, defaultContainerPath),
 			Entry("binding path specified", &v1alpha1.BindingPath{ContainersPath: "foo.bar"}, "foo.bar"),
 		)
+		DescribeTable("should return slice of size 1 if application is specified through label seclector", func(bindingPath *v1alpha1.BindingPath, expectedContainerPath string) {


test "should return slice of size 2 if 2 applications are specified through label seclector" covers the logic tested here, hence we could drop this test, because it does not introduce any new testing path.

Done. af6e990

pkg/reconcile/pipeline/context/impl_test.go

pedjak · 2021-06-08T10:19:29Z

pkg/reconcile/pipeline/context/impl_test.go

+			ref := v1alpha1.Application{
+				Ref: v1alpha1.Ref{
+					Group:   "app",
+					Version: "v1",
+					Kind:    "Foo",
+					Name:    "app1",
+				},
+				LabelSelector: ls,
+			}


it could be dropped from the test setup, because it is not used anywhere in the test later on.

The test is to check the effect of two kinds of application references: label & name.
Even though it is not directly used, this setup is required to validate the functionality.

pedjak · 2021-06-08T10:20:24Z

pkg/reconcile/pipeline/context/impl_test.go

+			ctx := &impl{client: client, serviceBinding: &sb, typeLookup: typeLookup}
+
+			_, err := ctx.Applications()
+			Expect(err).To(HaveOccurred())


yes, but which error has occurred? we need to assert that as well.

Done. af6e990

pedjak · 2021-06-08T11:19:42Z

test/acceptance/features/bindAppToServiceWithLabelSelector.feature

@@ -0,0 +1,111 @@
+Feature: Bind an application with label selector to a service


should we really consider this as a separate feature? this is all about referring application resources using attached labels

I think a separate feature file for the label selector is fine.

can you backup your statement with some reason?

I am fine with moving to another file. Please suggest the file.

pedjak · 2021-06-08T11:24:50Z

test/acceptance/features/bindAppToServiceWithLabelSelector.feature

+        Given Namespace [TEST_NAMESPACE] is used
+        * Service Binding Operator is running
+
+    Scenario: Bind an application with label selector to backend service


application resource has labels attached, but label selector is used inside service binding to define what application should be bound. Could we be a bit more verbose about the setup and the expected outcome? For example:

Bind successfully an application owning a given label to a service

Done. af6e990

pedjak · 2021-06-08T11:28:52Z

test/acceptance/features/bindAppToServiceWithLabelSelector.feature

+        And The application env var "BACKEND_USERNAME" has value "AzureDiamond"
+        And The application env var "BACKEND_PASSWORD" has value "hunter2"
+
+    Scenario: Attempt to bind an application with label selector and name to a service


please rephrase to include the outcome as well.. in this case service binding will be unsuccessful, right?

Done. af6e990

pedjak · 2021-06-08T11:30:10Z

test/acceptance/features/bindAppToServiceWithLabelSelector.feature

+
+        Then jq ".status.conditions[] | select(.type=="CollectionReady").status" of Service Binding "service-binding-a-s-e" should be changed to "True"
+        And jq ".status.conditions[] | select(.type=="InjectionReady").status" of Service Binding "service-binding-a-s-e" should be changed to "False"
+        And jq ".status.conditions[] | select(.type=="InjectionReady").reason" of Service Binding "service-binding-a-s-e" should be changed to "ApplicationNotFound"


IMHO - ApplicationNotFound is not correct reason - simply we do not allow name and label to cooexist - Hence, something like InvalidApplicationReference should be more appropriate.

I need some more time to figure out how to introduce a new failure reason.

pedjak · 2021-06-08T11:33:00Z

I couldn't figure out how to produce an error in the List call. Let me know if we can skip this for now.

@baijum use PrependReactor on the fake client and define for what verb and resource you would like to have an error. Something like: kubernetes/client-go#742 (comment)

baijum · 2021-06-08T14:15:20Z

I couldn't figure out how to produce an error in the List call. Let me know if we can skip this for now.

@baijum use PrependReactor on the fake client and define for what verb and resource you would like to have an error. Something like: kubernetes/client-go#742 (comment)

Done. ee27a52

pmacik · 2021-06-08T14:25:04Z

@baijum FYI, PR checks fail because of an issue with the workflows. This PR #973 fixes that.

baijum · 2021-06-08T17:28:25Z

it would be good to have an additonal acceptance test demonstrating how to bind 2 applications having the same label.

I will work on this.

baijum · 2021-06-09T05:01:04Z

pkg/reconcile/pipeline/context/impl.go

+				}
+
+				for index, _ := range objList.Items {
+					i.applications = append(i.applications, &application{gvr: gvr, persistedResource: &(objList.Items[index]), bindingPath: i.serviceBinding.Spec.Application.BindingPath})


The persistedResource value (address of the item) is taken using the index.
Ref. https://play.golang.org/p/ePs7st6f91g

pedjak · 2021-06-09T05:58:24Z

pkg/reconcile/pipeline/context/impl_test.go

+			Expect(applications[0].Resource()).To(Equal(u1))
+			Expect(applications[0].ContainersPath()).To(Equal(expectedContainerPath))
+			Expect(applications[1].Resource()).To(Equal(u2))
+			Expect(applications[1].ContainersPath()).To(Equal(expectedContainerPath))


I am not sure if we can/should really assert on the order of returned application instances. What is only interesting for us that the number of returned instances is 2 and one of them is u2 and another one is u2, it order in the slice is not important. If the order changes suddenly for some reason (say fake client implementation changes), the test would fail.

Order is not reliable and not part of the API.

Check Omega assertion library.

Done. 52aca7f

pedjak · 2021-06-09T06:05:08Z

pkg/reconcile/pipeline/context/impl.go

+			if i.serviceBinding.Spec.Application.Name != "" &&
+				i.serviceBinding.Spec.Application.LabelSelector != nil &&
+				i.serviceBinding.Spec.Application.LabelSelector.MatchLabels != nil {
+				return nil, errs.New("Both label selector and name for application should not be used")


we can extract error into a var that can be used later in test assertion - instead of string copying.

Created separate issue: #974

pedjak · 2021-06-09T06:07:09Z

pkg/reconcile/pipeline/context/impl_test.go

+			u := &unstructured.Unstructured{}
+			u.SetName("app")
+			u.SetNamespace(sb.Namespace)
+			u.SetGroupVersionKind(schema.GroupVersionKind{Group: "app", Version: "v1", Kind: "Foo"})
+			u.SetLabels(map[string]string{"env": "stage"})


we do not need to create any app object at all, since we are going to fail earlier. Omitting this will also implicitly check that we do not try to access the cluster for this scenario.

This code has been removed. Ref. #974

pedjak · 2021-06-09T06:08:48Z

pkg/reconcile/pipeline/context/impl_test.go

+			_, err := ctx.Applications()
+			Expect(err).To(HaveOccurred())
+			// FIXME: This is not the expected error message
+			Expect(err.Error()).To(ContainSubstring("no kind is registered for the type v1.DeploymentList in scheme"))


the error should be the same as the one you put in line 263: "Error listing foo"

For some reason, that error message is not coming. I have used client.PrependReactor (line 261-264) as per earlier suggestion, so the listing failed as expected.
I tried to use unstructured.Unstructured{} instead of v1.DeploymentList{}, but that also seems to be not working. I will come back to this issue once everything else finished.

Done. 8229485

baijum · 2021-06-09T09:43:30Z

it would be good to have an additonal acceptance test demonstrating how to bind 2 applications having the same label.

I will work on this.

Done. 53bb65a

pedjak · 2021-06-09T21:19:17Z

pkg/reconcile/pipeline/context/impl_test.go

+			client := fake.NewSimpleDynamicClient(runtime.NewScheme())
+			client.PrependReactor("list", "foos",
+				func(action testing.Action) (handled bool, ret runtime.Object, err error) {
+					return true, nil, e.New("Error listing foo")


DRY: please extract error in a variable and use it later in the assertion

Done. 5e91f6d

pedjak · 2021-06-09T21:21:25Z

pkg/reconcile/pipeline/context/impl_test.go

+
+			_, err := ctx.Applications()
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("Error listing foo"))


The error we get must be the one we got from the client, and the message should be the identical. Such assertion assures that the error is propagated to the caller:

Expect(err).To(Equal(expectedError))

Done. 5e91f6d

pedjak · 2021-06-09T21:21:55Z

pkg/reconcile/pipeline/context/impl_test.go

@@ -122,6 +262,7 @@ var _ = Describe("Context", func() {
 			Expect(err).To(HaveOccurred())
 			Expect(errors.IsNotFound(err)).To(BeTrue())
 		})
+


please revert unrelated whitespace change.

Done. 5e91f6d

pedjak · 2021-06-09T21:26:23Z

test/acceptance/features/steps/generic_testapp.py

+@then(u'The application env var "{name}" has value "{value}" in both apps')
+def check_env_var_value_in_both_apps(context, name, value):
+    found = polling2.poll(lambda: context.application1.get_env_var_value(name) == value, step=5, timeout=400)
+    assert found, f'Env var "{name}" should contain value "{value}" in {context.application1}'


if we reach this line, then found is true always - otherwise Timeout exception occurs. Hence, this and the similar line at line 108 can be removed.

Done. 5e91f6d

pedjak

+1 after having required CI jobs passing. Please also squash the commits.

- Fix `.spec.application.name` field becoming mandatory - If more than one application is selected through label selectors, all will be considered for binding Fixes redhat-developer#965 Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

pedjak · 2021-06-10T09:30:42Z

/lgtm
/approve

openshift-ci · 2021-06-10T09:30:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pedjak

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pedjak]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

baijum · 2021-06-10T10:11:57Z

/test 4.8-acceptance

baijum requested a review from pedjak June 4, 2021 10:01

openshift-ci bot requested a review from pmacik June 4, 2021 10:01

baijum force-pushed the label-selector branch from 3a42fb6 to 9faa091 Compare June 4, 2021 10:01

pedjak suggested changes Jun 4, 2021

View reviewed changes

baijum changed the title ~~Fix application.name field became mandatory~~ Fix support for label selectors Jun 7, 2021

baijum closed this Jun 7, 2021

baijum reopened this Jun 7, 2021

pedjak suggested changes Jun 8, 2021

View reviewed changes

baijum commented Jun 9, 2021

View reviewed changes

pedjak suggested changes Jun 9, 2021

View reviewed changes

baijum force-pushed the label-selector branch from b46fe69 to 52aca7f Compare June 9, 2021 10:13

pedjak suggested changes Jun 9, 2021

View reviewed changes

pedjak approved these changes Jun 10, 2021

View reviewed changes

baijum force-pushed the label-selector branch 3 times, most recently from 6023281 to 0676cc0 Compare June 10, 2021 07:25

Fix support for label selectors

5446f72

- Fix `.spec.application.name` field becoming mandatory - If more than one application is selected through label selectors, all will be considered for binding Fixes redhat-developer#965 Signed-off-by: Baiju Muthukadan <baiju.m.mail@gmail.com>

baijum force-pushed the label-selector branch from 0676cc0 to 5446f72 Compare June 10, 2021 07:30

openshift-ci bot assigned pedjak Jun 10, 2021

openshift-ci bot added the lgtm label Jun 10, 2021

openshift-ci bot added the approved label Jun 10, 2021

baijum mentioned this pull request Jun 10, 2021

Fix Service.OwnedResources() to return correct result #975

Merged

openshift-merge-robot merged commit 847b684 into redhat-developer:master Jun 10, 2021

baijum deleted the label-selector branch June 11, 2021 08:14

		@@ -0,0 +1,111 @@
		Feature: Bind an application with label selector to a service

Fix support for label selectors #971

Fix support for label selectors #971

Conversation

baijum commented Jun 4, 2021 • edited

pedjak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2021 • edited

Codecov Report

pedjak commented Jun 4, 2021

isutton commented Jun 5, 2021 via email

baijum commented Jun 7, 2021 • edited

baijum commented Jun 7, 2021

baijum commented Jun 7, 2021

baijum commented Jun 8, 2021

baijum commented Jun 8, 2021

baijum commented Jun 8, 2021 • edited

pedjak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedjak Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedjak commented Jun 8, 2021

baijum commented Jun 8, 2021

pmacik commented Jun 8, 2021 • edited

baijum commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baijum Jun 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baijum commented Jun 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedjak left a comment

Choose a reason for hiding this comment

pedjak commented Jun 10, 2021

openshift-ci bot commented Jun 10, 2021

baijum commented Jun 4, 2021 •

edited

codecov bot commented Jun 4, 2021 •

edited

baijum commented Jun 7, 2021 •

edited

baijum commented Jun 8, 2021 •

edited

pedjak Jun 8, 2021 •

edited

pmacik commented Jun 8, 2021 •

edited

baijum Jun 9, 2021 •

edited