argo-rollouts-manager
argo-rollouts-manager copied to clipboard
feat: Support additional metadata for controller
Fixes #16
Hi @iam-veeramalla , can I get a review on this change ?
Or @jgwest ?
Tests should be fixed now
Hi @jgwest , Can I hope for a review on this one ?
@jgwest could you give this PR a review plz? 🙇🏻
Thanks for your patience, @OpenGuidou! Happy to say the operator is finally in good shape, and we are now able to accept PRs that are outside the operator's initial, essential GA functionality (These were cluster-scoped support, unit tests, E2E tests, build improvements, etc )
Your PR looks great, and looks like a useful feature to add, I just have a question around the implementation.
What was the criteria used to determine which resources to apply the
.spec.controllerMetadata
labels/annotations to?In this PR, it appears that labels/annotations that are defined in
.spec.controllerMetadata
will be applied to these generated resources:
- Deployment + Pods of that Deployment (via PodSpec)
- RoleBinding
- Service (for metrics)
But labels/annotations will not be applied to these generated resources:
- ConfigMap
- ClusterRoleBinding
- ServiceAccount
- Role
- ClusterRole
(Correct me if I am wrong)
I don't see a pattern: I would assume that the user's expectation would be that the labels/annotations are applied to all resources, for example, rather than just Deployment/RoleBinding/Service. Likewise, it seems to inconsistent to apply the labels/annotations only to RoleBinding and not ClusterRoleBinding.
Was this intentional, or (I'm guessing) is just because it was based on an older version of the code where some of these resources didn't exist?
You are right, I tried to add it everywhere but I guess the Cluster resources didn't exist then. I can add them and rename the field as well to not only talk about the controller
@jgwest Done now, let me know what you think about it.