Fix issues with config file/directory logic#20
Conversation
| defaultConfigFile := filepath.Join(config.GetConfigDir(), config.ConfigFileName) | ||
| cmd.PersistentFlags().StringVar(&cfgFile, "config", defaultConfigFile, "config file") |
There was a problem hiding this comment.
I changed the flag from --config (which accepted the full file path to the config file) to --config-dir (which accepts the path to the directory containing the config file) to match the TIGER_CONFIG_DIR env var, and to make it clear that this config directory is used for both the config.yaml file as well as the API key fallback file. This is also in-line with the original spec.
I also updated it to use GetDefaultConfigDir as the default value (which is shown in the help output), which returns the "true" default value whether or not TIGER_CONFIG_DIR is set.
| // Configure viper to watch for file changes and update its in-memory | ||
| // representation of the config. Note that this won't automatically | ||
| // update [Config] structs already returned from [Load]. | ||
| viper.WatchConfig() |
There was a problem hiding this comment.
This ensures that config file changes made while the MCP server is running take effect immediately (i.e. for the next tool call). This depends on the fact that the config is re-loaded via a call to Load() for each MCP tool call.
| ConfigDir: GetConfigDir(), | ||
| } | ||
|
|
||
| // Use the global Viper instance that's already configured by initConfig() and bindFlags() |
There was a problem hiding this comment.
Neither of these function (initConfig() or bindFlags()) exist anywhere in the code base. Not sure where this comment came from, but I updated it to refer to the SetupViper function above, which seems most accurate.
| return expandPath(dir) | ||
| } | ||
| return filepath.Dir(viper.ConfigFileUsed()) | ||
| } |
There was a problem hiding this comment.
This function was being called to construct the file path that the API key is stored in when the keychain is not available (e.g. here), but was not taking the flag into consideration. I redefined it to accurately reflect the value of the flag (if provided), or env var, or default. It would probably be better to pass the config struct into those functions and use the ConfigDir field, but that's a bit of a larger refactor I don't want to undertake at the present moment.
nathanjcochran
force-pushed
the
nathan/fix-config-flag
branch
from
a91b349 to
efe3b68
Compare
nathanjcochran
force-pushed
the
nathan/fix-config-flag
branch
from
efe3b68 to
879d329
Compare
| func GetEffectiveConfigDir(configDirFlag *pflag.Flag) string { | ||
| if configDirFlag.Changed { | ||
| return expandPath(configDirFlag.Value.String()) | ||
| } | ||
|
|
||
| if dir := os.Getenv("TIGER_CONFIG_DIR"); dir != "" { | ||
| return expandPath(dir) | ||
| } | ||
|
|
||
| return GetDefaultConfigDir() | ||
| } |
There was a problem hiding this comment.
This new function is responsible for returning the --config-dir flag value (if set), or the TIGER_CONFIG_DIR env var value (if set), or the default value.
| } | ||
| } | ||
|
|
||
| func TestStoreAPIKeyToFile(t *testing.T) { |
There was a problem hiding this comment.
Moved into ./config/api_key_test.go. Should have moved these tests when I moved the corresponding functions in #12.
| if debug { | ||
| if configFile := viper.ConfigFileUsed(); configFile != "" { | ||
| fmt.Fprintln(os.Stderr, "Using config file:", configFile) | ||
| } | ||
| } |
There was a problem hiding this comment.
This was duplicative with the information logged here, and it was generating a lot of noise in the go test output.
nathanjcochran
force-pushed
the
nathan/fix-config-flag
branch
from
79aa4ca to
a7fcf5d
Compare
2 participants
This PR fixes several issues with the config file/directory logic:
--configflag).--configflag was being provided byGetConfigDir()(see here). That function returned either the value provided via theTIGER_CONFIG_DIRenv var or the default value. As a result, the flag's default value (i.e. in the help text) would change if theTIGER_CONFIG_DIRenv var was present, which is not how the defaults for any of the other flags work if their corresponding env vars are set (they always show the "true" default value).--configflag did not match the naming convention of theTIGER_CONFIG_DIRenv var. Furthermore, the CLI flag expected a full file path, whereas the env var just expected the path to the directory. This was inconsistent with the other flags, which all have a 1:1 correspondence with their env vars.GetConfigDir()function to get the path to the config dir where the API key file is stored (e.g. see here). However, that meant there was no way to control this via a flag, since that function did not take the flag into account (only theTIGER_CONFIG_DIRenv var).I also fixed and cleaned up a bunch of tests, and added some new tests to cover some of these issues and prevent them from happening again.