Skip to content
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.

Sign up for GitHub

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

Jump to bottom

# Load env variables into KwildConfig #705

Merged
merged 1 commit into from
May 8, 2024
Merged

# Load env variables into KwildConfig #705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 38 additions & 6 deletions cmd/kwild/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,15 +463,28 @@ func LoadConfigFile(configPath string) (*KwildConfig, error) {

// LoadEnvConfig loads a config from environment variables.
func LoadEnvConfig() (*KwildConfig, error) {
viper.SetEnvPrefix("KWILD")
viper.AutomaticEnv()

var cfg KwildConfig
if err := viper.Unmarshal(&cfg); err != nil {
// Manually bind environment variables to viper keys.
for _, key := range viper.AllKeys() {
// 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, ".", "_"))
Comment on lines +468 to +474
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bummer, so is this above now pointless?

viper.SetEnvPrefix("KWILD")
viper.AutomaticEnv()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@charithabandi charithabandi May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
	*/```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jchappelow jchappelow May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm, tried it out, didn't work for me.

viper.BindEnv(bindKey, envKey)
}

// var cfg KwildConfig, won't work because, viper won't be able to extract
// the heirarchical keys from the config structure as fields like cfg.app set to nil.
// It can only extract the first level keys [app, chain, log] in this case.
// To remedy that, we use DefaultEmptyConfig with all the sub fields initialized.
cfg := DefaultEmptyConfig()
if err := viper.Unmarshal(cfg); err != nil {
return nil, fmt.Errorf("decoding config: %v", err)
}

return &cfg, nil
return cfg, nil
}

var ErrConfigFileNotFound = fmt.Errorf("config file not found")
Expand All @@ -481,6 +494,25 @@ func fileExists(path string) bool {
return !os.IsNotExist(err)
}

// DefaultEmptyConfig returns a config with all fields set to their zero values.
// This is used by viper to extract all the heirarchical keys from the config
// structure.
func DefaultEmptyConfig() *KwildConfig {
return &KwildConfig{
AppCfg: &AppConfig{
Extensions: make(map[string]map[string]string),
},
ChainCfg: &ChainConfig{
P2P: &P2PConfig{},
RPC: &ChainRPCConfig{},
Mempool: &MempoolConfig{},
StateSync: &StateSyncConfig{},
Consensus: &ConsensusConfig{},
},
Logging: &Logging{},
}
charithabandi marked this conversation as resolved.
Show resolved Hide resolved
}

func DefaultConfig() *KwildConfig {
return &KwildConfig{
AppCfg: &AppConfig{
Expand Down
2 changes: 2 additions & 0 deletions cmd/kwild/root/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/kwilteam/kwil-db/internal/version"

"github.com/spf13/cobra"
"github.com/spf13/viper"
)

func RootCmd() *cobra.Command {
Expand Down Expand Up @@ -89,6 +90,7 @@ func RootCmd() *cobra.Command {
flagSet := cmd.Flags()
flagSet.SortFlags = false
config.AddConfigFlags(flagSet, flagCfg)
viper.BindPFlags(flagSet)

flagSet.BoolVarP(&autoGen, "autogen", "a", false,
"auto generate private key, genesis file, and config file if not exist")
Expand Down
Loading