fix(config): ensure all expected env vars are bound #1113
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While pairing with @markphelps yesterday, we discovered some fields were not getting set via
env
variables.This appears to be a regression, since the refactor I did to leverage
viper.Unmarshal
.Upon investigating I stumbled across spf13/viper#761 which gave lots of insight into what we're seeing.
Initially, this PR adds a test which runs over each case already in
TestLoad
.Instead of just checking loading from YAML works, it also builds up a set of env var keys based on these config files.
Then, using these keys, it runs the test a second time.
This time, configuring the environment to match the same content of the YAML.
Each case added to
TestLoad
will attempt to validate both the config YAML and equivalent environment variables, to produce the same config result.If I run this test without the accompanying fix, I get:
The PR also delivers on the fix. It follows one of the strategies outlined in the issue above.
One way to attempt to fix this is via
SetDefault()
for all potential fields.This has two problems:
SetDefault
is misbehaving sometimes, withIsSet
leading to faults in thedb
config.The better approach is to use both
viper.SetDefault
for deprecated keys andviper.BindEnv
with every key that has a presence in the unmarshal config struct.This PR does just that. It extends the reflection code used to support adding defaults and validation methods to config structs. It descends into each struct and registers each struct tag appropriately via
BindEnv
.Now the tests are passing appropriately.