argo-rollouts-manager icon indicating copy to clipboard operation
argo-rollouts-manager copied to clipboard

feat: Support additional metadata for controller

Open OpenGuidou opened this issue 1 year ago • 7 comments

Fixes #16

OpenGuidou avatar Dec 15 '23 13:12 OpenGuidou

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

OpenGuidou avatar Jan 10 '24 15:01 OpenGuidou

Or @jgwest ?

OpenGuidou avatar Jan 19 '24 13:01 OpenGuidou

Tests should be fixed now

OpenGuidou avatar Jan 25 '24 08:01 OpenGuidou

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

OpenGuidou avatar Mar 26 '24 16:03 OpenGuidou

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

harrietgrace avatar May 07 '24 16:05 harrietgrace

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 avatar May 21 '24 06:05 OpenGuidou

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

OpenGuidou avatar May 21 '24 07:05 OpenGuidou