feat(environmentConfig): allow setting sourceFieldPath label selector optional #4547
Conversation
…ctors as optional Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
phisco
changed the title
internal/controller/apiextensions/composite/environment/selector_test.go
Outdated
Show resolved
Hide resolved
internal/controller/apiextensions/composite/environment/selector_test.go
Show resolved
Hide resolved
| @@ -132,7 +135,7 @@ func (s *APIEnvironmentSelector) lookUpConfigs(ctx context.Context, cr resource. | |||
| func (s *APIEnvironmentSelector) buildEnvironmentConfigRefFromSelector(cl *v1alpha1.EnvironmentConfigList, selector *v1.EnvironmentSourceSelector) ([]corev1.ObjectReference, error) { | |||
| ec := make([]v1alpha1.EnvironmentConfig, 0) | |||
|
|
|||
| if len(cl.Items) == 0 { | |||
| if cl == nil || len(cl.Items) == 0 { | |||
There was a problem hiding this comment.
should be a regular case to pass nil for cl in this function? lookUpConfig never returns nil, nil so perhaps we should return error instead? Otherwise, could we add selector unit test for this change?
There was a problem hiding this comment.
It should never happen but I feel it's ok to handle it as if it had 0 items.
There was a problem hiding this comment.
If it should never happen, then passing nil here might smell like a bug, and failing early would uncover it.
There was a problem hiding this comment.
Then let me rephrase: "it can happen and should be handled as if it was an empty list"
There was a problem hiding this comment.
ok, can you please add selector unit test for that situation?
There was a problem hiding this comment.
I had a look at it. Currently there is no way to pass nil there and there are no tests yet specifically targeting buildEnvironmentConfigRefFromSelector. I don't personally see a reason to add a test for it, but feel free to let me know if you feel differently. In Go the length of nil lists and maps is already 0, so I would not be surprised reading this, nor would expect something else from it while using it.
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Description of your changes
Fixes #3723 .
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR, if necessary.