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.

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

Should find config file which with extension first. #844

Open
SmallTianTian opened this issue Feb 18, 2020 · 4 comments
Open

Should find config file which with extension first. #844

SmallTianTian opened this issue Feb 18, 2020 · 4 comments

Comments

@SmallTianTian
Copy link

package main

import (
	"fmt"
	"os"

	"github.com/spf13/viper"
)

func main() {
	viper.SetConfigName("main")
	viper.SetConfigType("yaml")
	viper.AddConfigPath(".")
	viper.AddConfigPath("/tmp")

	if err := viper.ReadInConfig(); err != nil {
		fmt.Println(err)
		os.Exit(1)
	}

	fmt.Println("File path:", viper.ConfigFileUsed())
}

When it's a go file, it can read main.yaml for config file. But when you build this go file(main), you will find the executable file will be reading first. I'm set config type and config name, but I couldn't get this file.
I've read #818, extension-less filename is neeeded. So, I think the extension config file should be high weight.

@wfernandes
Copy link

I'm running into a similar issue.
I have the file ~/.myconfdir/myconf.yaml
When I do

viper.SetConfigName("myconf")
viper.AddConfigPath("~/.myconfdir")

And then call viper.ConfigFileUsed() I get an empty string because it doesn't set v.configFile.

I propose that the following suggestion

func (v *Viper) ConfigFileUsed() string {
  file, _ := v.getConfigFile()
  return file
}

The current implementation is

func (v *Viper) ConfigFileUsed() string {
  return v.configFile
}

@winder
Copy link

winder commented Oct 27, 2020

I noticed this issue today as well, but don't really like the proposed solution. Some users may not have their config file be the same as the binary, in which case it's completely reasonable to want/expect a config file without an extension in the CWD/bindir to override some system config.

Another way to do this is to ignore executable files. There isn't a great way to do this in Windows, but on that system the binary generally has the .exe extension so I think this is a general solution:
viper/util.go

  func exists(fs afero.Fs, path string) (bool, error) {
  	stat, err := fs.Stat(path)
  	if err == nil {
- 		return !stat.IsDir(), nil
+ 		if stat.IsDir() {
+ 			return false, nil
+ 		}
+ 		if stat.Mode().Perm()&0111 != 0 {
+ 			return false, nil
+ 		}
  	}
  	if os.IsNotExist(err) {
  		return false, nil
  	}
  	return false, err
  }

@bearsh
Copy link

bearsh commented Oct 5, 2021

another way would be to compare it against the path of the application's binary

@drsybren
Copy link

Thanks @SmallTianTian for writing this report. I also had issues with the way Viper tries to find a suitable config file to load.

I noticed this issue today as well, but don't really like the proposed solution. Some users may not have their config file be the same as the binary, in which case it's completely reasonable to want/expect a config file without an extension in the CWD/bindir to override some system config.

I think this is something important to keep in mind.

IMO it would be best to have explicit control over this. As Viper v1 is currently built (I haven't looked at v2 yet), the viper.SetConfigType("yaml") has various effects (bold to aid in quickly scanning the text):

  • When loading config, this is used to figure out the intended format for situations that have no extension (because the filename is extensionless, or because the data comes from some nameless byte stream).
  • When loading config, this is also used to allow searching for extensionless files (because now their expected type is known).
  • When writing config, this sets which file type is used, and which extension to give the written file (if there is one).

It is this multi-purpose nature of the setting that makes it confusing for me. When writing the configuration, I need to set the config type. But when trying to read it, I have to clear it again, otherwise Viper tries to load the (extensionless) executable as if it were configuration.

On top of this, there is the confusion stemming from the file-finding implementation, which doesn't prioritise the set configuration type. I had really weird issues when I first used JSON and then switched to YAML, only to find that Viper would still try and load the JSON file first. IMO Viper should look at the configured file type before anything else.

My proposal is thus two-fold:

  1. Add something like func (v *Viper).FindExtensionlessFiles(allowed boolean), wrapped with a viper.Option for convenience. Only when that is set to true, try finding extensionless files. Of course this can be true by default to help with backward compatibility.
  2. Search for the configured file type, before iterating over viper.SupportedExts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants