(config): default path set to `./` not be suitable for all usecases
Closed this issue · 6 comments
In the config package the default path to load configuration from is set to ./
at https://github.com/odpf/salt/blob/main/config/config.go#L135
Which means that by default if the config is available in the ./
directory it would always load from that first.
This is not be suitable for all use cases as people might want different search order, and can cause confusion when trying to set more config paths.
While currently there is a way to override this its too cumbersome, that is to inject a custom viper instance and set the other defaults on it manually.
Removing this default and letting users use config.WithPathOption
for the same will make it more clear to the users.
@rohilsurana I do see the concern, and yes, injecting a custom viper
instance would be a painful task, but I don't get the usage of config.WithPathOption
. I guess you're trying to refer config.WithConfigPath
, right?
If yes, please do let me know, and I'll create a PR for the same :)
@burntcarrot Thanks! That's right, it was a typo. I was trying to refer to the config.WithConfigPath
.
Assigned the issue to you. 👍
I guess we should update config/README.md
to indicate the behavior change produced by this PR., that is, config.NewLoader
would not be having a default path for loading configuration, and instead the user needs to specify the config path using config.WithConfigPath
.
I propose uncommenting out the lines present in config/README.md:
l := config.NewLoader(
// config.WithConfigName("config"),
// config.WithConfigPath("$HOME/.test"),
config.WithEnvPrefix("CONFIG"),
)
to:
l := config.NewLoader(
config.WithConfigName("config"),
config.WithConfigPath("$HOME/.test"),
config.WithEnvPrefix("CONFIG"),
)
``
@rohilsurana If you want me to proceed with the proposed changes, I'll add these changes to the PR.
I also propose to add a disclaimer/note section below the Usage section in config/README.md
, which notifies about the changes, and recommends the user to use config.WithConfigPath
while using config.NewLoader
.
For the readme update I was thinking more like -
l := config.NewLoader(
// config.WithViper(viper.New()), // default
// config.WithConfigName("config"), // default
// config.WithConfigType("yaml"), // default
// config.WithEnvKeyReplacer(".", "_"), // default
config.WithConfigPath("$HOME/.test"),
config.WithEnvPrefix("CONFIG"),
)
This way we are also documenting the default values for the options. Only the config.WithEnvPrefix
remains with non-default value, but wanted to document its usage later in the env configs list hence its like this.
Don't think we need the disclaimer as of now.
The config package would basically just not load from any file and only from env variables if that option is missed.
Understandable. Made the changes, would love a review on the PR, as I think we're good to go :)