Support spaces in environment variable lists #2011
Conversation
| values = if value.include?(',') || comma_separated_only | ||
| value.split(',') | ||
| else | ||
| value.split # space splitting is the default |
There was a problem hiding this comment.
Minor: In the spirit of self-documenting code,
| value.split # space splitting is the default | |
| value.split(' ') |
is faster to read than reading the comment ;)
There was a problem hiding this comment.
Rubocop yelled at me for having the parameter that matches the default, so I removed it.
I'll add it back.
There was a problem hiding this comment.
Ahh Rubocop... That extremely opinionated colleague that we know nothing about :)
| # either trailing or leading are trimmed. | ||
| # | ||
| # Empty entries, after trimmed, are also removed from the result. | ||
| def env_to_list(var, default = [], comma_separated_only: false) |
There was a problem hiding this comment.
Minor: Would it perhaps make sense to make this a required argument, to make callers explicitly think about if they need commas or spaces for separation?
| def env_to_list(var, default = [], comma_separated_only: false) | |
| def env_to_list(var, default = [], comma_separated_only:) |
| context '1,2 3' do | ||
| let(:env_value) { '1,2 3' } | ||
|
|
||
| it { is_expected.to eq(['1', '2 3']) } | ||
| end |
There was a problem hiding this comment.
To be honest, I'm not sure how useful it is to support this, rather than just doing split by space and comma at the same time and be done with it. But it is true that the Python implementation also does some of these hijinks so...
(We're not actually matching the exact hijinks with spaces in python, e.g. (" key: val,key2:val2", {" key": " val", "key2": "val2"}, None),, but again, I'd just go for splitting on both at the same time and not supporting weird mixes).
There was a problem hiding this comment.
I'm following Go's implementation, Java's implementation, and Python's original implementation document (internal doc):
- Looking for commas, and using comma-separated if so.
I'm trying to match their implementations, to keep consistency, instead of doing what seems more right here.
Codecov Report
@@ Coverage Diff @@
## master #2011 +/- ##
==========================================
- Coverage 97.44% 97.44% -0.01%
==========================================
Files 1016 1016
Lines 51474 51499 +25
==========================================
+ Hits 50160 50184 +24
- Misses 1314 1315 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Fixes #1953
This PR adds space-separated list support, in addition to the existing comma-separated list, to environment variable list options.
This brings us up to parity with other traces.
The only application option currently is
DD_TAGS.Initially, I implemented this change to all users of
Datadog::Core::Environment::VariableHelpers#env_to_list, but the other use case,DD_PROPAGATION_STYLE_*, has a space in one of its supported values.It wouldn't be possible to configure
DD_PROPAGATION_STYLE_INJECT=B3 single headercorrectly, so I leftDD_PROPAGATION_STYLE_*as comma-separated only in this PR.