Add YAML aware variable substitution#8342
Add YAML aware variable substitution#8342AlphaOne1 wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
|
@AlphaOne1, can you please provide examples of current incorrect behavior? It would be nice if you create an issue (bug) for tracking. |
|
@AlphaOne1, bump. |
|
The issue 8405 was created to track this bug. |
|
The last commit addresses the issues pointed out during the last pipeline run. Could you run it again? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8342 +/- ##
=====================================
Coverage 82.1% 82.1%
=====================================
Files 179 179
Lines 13662 13719 +57
=====================================
+ Hits 11229 11276 +47
- Misses 2032 2037 +5
- Partials 401 406 +5
🚀 New features to boost your workflow:
|
pellared
left a comment
pellared
left a comment
There was a problem hiding this comment.
I have forgotten to publish some comments 2 weeks ago...
|
Comments of @pellared now included. |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Thanks @AlphaOne1! This looks great, just a few questions in the comments
| { // 24 | ||
| yamlInput: "key: ${UNDEFINED_KEY:-$${UNDEFINED_KEY}}", | ||
| // yamlOutput: `key: ${UNDEFINED_KEY:-${UNDEFINED_KEY}}`, | ||
| // this is what the spec tells, but I object, |
There was a problem hiding this comment.
Is there already an issue in the spec around this objection? It would be great to have consistency across implementations
There was a problem hiding this comment.
I did not (yet) file an issue to them. I did not yet dig into their contribution guidelines. As an initial step, do you agree with my argumentation there?
There was a problem hiding this comment.
pinging @mx-psi as he did a lot of the initial spec work. wdyt about this issue?
There was a problem hiding this comment.
At a first glance the argumentation seems sound and key: ${UNDEFINED_KEY} is what I would expect.
This does not seem to be what the Collector does (see open-telemetry/opentelemetry-collector/pull/14720), but I think what the Collector does is clearly a bug.
I think it would be worthwhile to raise an issue at the spec level.
|
I included the changes @codeboten proposed. |
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…rsing: - Adjust `envVarRegexp` to handle multiple dollar signs more effectively. - Improve node retyping logic for better YAML type detection. - Remove `checkRawConfType` function and related references. - Treat injected or invalid YAML types as strings rather than causing errors. - Update tests to reflect adjusted behavior for YAML injection and substitution handling. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
- Add `preserveYAMLLines` to retain original line numbers for improved debugging. - Replace errors in `retypeNode` with `errors.New` for consistency. - Enhance test cases to account for escaped newlines in YAML values. Signed-off-by: Alexander Adam <alphaone23@gmail.com> # Conflicts: # otelconf/config_test.go
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
AlphaOne1
force-pushed
the
add_yaml_aware_variable_substitution
branch
from
68bfd76 to
d649069
Compare
|
I rebased onto main, to resolve the conflicts. |
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
thanks @AlphaOne1, one of those comments can be resolved
| { // 24 | ||
| yamlInput: "key: ${UNDEFINED_KEY:-$${UNDEFINED_KEY}}", | ||
| // yamlOutput: `key: ${UNDEFINED_KEY:-${UNDEFINED_KEY}}`, | ||
| // this is what the spec tells, but I object, |
There was a problem hiding this comment.
pinging @mx-psi as he did a lot of the initial spec work. wdyt about this issue?
…14720) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Relates to open-telemetry/opentelemetry-go-contrib#8342 (comment)
4 participants
In the current implementation the environment variable substitution does not follow the specification found in https://opentelemetry.io/docs/specs/otel/configuration/data-model/. Specifically it does not obey the rule that substitution can only occur in YAML values. Further, according to the published specification examples, injected YAML-structured content leads to errors rather than correctly escaped/formatted strings.
This pull request solves these issues by using the yaml.Node structure of the yaml library to operate on already parsed YAML content. Further the redacted examples of the specification are included as further tests.