-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Support additional metadata for controller #17
Conversation
a2e9fdc
to
dcf5ad3
Compare
Hi @iam-veeramalla , can I get a review on this change ? |
Or @jgwest ? |
9297b8f
to
548cd15
Compare
Tests should be fixed now |
Hi @jgwest , |
@jgwest could you give this PR a review plz? 🙇🏻 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@jgwest Done now, let me know what you think about it. |
e143a94
to
425aa94
Compare
Signed-off-by: Guillaume Doussin <[email protected]>
Signed-off-by: Jonathan West <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks again for your patience/persistence with us 😅 , and thanks for your quick response to review comments!
Fixes #16