-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: bug_fix | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) | ||
component: confmap | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: When passing value to a string field through the `${<provider>:URI}` or `${URI}` syntax, the original raw string value is now preserved. | ||
|
||
# One or more tracking issues or pull requests related to the change | ||
issues: [10603] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | | ||
- This change only applies under the `confmap.unifyEnvVarExpansion` feature gate. | ||
|
||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,8 @@ import ( | |
|
||
"go.uber.org/zap" | ||
"gopkg.in/yaml.v3" | ||
|
||
"go.opentelemetry.io/collector/internal/featuregates" | ||
) | ||
|
||
// ProviderSettings are the settings to initialize a Provider. | ||
|
@@ -138,9 +140,14 @@ func NewRetrievedFromYAML(yamlBytes []byte, opts ...RetrievedOption) (*Retrieved | |
return nil, err | ||
} | ||
|
||
switch v := rawConf.(type) { | ||
switch rawConf.(type) { | ||
case string: | ||
opts = append(opts, withStringRepresentation(v)) | ||
if featuregates.UseUnifiedEnvVarExpansionRules.IsEnabled() { | ||
// If rawConf is a string, we override it with the raw yamlBytes. | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. This change is under the strict types gate There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
case int, int32, int64, float32, float64, bool: | ||
opts = append(opts, withStringRepresentation(string(yamlBytes))) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Let's add a row for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add |
||
| `!!str 0123` | string | !!str 0123 | 0123 | !!str 0123 | !!str 0123 | | ||
| `t` | boolean | true | true | Error: mapping string to bool | n/a | | ||
| `23` | boolean | true | true | Error: mapping integer to bool | n/a | |
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 usinghttp://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.
That would happen if you did
with
ENDPOINT
set to"http://localhost:4317"
(quotes included). The changes only affects how the values passed via a provider are parsed