fix: Allow overriding env variable names, restore CODER_TELEMETRY #4894
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`.
48d852b to
6408959
Compare
There was a problem hiding this comment.
I agree this code here would solve this problem, looks great.
What wasn't documented clearly was that we are OK with these breaking changes and planned on documenting them in some sort of release notes. I know @bpmct was aware of the changes, even if this one was missed in the initial pass we knew about it pretty quickly from dogfood breaking.
I'd prefer we just accept the one time breaking changes at this early stage in the product for a system that doesn't need to support exceptions.
Ah, I see. I do understand your wish to have no exceptions although I feel it will be a necessity down the line, as the product evolves. At some point we will almost certainly rename or restructure something in the config. At which point being able to override the name (just like with a flag) would be good. We could even turn Env into an array to support multiple or allowing for backwards compat. A nice added bonus here is that the API will inform the frontend of the name used for the env var. This is something I think we’d want even if we didn’t support renaming envs? |
Yup I totally agree we'll probably run into this case I'd just like to wait until then to do so. The conversion from config field to env var is deterministic so the frontend has the information to present to env var currently, even if we we don't explicitly send it back as a field. |
Alright, I'll close this down for now then 👍🏻 |
This change allows us to set a custom env for certain flags. Useful when it does not match the deployment config struct 1-to-1.
This also restores the
CODER_TELEMETRYname (was changed toCODER_TELEMETRY_ENABLEfor the last release or two).NOTE: This PR is targets #4893 as it builds upon the change there.