Add Service Credential Bindings of type Key #3662
Conversation
ddraganovv
force-pushed
the
add_service_binding_key_type
branch
from
de22dd0 to
885960b
Compare
There was a problem hiding this comment.
Looks good overall, minor comments. The most important is that there is no controller test that verifies that servicebindingio bindings are not created when the binding is of type key
Could you please just push the changes addressing the comments into this branch without recreating the PR? When you recreate the PR my previous comments and review notes are lost and I have to rereview all the changes all over again which takes time
| ServiceInstance: &payloads.Relationship{ | ||
| Data: &payloads.RelationshipData{ | ||
| GUID: "service-instance-guid", | ||
| When("creating a service binding of type key", func() { |
There was a problem hiding this comment.
This When is missing the "happy" path when creating a binding of type key to a managed service succeeds. I guess this test should only check that the expected HTTP response code is returned and the repository has been invoked with correct arguments. Inspecting the response content is already done in the tests below, no need to duplicate it.
api/payloads/service_binding.go
Outdated
| func (l *ServiceBindingList) ToMessage() repositories.ListServiceBindingsMessage { | ||
| return repositories.ListServiceBindingsMessage{ | ||
| message := repositories.ListServiceBindingsMessage{ |
There was a problem hiding this comment.
Just return the literal here, the `message variable is not needed
| @@ -18,13 +20,23 @@ var _ = Describe("ServiceBindingList", func() { | |||
| Expect(decodeErr).NotTo(HaveOccurred()) | |||
| Expect(*actualServiceBindingList).To(Equal(expectedServiceBindingList)) | |||
| }, | |||
| Entry("type", "type=key", payloads.ServiceBindingList{Type: korifiv1alpha1.CFServiceBindingTypeKey}), | |||
There was a problem hiding this comment.
as the validation of the payload type now allows key and app values for type, I would suggest to test those valud values with two entries:
Entry("type=key", "type=key", payloads.ServiceBindingList{Type: korifiv1alpha1.CFServiceBindingTypeKey}),
Entry("type=app", "type=app", payloads.ServiceBindingList{Type: korifiv1alpha1.CFServiceBindingTypeApp}),
Same goes for ServiceBindingCreate - as the type now can be key, I guess you should ammend this test - I believe it would fail
| Entry("app_guids", "app_guids=app_guid", payloads.ServiceBindingList{AppGUIDs: "app_guid"}), | ||
| Entry("service_instance_guids", "service_instance_guids=si_guid", payloads.ServiceBindingList{ServiceInstanceGUIDs: "si_guid"}), | ||
| Entry("include", "include=include", payloads.ServiceBindingList{Include: "include"}), | ||
| Entry("include", "include=app", payloads.ServiceBindingList{Include: "app"}), |
There was a problem hiding this comment.
Same goes here, include now can be either app, or service_instance, therefore two entries with those two values could verify they are accepted
api/payloads/service_binding_test.go
Outdated
| Expect(serviceBindingCreate).To(gstruct.PointTo(Equal(createPayload))) | ||
| }) | ||
|
|
||
| When("binding is key and name field is omitted", func() { |
There was a problem hiding this comment.
when the binding is key has been already specified by the parent When description, no need to repeat it here (i.e. change the description to When("name field is imitted", ...)
| @@ -272,9 +277,9 @@ var _ = Describe("ServiceBindingRepo", func() { | |||
|
|
|||
| It("creates a new CFServiceBinding resource and returns a record", func() { | |||
| Expect(createErr).NotTo(HaveOccurred()) | |||
|
|
|||
| Expect(serviceBindingRecord.GUID).NotTo(BeEmpty()) | |||
There was a problem hiding this comment.
guid is asserted to be a valid guid below, no need to check for not being empty (an empty string is not a valid uuid)
| Expect(serviceBindingRecord.GUID).To(matchers.BeValidUUID()) | ||
| Expect(serviceBindingRecord.Type).To(Equal("app")) | ||
| Expect(serviceBindingRecord.Name).To(BeNil()) | ||
| Expect(serviceBindingRecord.GUID).NotTo(BeEmpty()) |
There was a problem hiding this comment.
In order to be consistent with the tests above, you could Expect(serviceBindingRecord.GUID).To(matchers.BeValidUUID())
| }, | ||
| }, | ||
| )) | ||
| Expect(serviceBinding.Spec.Type).To(Equal(korifiv1alpha1.CFServiceBindingTypeApp)) |
There was a problem hiding this comment.
Checking the spec's field one by one with Equal is a bit awkward. What you have done is perfectly fine, but if you want to, you could only match the fields that are interesting to this test. For example, the code does not set the Spec.Service.APIVersion explicitly, so no need to check it
Here is an example of MatchFields where we only check the fields we are interested in.
| It("creates the binding with the specified name", func() { | ||
| Expect(serviceBindingRecord.Name).To(Equal(bindingName)) | ||
| }) | ||
| It("creates a key binding", func() { |
There was a problem hiding this comment.
I would suggesst shortening this It to only check the fields of the record that make sense to the key binding type, i.e. Type being set to key, AppGUID being empty and app relationship not being set. The rest of the fields are being checked in the parent context, so checking them here as well is just unneeded duplication
| @@ -145,6 +144,10 @@ func (r *Reconciler) ReconcileResource(ctx context.Context, cfServiceBinding *ko | |||
| return res, err | |||
| } | |||
|
|
|||
| if cfServiceBinding.Spec.Type == korifiv1alpha1.CFServiceBindingTypeKey { | |||
| return ctrl.Result{}, nil | |||
There was a problem hiding this comment.
There is no controller test that asserts that no servicebindingio bindings are created to bindings of type key. Please add one
ddraganovv
force-pushed
the
add_service_binding_key_type
branch
5 times, most recently
from
9389946 to
9874f7f
Compare
danail-branekov
force-pushed
the
add_service_binding_key_type
branch
from
9874f7f to
994ddbd
Compare
ddraganovv
force-pushed
the
add_service_binding_key_type
branch
from
994ddbd to
bd19ef3
Compare
Is there a related GitHub Issue?
#2320
What is this change about?
With this PR we are introducing a new type of service binding - 'key'.
Does this PR introduce a breaking change?
No
Acceptance Steps
Tag your pair, your PM, and/or team