-
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] Fix for unset env var setting non-string field. #10950
[confmap] Fix for unset env var setting non-string field. #10950
Conversation
This doesn't work, the reflected default value ends up being used instead of the config's default value. I think we'd need |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10950 +/- ##
==========================================
- Coverage 91.88% 91.87% -0.01%
==========================================
Files 411 411
Lines 19327 19330 +3
==========================================
+ Hits 17759 17760 +1
- Misses 1218 1219 +1
- Partials 350 351 +1 ☔ View full report in Codecov by Sentry. |
Maybe that is ok? In the string situation the value would be getting set to "" which would also override any default string value. So exporters:
debug:
verbosity: ${env:UNSET_VAR} would be seen as exporters:
debug:
verbosity: "" Not exporters:
debug:
verbosity: "basic" We log a warning when an unset env var is detected. |
I've confirmed that prior to this regression the unset env var would result in the field being set with the type's default value, not the default value from |
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.
LGTM, is there any way we can reproduce the panic in a unit test?
This test in func TestIssue10949_UnsetVar(t *testing.T) {
resolver := NewResolver(t, "types_expand.yaml")
conf, err := resolver.Resolve(context.Background())
require.NoError(t, err)
var cfg TargetConfig[int]
err = conf.Unmarshal(&cfg)
require.NoError(t, err)
require.Equal(t, 0, cfg.Field)
} (edit: Also tested on 1c96225 (last commit before the panic happens) and confirmed that this the behavior we had before) Could you add this/something like this as a test? |
@mx-psi added, thank you! |
Description
Still working on making a new test for this scenario, but all existing tests pass.
Link to tracking issue
Fixes #10949
Testing
Manual testing showed the bug resolved, but I'd still like to get an e2e test to check it.