open-telemetry / opentelemetry-helm-charts

OpenTelemetry Helm Charts
https://opentelemetry.io
Apache License 2.0
391 stars 473 forks source link

[Operator] Delete collector image version from the values.yaml #1189

Open jdiegosierra opened 4 months ago

jdiegosierra commented 4 months ago

Hello,

I noticed that currently .Chart.AppVersion is the default value for the opentelemetry-operator.image https://github.com/open-telemetry/opentelemetry-helm-charts/blob/2f4e7a8f960366b961d621c9bc7c6ae389129af3/charts/opentelemetry-operator/templates/_helpers.tpl#L150

Keeping in mind that the operator and the collector should have the same version https://github.com/open-telemetry/opentelemetry-operator?tab=readme-ov-file#opentelemetry-operator-vs-opentelemetry-collector.

Why not using appVersion as default for collectorImage as well?

Or simply why the values.yaml has the collector image set? https://github.com/open-telemetry/opentelemetry-helm-charts/blob/2f4e7a8f960366b961d621c9bc7c6ae389129af3/charts/opentelemetry-operator/values.yaml#L45

If I'm not wrong, if we don't set the --collector-image flag https://github.com/open-telemetry/opentelemetry-helm-charts/blob/2f4e7a8f960366b961d621c9bc7c6ae389129af3/charts/opentelemetry-operator/templates/deployment.yaml#L51 The operator will take care of deploying a collector with the same version as itself, isn't it? So why is needed to set a value for the collector by default?

TylerHelmuth commented 4 months ago

The operator will take care of deploying a collector with the same version as itself, isn't it? So why is needed to set a value for the collector by default?

I'd rather do this instead of reuse appVersion.

jdiegosierra commented 4 months ago

The operator will take care of deploying a collector with the same version as itself, isn't it? So why is needed to set a value for the collector by default?

I'd rather do this instead of reuse appVersion.

Yeah, probably it makes more sense. So there is no reason to have a default value in the values.yaml, right? Do you agree to delete the default value?

TylerHelmuth commented 4 months ago

Yes I think that is a safe, un-breaking change that will make operator upgrades easier

jdiegosierra commented 4 months ago

I'm going to check if the operator works as expected in case of not using the flag --collector-image

jdiegosierra commented 4 months ago

I'm going to check if the operator works as expected in case of not using the flag --collector-image

Seems that it works as expected, I deployed the operator setting an empty tag:

collectorImage:
    repository: ghcr.io/otel/opentelemetry-collector-contrib
    tag: ""
(k8s|kind-kind:otel-test) ~/Projects/Personal/opentelemetry-helm-charts/charts/opentelemetry-operator: k describe pod simplest-collector-54bb659675-m7jfg | grep Image
    Image:         ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector:0.99.0
    Image ID:      ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector@sha256:bba8474ea69d11f0450e38c50040efea47f4190723a9fb64cc9d44939b4ade4c
jdiegosierra commented 4 months ago

Oh, I was not aware of this https://github.com/open-telemetry/opentelemetry-helm-charts/issues/1153.

jdiegosierra commented 4 months ago

Taking into account the https://github.com/open-telemetry/opentelemetry-helm-charts/issues/1153 I tried with --collector-image=ghcr.io/otel/opentelemetry-collector-contrib rather than --collector-image=ghcr.io/otel/opentelemetry-collector-contrib:0.99.0 and it doesn't work, so I proceed with closing the issue.

jdiegosierra commented 4 months ago

I noticed that I was using a wrong collector URL to test the flag. I used ghcr.io/otel/opentelemetry-collector-contrib rather than otel/opentelemetry-collector or otel/opentelemetry-collector-contrib. Now it works properly. So I think it is safe to delete the tag version.