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

[configuration] Fail on unsupported identifiers #4002

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Apr 18, 2024

Fixes #3981, fixes #3915

Changes

Fail to parse when invalid identifiers are found (this is basically anything that uses the ${...} syntax but contains an invalid blob in between). The rationale is to allow for changes in a backwards compatible way. Right now it would not be possible to extend the syntax in a non-breaking way to support more sh/POSIX/Bash syntax like the ones here or potentially to support other providers (like we support in the Collector e.g. ${file:<something>}).

As an example both #3974 and #3948 would have been nonbreaking had we done this change first (we would go from "fail to parse" to "do something", instead of going from one way of parsing to a different one).

For non-trivial changes, follow the change proposal process.

@mx-psi mx-psi marked this pull request as ready for review April 18, 2024 15:59
@mx-psi mx-psi requested review from a team April 18, 2024 15:59
@mx-psi
Copy link
Member Author

mx-psi commented Apr 18, 2024

cc @open-telemetry/configuration-maintainers @TylerHelmuth

@jack-berg
Copy link
Member

jack-berg commented Apr 18, 2024

The rationale is to allow for changes in a backwards compatible way. Right now it would not be possible to extend the syntax in a non-breaking way to support more sh/POSIX/Bash syntax like the ones here or potentially to support other providers (like we support in the Collector e.g. ${file:}).

I think we we can accommodate this differently. We can adjust the regular expression definition to define a group for the optional env:, file: prefix, and state later state that the only supported group is env:. This would allow us to add additional substitution sources later in a non-breaking way.

EDIT: I think I misunderstood the goal. You're not saying to fail when an env var is referenced but unset. You're saying fail when an env var substitution expression doesn't quite match the regexp, and thus would be left as a string unless we fail.

@mx-psi
Copy link
Member Author

mx-psi commented Apr 19, 2024

I think I misunderstood the goal. You're not saying to fail when an env var is referenced but unset. You're saying fail when an env var substitution expression doesn't quite match the regexp, and thus would be left as a string unless we fail.

Indeed, that's what I meant :)

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @mx-psi

@jack-berg
Copy link
Member

FYI, this also closes #3915, but in a different want than suggested in the convo on that issue.

@mx-psi
Copy link
Member Author

mx-psi commented Apr 23, 2024

FYI, this also closes #3915, but in a different want than suggested in the convo on that issue.

Updated PR description to also close this one

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@jack-berg
Copy link
Member

Planning on merging in two days if there are no additional comments. I know @pellared and @marcalff expressed a preference to not error in the #3915 discussion. Please review / comment.

@mx-psi
Copy link
Member Author

mx-psi commented Apr 30, 2024

docfx is failing because of open-telemetry/semantic-conventions/issues/987

@reyang reyang merged commit 6f4fd8d into open-telemetry:main Apr 30, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants