Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[demo] Remove duplicate envvar on frontendproxy #1438

Merged
merged 2 commits into from
Dec 1, 2024

Conversation

lambdanis
Copy link
Contributor

@lambdanis lambdanis commented Dec 1, 2024

OTEL_SERVICE_NAME envvar is set on all components as one of envvars listed in default.env, to the value of app.kubernetes.io/component label. Recently 51993de ("Bump dependencies and fix grafana dashboards") duplicated OTEL_SERVICE_NAME on the frontendproxy deployment by setting it explicitly to frontend-proxy. A duplicated envvar causes an error when upgrading the opentelemetry-demo chart:

 Deployment.apps "opentelemetry-demo-frontendproxy" is invalid:
 spec.template.spec.containers[0].env[0].valueFrom: Invalid value: "": may
 not be specified when `value` is not empty

My understanding is that the duplicated envvar was introduced in #1422 as a follow-up to open-telemetry/opentelemetry-demo#1768 that introduced it in docker-compose setup of opentelemetry-demo? But I don't have context on why it was needed there.

cc @julianocosta89 @puckpuck as authors of the PRs.

OTEL_SERVICE_NAME envvar is set on all components as one of envvars listed in
default.env, to the value of app.kubernetes.io/component label. Recently
51993de ("Bump dependencies and fix grafana dashboards") duplicated
OTEL_SERVICE_NAME on the frontendproxy deployment by setting it explicitly to
frontend-proxy. A duplicated envvar causes an error when upgrading the
opentelemetry-demo chart:

     Deployment.apps "opentelemetry-demo-frontendproxy" is invalid:
     spec.template.spec.containers[0].env[0].valueFrom: Invalid value: "": may
     not be specified when `value` is not empty

This commit removes the duplicated envvar.

Signed-off-by: Anna Kapuscinska <anna@isovalent.com>
@julianocosta89
Copy link
Member

Thanks for spotting it @lambdanis!

To update the Helm chart you also need to bump the minor version of the chart (https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-demo/Chart.yaml#L4), and then run:

make generate-examples CHARTS=opentelemetry-demo

Signed-off-by: Anna Kapuscinska <anna@isovalent.com>
@lambdanis
Copy link
Contributor Author

Done, thanks for review @julianocosta89

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dmitryax dmitryax merged commit 41cd816 into open-telemetry:main Dec 1, 2024
3 checks passed
rohanarora pushed a commit to rohanarora/opentelemetry-helm-charts that referenced this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants