Fix annotations propagation #1505
Conversation
Signed-off-by: Yuri Sa <[email protected]>
Signed-off-by: Yuri Sa <[email protected]>
|
@yuriolisa please add changelog entry |
Signed-off-by: Yuri Sa <[email protected]>
| @@ -43,22 +43,6 @@ func Annotations(instance v1alpha1.OpenTelemetryCollector) map[string]string { | |||
| return annotations | |||
| } | |||
|
|
|||
| // PodAnnotations return the spec annotations for OpenTelemetryCollector pod. | |||
| func PodAnnotations(instance v1alpha1.OpenTelemetryCollector) map[string]string { | |||
There was a problem hiding this comment.
I wonder why the pod annotations were separated from the others. Did you try to look it up in the history?
There was a problem hiding this comment.
I took a look at the history, but it seems this behavior was a try to fix a bug related to reconciling the collector once the configmap changes. However, I don't know why it wasn't simply added the sha256 onto the Deployment annotations and replicate them to the podAnnotations.
There was a problem hiding this comment.
Do you think we could have a situation in which pods should have a specific annotation not present on Deployment's annotations?
| @@ -103,9 +106,17 @@ func TestDaemonsetHostNetwork(t *testing.T) { | |||
| func TestDaemonsetPodAnnotations(t *testing.T) { | |||
| // prepare | |||
| testPodAnnotationValues := map[string]string{"annotation-key": "annotation-value"} | |||
|
|
|||
| // Add sha256 podAnnotation | |||
| testPodAnnotationValues["opentelemetry-operator-config/sha256"] = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" | |||
There was a problem hiding this comment.
this can be moved up to the declaration of testPodAnnotationValues
| @@ -48,7 +47,7 @@ func Deployment(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTele | |||
| Template: corev1.PodTemplateSpec{ | |||
| ObjectMeta: metav1.ObjectMeta{ | |||
| Labels: labels, | |||
| Annotations: podAnnotations, | |||
| Annotations: annotations, | |||
There was a problem hiding this comment.
Won't this introduce a regression where you can no longer set specific pod annotations? I rely on exactly this currently in a few places.
There was a problem hiding this comment.
Okay, so, to avoid this kind of regression, I would propagate the annotations into podAnnotations. WDYT?
There was a problem hiding this comment.
I think it would be fine to append the annotations in to the pod annotations. How would you handle conflicts?
There was a problem hiding this comment.
WDYT to take the Deployment's annotations as precedence?
There was a problem hiding this comment.
There was a problem hiding this comment.
@yuriolisa i think the pod annotations should win, otherwise there would be no way to override something on the deployment.
Fixes #900