use dataDir/config.yaml as the default config path #4466
Conversation
|
Looks good to me, with one question: # will use ~/.bacalhau/config.yaml
bacalhau config set api.port 2233
# will use hello.yaml
bacalhau config set --config hello.yaml api.port 2233
# will use customDir/config.yaml
bacalhau config set --data-dir customDir api.port 2233
# will use hello.yaml
bacalhau config set --config hello.yaml --data-dir customDir api.port 2233With this example at the end of the PR notes - if I were to run What I'm really asking here is, if I run |
No it wont. It will actually fail with I have updated the examples in the PR description to make it clearer |
|
OK, this makes sense to me. My understanding is as follows:
So, data dir Have I got that right? |
|
this seems solid to me. Just please use whatever is standard for norms with Viper! |
|
You lost me a little 😅
Not sure I understand this. If you run
data dir config.yaml IS the default config file, unless by "default config.yaml" you mean the hardcoded default configurations, then you are right. So in order of presedence:
What is worth clarifying is we DO NOT merge |
cmd/cli/config/set.go
Outdated
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| // initialize a new or open an existing repo. We need to ensure a repo | ||
| // exists before we can create or modify a config file in it. | ||
| _, err := util.SetupRepoConfig(cmd) | ||
| // Get the value of the --config flag | ||
| configFlag, err := cmd.Flags().GetString("config") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to setup repo: %w", err) | ||
| return fmt.Errorf("failed to get config flag: %w", err) | ||
| } | ||
| return setConfig(cmd.PersistentFlags().Lookup("config").Value.String(), args[0], args[1:]...) | ||
| }, | ||
| // provide auto completion for arguments to the `set` command | ||
| ValidArgsFunction: setAutoComplete, | ||
| } | ||
|
|
||
| bacalhauCfgDir := "bacalhau" | ||
| bacalhauCfgFile := config.DefaultFileName | ||
| // If --config flag is set, use it to set the Viper key | ||
| if configFlag != "" { | ||
| viper.Set(cliflags.RootCommandConfigFiles, []string{configFlag}) | ||
| } | ||
|
|
||
| usrCfgDir, err := os.UserConfigDir() | ||
| if err != nil { | ||
| log.Warn().Err(err).Msg("Failed to find user-specific configuration directory. Using current directory to write config.") | ||
| } else { | ||
| bacalhauCfgDir = filepath.Join(usrCfgDir, bacalhauCfgDir) | ||
| if err := os.MkdirAll(bacalhauCfgDir, util2.OS_USER_RWX); err != nil { | ||
| // This means we failed to create a directory either in the current directory, or the user config dir | ||
| // indicating a some-what serious misconfiguration of the system. We panic here to provide as much | ||
| // detail as possible. | ||
| log.Panic().Err(err).Msgf("Failed to create bacalhau configuration directory: %s", bacalhauCfgDir) | ||
| } | ||
| bacalhauCfgFile = filepath.Join(bacalhauCfgDir, bacalhauCfgFile) | ||
| var configPath string | ||
| // load configs to get the config file path | ||
| rawConfig, err := util.SetupConfigType(cmd) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to setup config: %w", err) | ||
| } | ||
|
|
||
| configPath = rawConfig.ConfigFileUsed() | ||
| if configPath == "" { | ||
| // we fall back to the default config file path $BACALHAU_DIR/config.yaml | ||
| // this requires initializing a new or opening an existing data-dir | ||
| bacalhauConfig, err := util.DecodeBacalhauConfig(rawConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to decode bacalhau config: %w", err) | ||
| } | ||
| _, err = util.SetupRepo(bacalhauConfig) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to setup repo: %w", err) | ||
| } | ||
| configPath = filepath.Join(bacalhauConfig.DataDir, config.DefaultFileName) | ||
|
|
||
| // create the config file if it doesn't exist | ||
| if _, err := os.Stat(configPath); os.IsNotExist(err) { | ||
| if err := os.WriteFile(configPath, []byte{}, util2.OS_USER_RWX); err != nil { | ||
| return fmt.Errorf("failed to create default config file %s: %w", configPath, err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return setConfig(configPath, args[0], args[1:]...) | ||
| }, | ||
| // Provide auto completion for arguments to the `set` command | ||
| ValidArgsFunction: setAutoComplete, | ||
| } | ||
|
|
||
| setCmd.PersistentFlags().String("config", bacalhauCfgFile, "Path to the config file") | ||
| setCmd.PersistentFlags().String("config", "", "Path to the config file (default is $BACALHAU_DIR/config.yaml)") | ||
| return setCmd |
There was a problem hiding this comment.
This can all be simplified a fair bit. setConfig will create the file if it doesn't already exist so no need to create one here.
| RunE: func(cmd *cobra.Command, args []string) error { | |
| // initialize a new or open an existing repo. We need to ensure a repo | |
| // exists before we can create or modify a config file in it. | |
| _, err := util.SetupRepoConfig(cmd) | |
| // Get the value of the --config flag | |
| configFlag, err := cmd.Flags().GetString("config") | |
| if err != nil { | |
| return fmt.Errorf("failed to setup repo: %w", err) | |
| return fmt.Errorf("failed to get config flag: %w", err) | |
| } | |
| return setConfig(cmd.PersistentFlags().Lookup("config").Value.String(), args[0], args[1:]...) | |
| }, | |
| // provide auto completion for arguments to the `set` command | |
| ValidArgsFunction: setAutoComplete, | |
| } | |
| bacalhauCfgDir := "bacalhau" | |
| bacalhauCfgFile := config.DefaultFileName | |
| // If --config flag is set, use it to set the Viper key | |
| if configFlag != "" { | |
| viper.Set(cliflags.RootCommandConfigFiles, []string{configFlag}) | |
| } | |
| usrCfgDir, err := os.UserConfigDir() | |
| if err != nil { | |
| log.Warn().Err(err).Msg("Failed to find user-specific configuration directory. Using current directory to write config.") | |
| } else { | |
| bacalhauCfgDir = filepath.Join(usrCfgDir, bacalhauCfgDir) | |
| if err := os.MkdirAll(bacalhauCfgDir, util2.OS_USER_RWX); err != nil { | |
| // This means we failed to create a directory either in the current directory, or the user config dir | |
| // indicating a some-what serious misconfiguration of the system. We panic here to provide as much | |
| // detail as possible. | |
| log.Panic().Err(err).Msgf("Failed to create bacalhau configuration directory: %s", bacalhauCfgDir) | |
| } | |
| bacalhauCfgFile = filepath.Join(bacalhauCfgDir, bacalhauCfgFile) | |
| var configPath string | |
| // load configs to get the config file path | |
| rawConfig, err := util.SetupConfigType(cmd) | |
| if err != nil { | |
| return fmt.Errorf("failed to setup config: %w", err) | |
| } | |
| configPath = rawConfig.ConfigFileUsed() | |
| if configPath == "" { | |
| // we fall back to the default config file path $BACALHAU_DIR/config.yaml | |
| // this requires initializing a new or opening an existing data-dir | |
| bacalhauConfig, err := util.DecodeBacalhauConfig(rawConfig) | |
| if err != nil { | |
| return fmt.Errorf("failed to decode bacalhau config: %w", err) | |
| } | |
| _, err = util.SetupRepo(bacalhauConfig) | |
| if err != nil { | |
| return fmt.Errorf("failed to setup repo: %w", err) | |
| } | |
| configPath = filepath.Join(bacalhauConfig.DataDir, config.DefaultFileName) | |
| // create the config file if it doesn't exist | |
| if _, err := os.Stat(configPath); os.IsNotExist(err) { | |
| if err := os.WriteFile(configPath, []byte{}, util2.OS_USER_RWX); err != nil { | |
| return fmt.Errorf("failed to create default config file %s: %w", configPath, err) | |
| } | |
| } | |
| } | |
| return setConfig(configPath, args[0], args[1:]...) | |
| }, | |
| // Provide auto completion for arguments to the `set` command | |
| ValidArgsFunction: setAutoComplete, | |
| } | |
| setCmd.PersistentFlags().String("config", bacalhauCfgFile, "Path to the config file") | |
| setCmd.PersistentFlags().String("config", "", "Path to the config file (default is $BACALHAU_DIR/config.yaml)") | |
| return setCmd | |
| // initialize a new or open an existing repo. We need to ensure a repo | |
| // exists before we can create or modify a config file in it. | |
| cfg, err := util.SetupRepoConfig(cmd) | |
| if err != nil { | |
| return fmt.Errorf("failed to setup repo: %w", err) | |
| } | |
| cfgFile := cmd.PersistentFlags().Lookup("config").Value.String() | |
| if cfgFile == "" { | |
| cfgFile = filepath.Join(cfg.DataDir, config.DefaultFileName) | |
| } | |
| return setConfig(cfgFile, args[0], args[1:]...) | |
| }, | |
| // provide auto completion for arguments to the `set` command | |
| ValidArgsFunction: setAutoComplete, | |
| } | |
| setCmd.PersistentFlags().String("config", "", "Path to the config file") | |
| return setCmd |
There was a problem hiding this comment.
few issues with this approach:
- a repo is going to be initialized and created unnecessarily even if the user define a config path. The repo can be well different than the one in user's config file as this
--configflag is different than the oneSetupRepoConfig()is basing on - we will create a config instance and log that a config was initialize using paths that can be different from one provided by the user as we are calling
SetupRepoConfig()before parsing--config setConfigis not creating the config file for us, and we don't want it to. We are only creating a config file if we are using the default path where we are creating config.yaml under the repo. Otherwise if the user provided a config file that doesn't exist, then we better fail instead of creating it to avoid undesired behavour with typos
Here are examples of this behaviour:
# config file with datadir setup
→ cat bacalhau_config.yaml
datadir: /Users/walid/test/config
# set a new entry in that config, you will see that repo is getting initialized
# at the default location instead of the one in the config file
→ bacalhau config set api.host 2.2.2.2 --config bacalhau_config.yaml
21:49:11.858 | INF ../../workspace/bacalhau/pkg/config/config.go:211 > Config loaded from: [], and with data-dir /Users/walid/.bacalhau
21:49:11.859 | INF ../../workspace/bacalhau/pkg/repo/fs.go:91 > Initializing repo at /Users/walid/.bacalhau
21:49:12.018 | INF ../../workspace/bacalhau/cmd/cli/config/set.go:49 > Writing config to bacalhau_config.yaml
There was a problem hiding this comment.
Sorry, you're right, the creation of the repo should happen on if a flag isn't provided:
func newSetCmd() *cobra.Command {
setCmd := &cobra.Command{
Use: "set",
Args: cobra.MinimumNArgs(2),
Short: "Set a value in the config.",
PreRunE: hook.ClientPreRunHooks,
PostRunE: hook.ClientPostRunHooks,
SilenceUsage: true,
RunE: func(cmd *cobra.Command, args []string) error {
cfgFile := cmd.PersistentFlags().Lookup("config").Value.String()
if cfgFile == "" {
// if the user didn't provide a flag, then ensure a repo is initialize and write/create file there.
cfg, err := util.SetupRepoConfig(cmd)
if err != nil {
return err
}
cfgFile = filepath.Join(cfg.DataDir, config.DefaultFileName)
}
return setConfig(cfgFile, args[0], args[1:]...)
},
// provide auto completion for arguments to the `set` command
ValidArgsFunction: setAutoComplete,
}
setCmd.PersistentFlags().String("config", "", "Path to the config file")
return setCmd
}
func setConfig(cfgFilePath, key string, value ...string) error {
v := viper.New()
v.SetConfigFile(cfgFilePath)
if err := v.ReadInConfig(); err != nil {
if !os.IsNotExist(err) {
return err
}
}
parsed, err := types.CastConfigValueForKey(key, value)
if err != nil {
return err
}
v.Set(key, parsed)
if err := v.WriteConfigAs(cfgFilePath); err != nil {
return err
}
return nil
}There was a problem hiding this comment.
As far as 21:49:11.858 | INF ../../workspace/bacalhau/pkg/config/config.go:211 > Config loaded from: [], and with data-dir /Users/walid/.bacalhau
I think we should only produce this log line when we have loaded a config from a file
There was a problem hiding this comment.
First, I don't like that we are forking how config path is resolved in set command compared to others. I tried to minimize the fork by still calling SetupConfig the same as how all other methods do. If in the future we introduce a new logic on how config path is resolved, such as introducing an env variable, we wont fall in bugs where we miss to update the code here as well. It is getting late here. I have an idea to reuse the same config flags that I'll push tomorrow.
Second, we still have to create the config file if it is coming from the repo
Last, I have to disagree about the logging. Even if configs are not loaded from a file because we couldn't fine one or the user didn't provide one, we should still log that to help them understand what is going on
There was a problem hiding this comment.
Okay, my goal with these comments is to get use to simplify things, my approval is sticky, so merge when you are satisfied.
There was a problem hiding this comment.
I've made changes here to make sure we are using the same --config flag for all commands, both for reading and writing it. Config initialization is now as unified as it can get using util.SetupConfig() that relies on that flag
| if cf.isWriteMode { | ||
| // Check if a config file is already set in Viper | ||
| if viper.GetString(RootCommandConfigFiles) != "" { | ||
| return fmt.Errorf("single config file can be set") | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this how we are enforcing that the flag may only be used once with the config set command?
A recent change in behaviour has caused some confusion where the default config path changed from
repo/config.yamlto~/.config/bacalhau/config.yaml. This PR reverts this change and continue using aconfig.yamlunder the repo/dataDir if present and if no explicit config file was provided by the user through--configflag.The assumption that many users have is deleting the data directory (e.g.
~/.bacalhau), or pointing to a fresh directory using$BACALHAU_DIRenv variable or--data-dirflag, is reseting the state and configuration of the node or client. However, this wasn't the case as the default config path was outside the data directory.Before this PR
This is an example where issues could happen:
After this PR
Different Combinations
with the upcoming v1.5.0 release, users are able to define explicit configuration files using
--configflag, as well as the existing--data-dirflag to define where bacalhau stores its state. These are also configurable using env variables.The default
data-dirif none is specified is~/.bacalhau, and the default config path is$(dataDir)/config.yaml. Lets explore what that means in actionSetting config
Applying config
bacalhau servehas the same flags asbacalhau config set, and the same logic and precedence applies