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

^1.6: Unsupported Config Type "" when config file not found and falling back to extension-less filename #818

Closed
andig opened this issue Dec 25, 2019 · 12 comments
Labels
kind/bug Something isn't working

Comments

@andig
Copy link
Contributor

andig commented Dec 25, 2019

1.6.0 has introduced a breaking change that will lead to failure if not config file is found. I used to do this:

// If a config file is found, read it in
if err := viper.ReadInConfig(); err == nil {
	// using config file
	cfgFile = viper.ConfigFileUsed()
} else if _, ok := err.(viper.ConfigFileNotFoundError); !ok {
	// parsing failed - exit
	fmt.Println(err)
	os.Exit(1)
} else {
	// not using config file
	cfgFile = ""
}

Starting with 1.6.0, viper.ReadInConfig will return

Unsupported Config Type ""

if the config file does not exist. This is highly confusing as it should really be ConfigFileNotFoundError instead of UnsupportedConfigError. Please advise.

andig added a commit to andig/mbmd that referenced this issue Dec 25, 2019
@sagikazarmark
Copy link
Collaborator

@andig unfortunately I cannot reproduce the issue with your code sample.

I tried to simplify it a little bit:

package main

import (
	"fmt"

	"github.com/spf13/viper"
)

func main() {
	viper.AddConfigPath(".")
	viper.SetConfigName("config")

	// If a config file is found, read it in
	if err := viper.ReadInConfig(); err != nil {
		fmt.Println(err)
	}
}

But I'm unable to reproduce it with 1.6.0 and 1.6.1.

There was indeed a breaking change introduced in 1.6.0, which the above code sample causes to return Unsupported Config Type: when there is a directory with the same name (eg. config is the config name and there is a directory with that name). But that was fixed in 1.6.1.

Please try to provide a breaking test, or a simple code sample that reproduces the issue. It might have environmental components (eg. existing file/directory, envorinment variable, etc), so please take those into account as well (eg. what config file are you trying to read? is there a directory with the same name?, etc).

@sagikazarmark
Copy link
Collaborator

@andig any chance you can provide an example reproducing this bug?

@andig
Copy link
Contributor Author

andig commented Dec 29, 2019

Will do but need some time for debugging to find the faulty case.

@andig
Copy link
Contributor Author

andig commented Dec 30, 2019

Closing this one. I have the sneaking suspicion that I had a cobra PersistentFlags().StringVarP in somewhere for an unrelated option pointing to the config file setting. Please forgive me :/

@andig andig closed this as completed Dec 30, 2019
@sagikazarmark
Copy link
Collaborator

No worries :)

@andig
Copy link
Contributor Author

andig commented Jan 10, 2020

I've finally found what happens and why I didn't see it every time:

Problem ist das in searchInPath, if config file doesn't exist, it will fall back to filename without extension. That file exists- during dev and for some user reports- it is the binary. When that is read the config type is not known:

func (v *Viper) searchInPath(in string) (filename string) {
	jww.DEBUG.Println("Searching for config in ", in)
	for _, ext := range SupportedExts {
		jww.DEBUG.Println("Checking for", filepath.Join(in, v.configName+"."+ext))
		if b, _ := exists(v.fs, filepath.Join(in, v.configName+"."+ext)); b {
			jww.DEBUG.Println("Found: ", filepath.Join(in, v.configName+"."+ext))
			return filepath.Join(in, v.configName+"."+ext)
		}
	}

	if b, _ := exists(v.fs, filepath.Join(in, v.configName)); b {
		return filepath.Join(in, v.configName)
	}

	return ""
}

I haven't checked why the fallback problem didn't exist in 1.5 (or maybe it did), but it is a troublesome. What do you think?

The culprit is #722, /cc @pedromss

@andig andig reopened this Jan 10, 2020
@andig andig changed the title 1.6.0/1.6.1: Unsupported Config Type "" ^1.6: Unsupported Config Type "" when config file not found and falling back to extension-less filename Jan 10, 2020
@pedromss
Copy link
Contributor

Hey @andig thanks for the heads up. I'm curious: why is there a file with the same name as the config file but with no extension? Why not just name it something different or give it a meaningful extension?

You also say the file exists during dev, this suggests it doesn't always exist, if so, wouldn't it be better to be in a build directory that would be clean every now and then?

Having a config file without an extension is something that happens quite alot.

I don't agree that it should be ConfigFileNotFound as the file acctually exists, it just can't be parsed as you said, because its a binary file

@andig
Copy link
Contributor Author

andig commented Jan 10, 2020

Thanks @pedromss for joining. Please let me start by reiterating the issue I perceive:

With 1.6, untyped config files will be accepted by default. Unprepared applications, i.e. those that don't set a config type, will experience this as breaking change if such a file- for whatever reasons- exists in any of the search paths.

I'm curious: why is there a file with the same name as the config file but with no extension? Why not just name it something different or give it a meaningful extension?

My app is specifically using yaml config (with extension). It comes bundled as .dist.yaml. The extension-less variant is just a compiled binary in my case but might be anything else.

You also say the file exists during dev, this suggests it doesn't always exist, if so, wouldn't it be better to be in a build directory that would be clean every now and then?

Sure. I still cannot control if such a file- that I'd really like to ignore- might not exist anywhere in the search path, especially if that includes user home dir or similar.

Having a config file without an extension is something that happens quite a lot.

I accept that. I do not want this to impact my application though, especially when I have SupportedExts to control what type of config I'd like to support.

I don't agree that it should be ConfigFileNotFound as the file acctually exists, it just can't be parsed as you said, because its a binary file

This is not about parsing. It is rather about treating arbitrary files as config which- as with etcd- needs special preparation on the application side like setting config type.

So... maybe one way to do this could be using SupportedExts to allow "" as supported extension if so desired and leave off by default?

@sagikazarmark
Copy link
Collaborator

It is rather about treating arbitrary files

Well, it's not arbitrary, you gave it a name to look for. But I understand how it might be a problem.

Since setting a config type for extensionless files is required anyway, do you think @pedromss that we could restrict the search to only check extensionless files if there is a config type set?

It's kind of a compromise solution: if you set a config type @andig Viper will find and try to parse those files.

@andig
Copy link
Contributor Author

andig commented Jan 10, 2020

do you think @pedromss that we could restrict the search to only check extensionless files if there is a config type set?

Sounds really reasonable and especially consistent with 1.5 behaviour to me. If this is the way forward it would be great to cut a point release asap.

@pedromss
Copy link
Contributor

@sagikazarmark sounds ok to me. Should I get on it?

@sagikazarmark
Copy link
Collaborator

Fixed in v1.6.2

Thanks @andig and @pedromss ! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants