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

[file configuration] Clarify what should happen for invalid environment variable substitution #3915

Closed
pellared opened this issue Feb 29, 2024 · 5 comments · Fixed by #4002
Closed
Assignees
Labels
spec:miscellaneous For issues that don't match any other spec label triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned

Comments

@pellared
Copy link
Member

What are you trying to achieve?

From: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#environment-variable-substitution

For example, ${API_KEY} is valid, while ${1API_KEY} and ${API_$KEY} are invalid.

I think saying "invalid" does not make it clear how it should it be interpreted.

Should the parsing fail? I do not think so,

Should it not make the substitution and leave it as a string? This is what I propose.

Maybe we could rephrase it to e.g.

For example, ${API_KEY} is a environment variable substitution, while ${1API_KEY} and ${API_$KEY} are just strings.

CC @jack-berg

@pellared pellared added the spec:miscellaneous For issues that don't match any other spec label label Feb 29, 2024
@pellared pellared changed the title [file configuration] [file configuration] Clarify what should happen for invalid environment variable substitution Feb 29, 2024
@pellared
Copy link
Member Author

FY @marcalff

@marcalff
Copy link
Member

For example, ${API_KEY} is a environment variable substitution, while ${1API_KEY} and ${API_$KEY} are just strings.

I agree.

@jack-berg
Copy link
Member

Agree with the proposal. Let's open a PR and include an example as well.

@jack-berg jack-berg added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Mar 13, 2024
@svrnm svrnm added triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned and removed [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR labels Apr 22, 2024
@svrnm
Copy link
Member

svrnm commented Apr 22, 2024

@jack-berg we assumed that you are the sponsor for this (cc @jpkrohling @danielgblanco)

@jack-berg
Copy link
Member

I can sponsor, but not necessarily opening the PR myself.

@pellared, @marcalff please review #4002, which proposes solving this by erroring instead of keeping a string. The related issue #3981 makes a good case for why erroring is preferred.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:miscellaneous For issues that don't match any other spec label triage:accepted:ready-with-sponsor Ready to be implemented and has a specification sponsor assigned
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants