-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[confmap] Use original string value on expansion #10603
Conversation
// This is the original ${ENV} expansion behavior. | ||
rawConf = string(yamlBytes) | ||
} | ||
opts = append(opts, withStringRepresentation(string(yamlBytes))) |
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.
This change is under the strict types gate
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10603 +/- ##
=======================================
Coverage 92.26% 92.26%
=======================================
Files 395 395
Lines 18703 18705 +2
=======================================
+ Hits 17257 17259 +2
Misses 1086 1086
Partials 360 360 ☔ View full report in Codecov by Sentry. |
value: "\"0123\"", | ||
targetField: TargetFieldInt, | ||
expected: 83, | ||
expected: "\"0123\"", |
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.
This breaks users right?
If I entered endpoint: "http://localhost:4317"
instead of my exporter using http://localhost:4317
would it attempt to use a value that contains double-quote literals?
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.
If I entered endpoint: "http://localhost:4317" instead of my exporter using http://localhost:4317 would it attempt to use a value that contains double-quote literals?
That would happen if you did
endpoint: ${env:ENDPOINT}
with ENDPOINT
set to "http://localhost:4317"
(quotes included). The changes only affects how the values passed via a provider are parsed
If we were to do this we would have to sync with the OpenTelemetry Config WG to see how this lines up with their behavior. I am still not sure if this is a good idea or not |
// This is the original ${ENV} expansion behavior. | ||
rawConf = string(yamlBytes) | ||
} | ||
opts = append(opts, withStringRepresentation(string(yamlBytes))) |
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.
This changes the behavior when the feature gate is disabled. Before it would set the string representation to be the yaml parsed version of the string (so \n
is removed.
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.
Yup, but the string representation is only used under the strict types feature gate, so it's also under a feature gate
@@ -216,7 +216,7 @@ loading a field with the braces syntax, `env` syntax. | |||
| `0123` | integer | 83 | 83 | 83 | n/a | | |||
| `0123` | string | 0123 | 83 | 0123 | 0123 | | |||
| `0xdeadbeef` | string | 0xdeadbeef | 3735928559 | 0xdeadbeef | 0xdeadbeef | | |||
| `"0123"` | string | "0123" | 0123 | 0123 | 0123 | | |||
| `!!str 0123` | string | !!str 0123 | 0123 | 0123 | 0123 | | |||
| `"0123"` | string | "0123" | 0123 | "0123" | "0123" | |
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.
Let's add a row for foo\nbar
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.
Add foo::
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Adds more e2e tests to match main's current behavior. Some of these are weird, and could be used to justify #10603 <!-- Issue number if applicable --> #### Link to tracking issue Related to #10603
I have been mulling over this over the weekend and I don't think this is a good idea, since it completely prevents users from escaping content to say "this is a string". A better solution would be to preserve the original string value and use it while unmarshaling with a decoding hook if the target field is a string. I have tried and failed to implement this (not saying it can't be done, but it's definitely messy to do) |
#10618 is a more complex but more sensible version of this |
Closing this in favor of #10618 |
Description
Originally on the env var RFC I proposed using the YAML parsed value when parsing a string, which has been the default behavior until now of
${env:ENV}
and friends.An alternative is to use the raw string value, which is what
${ENV}
used to do. The difference (after enabling strict types) does not matter much in practice save for a few edge cases (since strings can be represented in multiple ways), namely on the YAML parsing scenario:"opentelemetry"
->opentelemetry
)!!str opentelemetry
->opentelemetry
)It's a matter of opinion which behavior is more intuitive to end users. In this PR I change the behavior to use the raw string value. I think the arguments in favor are:
Link to tracking issue
Fixes #10405 (comment)