raystack/salt

(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 :)