Skip to content
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

Merged
merged 2 commits into from May 22, 2024

Conversation

OpenGuidou
Copy link
Contributor

@OpenGuidou OpenGuidou commented Dec 15, 2023

Fixes #16

@OpenGuidou OpenGuidou force-pushed the metadata branch 5 times, most recently from a2e9fdc to dcf5ad3 Compare December 15, 2023 15:21
@OpenGuidou
Copy link
Contributor Author

Hi @iam-veeramalla , can I get a review on this change ?

@OpenGuidou
Copy link
Contributor Author

Or @jgwest ?

@OpenGuidou OpenGuidou force-pushed the metadata branch 2 times, most recently from 9297b8f to 548cd15 Compare January 25, 2024 08:28
@OpenGuidou
Copy link
Contributor Author

Tests should be fixed now

@OpenGuidou
Copy link
Contributor Author

Hi @jgwest ,
Can I hope for a review on this one ?

@harrietgrace
Copy link

@jgwest could you give this PR a review plz? 🙇🏻

Copy link
Member

@jgwest jgwest left a 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?

@OpenGuidou
Copy link
Contributor Author

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

@OpenGuidou
Copy link
Contributor Author

@jgwest Done now, let me know what you think about it.

@OpenGuidou OpenGuidou force-pushed the metadata branch 4 times, most recently from e143a94 to 425aa94 Compare May 21, 2024 09:22
Copy link
Member

@jgwest jgwest left a 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!

@jgwest jgwest merged commit 83239cb into argoproj-labs:main May 22, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide metadata for rollout controller resource
3 participants