# Load env variables into KwildConfig #705
Conversation
charithabandi
changed the title
| // Replace dashes with underscores in the key to match the flag name. | ||
| // This is required because there is inconsistency between our flag names | ||
| // and the struct tags. The struct tags use underscores, but the flag names | ||
| // use dashes. Viper uses the flag names to bind environment variables | ||
| // and this conversion is required to map it to the struct fields correctly. | ||
| bindKey := strings.ReplaceAll(key, "-", "_") | ||
| envKey := "KWILD_" + strings.ToUpper(strings.ReplaceAll(bindKey, ".", "_")) |
There was a problem hiding this comment.
Bummer, so is this above now pointless?
viper.SetEnvPrefix("KWILD")
viper.AutomaticEnv()
There was a problem hiding this comment.
viper.AutomaticEnv() would never have because of maps of interfaces in the comet's config. So we have to do this anyways. I had commented about it here
There was a problem hiding this comment.
Just pasting the comment here for reference:
/*
Lots of Viper magic here, but the gist is:
We want to be able to set config values via
- flags
- environment variables
- config file
- default values
for env variables support:
Requirement is, we need to be able to config from env variables with a prefix "KWILD_"
It can be done 2 ways:
1. AutomaticEnv: off mode
- This will not bind env variables to config values automatically
- We need to manually bind env variables to config values (this is what we are doing currently)
- As we bound flags to viper, viper is already aware of the config structure mapping,
so we can explicitly call viper.BindEnv() on all the keys in viper.AllKeys()
- else we would have to reflect on the config structure and bind env variables to config values
2. AutomaticEnv: on mode
- This is supposed to automatically bind env variables to config values
(but it doesn't work without doing a bit more work from our side)
- One way to make this work is add default values using either viper.SetDefault() for all the config values
or can do viper.MergeConfig(<serialized config>)
- Serializing is really painful as cometbft has a field which is using map<interface><interface> though its deprecated.
which prevents us from doing the AutomaticEnv binding
Issues referencing the issues (or) correct usage of AutomaticEnv: https://github.com/spf13/viper/issues/188
For now, we are going with the first approach
Note:
The order of preference of various modes of config supported by viper is:
explicit call to Set > flags > env variables > config file > default values
*/```There was a problem hiding this comment.
I thought that map[string]any issue is gone since we made all of our own structs instead of using cometbft's.
Anyway, I don't understand the viper magic at all, so whatever works is fine. Remove those two lines and comment on why they don't have any purpose?
There was a problem hiding this comment.
Oh yeah! I totally forgot about that, Yeah, should be doable with automatic env, with serialization. But its still broken bcz of the inconsistency in the tags.
There was a problem hiding this comment.
I think your solution is fine, but have a look at viper.SetEnvKeyReplacer. I think it's built for this purpose like viper.SetEnvKeyReplacer(strings.NewReplacer("_", "-")). But I really am just guessing here, not a test!
EDIT: probably viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_", "-", "_")) // --app.output-paths => KWILD_APP_OUTPUT_PATHS but meh
There was a problem hiding this comment.
umm, tried it out, didn't work for me.
|
After this goes in a backport PR for v0.7.5 we can re-open #678 I'll do a quick test on this PR now but it looks fine. |
Env variables are not supported since we renamed the cmd line flags to have hyphens or dashes instead of underscores. Whereas the struct tags remained to use dashes to support config file parsing. Therefore manual binding is needed to map the env variables correctly to the structures.