fix: Disable viper auto-env to avoid assigning to parent structs #4893
Conversation
The viper automatic env mapping and BindEnv were both creating mappings
like `vip.BindEnv("telemetry", "CODER_TELEMETRY")` which we don't want
since `DeploymentConfig.Telemetry` is a struct housing fields.
For some reason, this was causing `DeploymentConfig.Telemetry.URL` to
**not** be assigned its default value when `CODER_TELEMETRY=false` was
set as an environment variable.
Potentially we would want `"telemetry.enable"` to be mapped to
`"CODER_TELEMETRY"` for simplicity. But that behavior is not changed by
this commit.
Arguably, we could remove `vip.SetEnvPrefix` and `vip.SetEnvKeyReplacer`
as well since we're manually controlling all environment variable names
via `formatEnv`.
| @@ -370,7 +370,6 @@ func Config(flagset *pflag.FlagSet, vip *viper.Viper) (*codersdk.DeploymentConfi | |||
| return nil, xerrors.Errorf("get global config from flag: %w", err) | |||
| } | |||
| vip.SetEnvPrefix("coder") | |||
| vip.AutomaticEnv() | |||
There was a problem hiding this comment.
Have you reviewed open issues for the spf13/viper? There are a few open regarding vip.AutomaticEnv. Maybe it's a bug you could raise with the team.
There was a problem hiding this comment.
Not really, I thought it'd be good for us to manually control these (we already had all the code in place to do it). But it's a good suggestion, I did a quick browse but nothing stood out. I also just pushed a follow-up PR for this: #4894 in support of not enabling AutomaticEnv.
There was a problem hiding this comment.
This might be related, but it's not the exact same issue. Still, looks like a rabbit hole I don't want to dive into 😄 spf13/viper#1243
There was a problem hiding this comment.
Good for me, thanks for reviewing those issues! I suspected that this might be a rabbit hole hence the idea of covering it with a basic unit test. It's up to you to decide if you prefer to open a new issue for viper or leave it as it :)
| // Ensure that we only bind env variables to proper fields, | ||
| // otherwise Viper will get confused if the parent struct is | ||
| // assigned a value. |
There was a problem hiding this comment.
Thanks for writing the comment here. I'm afraid that we may run into this issue again in the future (with library bump). Do you think that it's possible to prepare a basic unit test to validate the behavior?
There was a problem hiding this comment.
I'm afraid 2f4ac30 (#4893) is the best I can do. I didn't find too many ways to reproduce the issue.
The viper automatic env mapping and BindEnv were both creating mappings
like
vip.BindEnv("telemetry", "CODER_TELEMETRY")which we don't wantsince
DeploymentConfig.Telemetryis a struct housing fields.For some reason, this was causing
DeploymentConfig.Telemetry.URLtonot be assigned its default value when
CODER_TELEMETRY=falsewasset as an environment variable.
Potentially we would want
"telemetry.enable"to be mapped to"CODER_TELEMETRY"for simplicity. But that behavior is not changed bythis commit.
Arguably, we could remove
vip.SetEnvPrefixandvip.SetEnvKeyReplaceras well since we're manually controlling all environment variable names
via
formatEnv.