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

Support default values for env var expansion #10907

Merged
merged 7 commits into from
Sep 10, 2024

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Aug 17, 2024

Description

Support shell-style default value and error message for env var expansion.

// A default value for unset variable can be provided after :- suffix, for example:
// `env:NAME_OF_ENVIRONMENT_VARIABLE:-default_value`

Link to tracking issue

Fixes #5228

Testing

Unit tests

Documentation

  • Provider Go docs
  • Probably needs some other docs changes?

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro requested review from a team and djaglowski August 17, 2024 19:54
Signed-off-by: Yuri Shkuro <github@ysh.us>
@TylerHelmuth
Copy link
Member

I'd like to make sure we're in alignment with the configuration sig on the syntax before merging this

@yurishkuro
Copy link
Member Author

@TylerHelmuth I though that was settled, per #5228 (comment)

The OpenTelemetry Configuration WG now admits the shell syntax for the SDK configuration files. Since we aim to be aligned with them, it makes sense in my opinion to add support in the Collector for this as well.

@yurishkuro
Copy link
Member Author

@mx-psi is Configuration WG decision on shell syntax documented / implemented somewhere?

Copy link

codecov bot commented Aug 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.90%. Comparing base (b10029c) to head (4d741e7).
Report is 87 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10907      +/-   ##
==========================================
+ Coverage   91.89%   91.90%   +0.01%     
==========================================
  Files         411      411              
  Lines       19311    19320       +9     
==========================================
+ Hits        17745    17756      +11     
+ Misses       1217     1216       -1     
+ Partials      349      348       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mx-psi
Copy link
Member

mx-psi commented Aug 19, 2024

@mx-psi is Configuration WG decision on shell syntax documented / implemented somewhere?

Yup, see https://opentelemetry.io/docs/specs/otel/configuration/file-configuration/#environment-variable-substitution

Signed-off-by: Yuri Shkuro <github@ysh.us>
@mx-psi
Copy link
Member

mx-psi commented Aug 19, 2024

Note that the :?error syntax is not supported by the Configuration WG. I am in favor of it, but it would be good to at least check that the Configuration WG is not vehemently opposed to eventually adding that

// `env:NAME_OF_ENVIRONMENT_VARIABLE:-default_value`
//
// An error message for unset variable can be provided after :? suffix, for example:
// `env:NAME_OF_ENVIRONMENT_VARIABLE:?error_message`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not aware of a definition for setting an error message, is there an open issue somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should still have it. I don't understand the approach "let's be like Shell but just different enough so that common things don't work".

Copy link
Contributor

@codeboten codeboten Aug 19, 2024

Choose a reason for hiding this comment

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

not disagreeing, this was me asking if there is already a discussion about this somewhere I haven't found yet

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the approach "let's be like Shell but just different enough so that common things don't work".

This doesn't mean that we need to add full support for Bash/POSIX-like syntax today. Your PR does not add full support for all the features that Bash supports and that's okay. As I said above, I am not opposed to adding this, but since alignment with the Configuration WG is an explicit goal we should bring it up on their repo first.

yurishkuro and others added 3 commits August 25, 2024 17:35
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro
Copy link
Member Author

I removed error syntax -- 3a3f939.

Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Sep 10, 2024
@mx-psi mx-psi merged commit 6082ac7 into open-telemetry:main Sep 10, 2024
49 checks passed
@github-actions github-actions bot added this to the next release milestone Sep 10, 2024
dmathieu pushed a commit to dmathieu/opentelemetry-collector that referenced this pull request Sep 10, 2024
…ry#10907)

#### Description
Support shell-style default value and error message for env var
expansion.
```
// A default value for unset variable can be provided after :- suffix, for example:
// `env:NAME_OF_ENVIRONMENT_VARIABLE:-default_value`
```

#### Link to tracking issue
Fixes open-telemetry#5228

#### Testing
Unit tests

#### Documentation
* [x] Provider Go docs 
* [ ] Probably needs some other docs changes?

---------

Signed-off-by: Yuri Shkuro <github@ysh.us>
@yurishkuro yurishkuro changed the title Support default values and erorrs for env var expansion Support default values for env var expansion Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support default values for configuration environment variables
4 participants