-
Notifications
You must be signed in to change notification settings - Fork 674
otelconf: add env var substitution in ParseYAML #6215
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
base: main
Are you sure you want to change the base?
otelconf: add env var substitution in ParseYAML #6215
Conversation
e08dc29
to
7fc2745
Compare
Blocked by #6796: #6796 (comment) |
This replaces the last bit of functionality that was opened in open-telemetry#4826 to support env variable replacement. Pulled the envprovider.go code from https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/provider/envprovider/provider.go Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
7fc2745
to
99b0ce3
Compare
@MrAlias moved the code to the new package, this is no longer blocked by that. There's other comments to address that I'll get back to this week |
Signed-off-by: Alex Boten <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6215 +/- ##
=====================================
Coverage 75.8% 75.8%
=====================================
Files 207 208 +1
Lines 19494 19554 +60
=====================================
+ Hits 14783 14837 +54
- Misses 4275 4279 +4
- Partials 436 438 +2
🚀 New features to boost your workflow:
|
Signed-off-by: Alex Boten <[email protected]>
It is still blocked, until the release is cut. We agreed |
@dmathieu thanks for clarifying, with the next release started I don't suspect this change will be merged by then anyways :) |
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
|
CLA faillures is being investigated linuxfoundation/easycla#4605 |
/easycla |
Co-authored-by: Damien Mathieu <[email protected]>
@pellared would love a review from you as you've requested changes which I made |
I will do my best to review this week. Thanks a lot. |
Co-authored-by: Robert Pająk <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
} | ||
} | ||
|
||
func TestReplaceEnvVars(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add all test cases from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/data-model.md#environment-variable-substitution (the ones that are in the table)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a bunch more tests, still need to ensure all the scenarios are covered and passing, but it's underway
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks very good 👍
I would like only this to get resolved as e.g. I want to have tests that ensures that $$
are escaped correctly 😉
Signed-off-by: Alex Boten <[email protected]>
Signed-off-by: Alex Boten <[email protected]>
This replaces the last bit of functionality that was opened in #4826 to support env variable replacement. Pulled the envprovider.go code from https://github.com/open-telemetry/opentelemetry-collector/blob/main/confmap/provider/envprovider/provider.go
Fixes #4373