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

[opentelemetry-collector] update MY_POD_IP with ${env:MY_POD_IP} #767

Merged
merged 5 commits into from
May 2, 2023

Conversation

JaredTan95
Copy link
Member

close #761

@Allex1
Copy link
Contributor

Allex1 commented Apr 28, 2023

@JaredTan95 this seems like a breaking change so can you pls bump the minor version of the chart and update the docs ?

@JaredTan95
Copy link
Member Author

this seems like a breaking change

Sorry, I don't understand why it's a breaking change. Can you tell me about it? thx~

@Allex1
Copy link
Contributor

Allex1 commented Apr 28, 2023

Sorry, I don't understand why it's a breaking change. Can you tell me about it? thx~

Reading #761 seems that ${env:ENV} syntax is not supported before collector 0.71 . We may have users of this helm chart that overwrite the older version of the collector right ?
I would make it clear that it will not work in that case. wdyt ?

@povilasv
Copy link
Contributor

Probably in this case it's better to be safe than sorry, so let's bump the minor version and add some note in https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/UPGRADING.md :)

@JaredTan95
Copy link
Member Author

@Allex1 @povilasv thx, got you~

@dmitryax
Copy link
Member

dmitryax commented Apr 30, 2023

Reading #761 seems that ${env:ENV} syntax is not supported before collector 0.71 . We may have users of this helm chart that overwrite the older version of the collector right ?

0.71.0 got only a fix that is unrelated to our use cases. ${env:...} syntax has been supported for long time. I belive it was added in 2021 :) Not sure we really need the upgrade guidelines

charts/opentelemetry-collector/UPGRADING.md Outdated Show resolved Hide resolved
@dmitryax dmitryax merged commit 2d9c3dc into open-telemetry:main May 2, 2023
@JaredTan95 JaredTan95 deleted the update_pod_ip branch May 2, 2023 06:01
@mx-psi
Copy link
Member

mx-psi commented May 2, 2023

Reading #761 seems that ${env:ENV} syntax is not supported before collector 0.71 . We may have users of this helm chart that overwrite the older version of the collector right ?

0.71.0 got only a fix that is unrelated to our use cases. ${env:...} syntax has been supported for long time. I belive it was added in 2021 :) Not sure we really need the upgrade guidelines

The ${env:ENV} syntax has been supported since v0.62.0 (October 2022). It was available under a feature gate before, but it was disabled by default.

The fix in v0.71.0 (February 2023) is necessary for all the usages here to work properly; before v0.71.0 every single usage on the Helm chart will silently fail to expand.

@TylerHelmuth
Copy link
Member

@mx-psi thanks for the details. @JaredTan95 sorry, but would you be willing to submit another PR touching up the UPGRADING doc saying that using chart version 0.55.1 or higher requires a collector version 0.71.0 or higher?

@dmitryax
Copy link
Member

dmitryax commented May 2, 2023

Oh, @mx-psi is right. I am sorry for causing the confusion!

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.

Use ${env:ENV} style syntax when getting environment variables
6 participants