do not add CR serviceAnnotations into grafanaAlert service #1064
Conversation
Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
paulczar
changed the title
|
@celestialorb do you have any thoughts on this PR? |
@NissesSenap I'm fine with this, though depending on how you view this this could potentially be a breaking change. Someone might be relying on the fact that the I think the ideal solution is that there would be a separate field for applying annotations to the alert service, but removing the application of annotations from the alert service is a fine first step if we're okay with it and don't consider it to be a breaking change. |
|
@NissesSenap I reviewed the CR definition for the Grafana type and I think this change is fine and not breaking. The service field of it reads to me as only applying to the Grafana service (and not alert service) so I would view this as a bugfix and not a breaking change. 👍 |
There was a problem hiding this comment.
Since we only send in a nil value, so in practice it's not possible to set any specific AlertServiceAnnotation https://github.com/grafana-operator/grafana-operator/blob/f41668f73c4aa54129472a3aab5dfb50f7db6979/controllers/model/grafanaAlertService.go#L37
It's better that we remove the function and remove the usage of it all together.
|
@celestialorb and @paulczar I think we should be able to remove this function all together, unless I'm thinking wrong. When the PR gets updated, I'm happy to merge it. |
|
@NissesSenap Yeah, I'd say there's not really a reason to be adding annotations to the alert service for now. Only case I can think of is maybe for a service mesh or other operator that acts off of annotations. I think until someone raises a use case for having them removing the annotations (and thus the function call and definition) is valid. |
|
Created #1065 to address this. Feel free to close it if this PR is updated with the requested changes. |
|
@NissesSenap @celestialorb thanks for taking care of this!! I was travelling so it was a happy surprise to get back and find it being resolved. I had left the function in case someone wanted the scaffolding in place to set separate annotations for grafana-alert, but happy to see it gone and merged in #1065. |
fixes #1050