Consolidate transformers for common CRD fields, registry and config #29
Conversation
knative-prow-robot
added
approved
|
/retest |
There was a problem hiding this comment.
/lgtm
/approve
/hold
/assign @aliok @houshengbo
I'd love if somebody else could take a look as well. I don't feel quite qualified enough.
knative-prow-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcrossley3, markusthoemmes 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:
Approvers can indicate their approval by writing |
Fixes knative#26 Also factored the common test utilities out of images.go and into a common/testing package. All tests should remain intact -- eventing had a few more than serving -- now all in a single place.
I like seeing the exact list of transformers in the reconciler's transform function.
|
The following is the coverage report on the affected files.
|
|
/retest |
| if overrideImage := registry.Override[containerName]; overrideImage != "" { | ||
| return overrideImage | ||
| func getNewImage(registry *v1alpha1.Registry, containerName, parent string) string { | ||
| if image, ok := registry.Override[parent+delimiter+containerName]; ok { |
There was a problem hiding this comment.
This made the operator CR support /containerName as valid key when parent is empty. I know it does not seem harmful, but is this what you intentionally to enable?
There was a problem hiding this comment.
It wasn't intentional, but I'm not sure it's worth accounting for since / isn't a valid character in container names. At least, that's how I interpret this: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#path-segment-names
|
/hold cancel |
knative-prow-robot
added
lgtm
Signed-off-by: Matthias Wessendorf <[email protected]>
Fixes #26
Also factored the common test utilities out of images.go and into a common/testing package.
All tests should remain intact -- eventing had a few more than serving -- now all in a single place.