adding correct annotations for svcms and fixing sa

[tkennes]

Provide a description of what has been changed

• Wrong annotations for Prometheus service monitors
• Bug in indentation of the keda-operator serviceaccount

Fixes #437 (comment)

[tomkerkhove]

Added some breaking changes, but would you mind also fixing DCO?

[tkennes]

@tomkerkhove : I don't really understand the lack of flexible when it comes to setting annotations for specific resources, could you explain?

Also, would it not make more sense to opt for some sort of global + local mechanism to set labels? See also here for example: https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/templates/argocd-application-controller/statefulset.yaml#L3

[tomkerkhove]

@tomkerkhove : I don't really understand the lack of flexible when it comes to setting annotations for specific resources, could you explain?

Also, would it not make more sense to opt for some sort of global + local mechanism to set labels? See also here for example: argoproj/argo-helm@main/charts/argo-cd/templates/argocd-application-controller/statefulset.yaml#L3

Can you elaborate on this please? My comments were only to keep supporting what we already support, I'm find adding more annotations if there is a need, as long as it does not break things =)

[tkennes]

@tomkerkhove , in that case, it depends a bit on the cluster whether annotations are heavily used for individual resources. As a rule of thumb, you would use labels for most things especially when it comes to managing infrastructure and tying certain resources together. E.g. a service binds to a pod based on the label, same for servicemonitors and podmonitors. This is also why you cannot really use labels if you intend to segment resources further, as you will have to be careful not to misuse any labels. Hence, you use annotations. I have seen companies use annotations to route alerts to the right teams for example.

Therefore, it might make sense to allow for individual annotations per resource rather than having only one global annotation. If you see benefit in the approach, I can set this up for all resources like for ArgoCD (https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/templates/argocd-application-controller/statefulset.yaml#L3), e.g., we merge global .Values.additionalAnnotations with local resource specific ones, like for example .Values.prometheus.metricServer.serviceMonitor.additionalAnnotations.

[tkennes]

Added some breaking changes, but would you mind also fixing DCO?

@tomkerkhove , this should be resolved by now right?

[tomkerkhove]

This is OK for me, can you pull in the changes from main & document the new options please?

[tkennes]

This is OK for me, can you pull in the changes from main & document the new options please?

I don't think you need documentation here, since you are not documenting the other fields of these service- and podmonitors and since they are basic fields. This approach is also more common, instead of setting annotations over all resoruces using one global value.

Let me know if you disagree, I can add a comment in the values.yaml if you really insist, or let me know where you want these comments.

[tomkerkhove]

We should document all options, even if they are well-known ones because people who are new to the cloud-native space would not know about them unfortunately.

Given #444 is being done, it's OK to just document them in the values file, thanks!

[tkennes]

We should document all options, even if they are well-known ones because people who are new to the cloud-native space would not know about them unfortunately.

Given #444 is being done, it's OK to just document them in the values file, thanks!

I disagree on this one, but sure, there you go

[tomkerkhove]

LGTM, thanks!

Would you mind fixing the merge conflicts please?