configFileNotFound bug?

[ghostsquad]

I was just reviewing the code, and I found a part that looks a little suspect..

https://github.com/sagikazarmark/modern-go-application/blob/master/cmd/modern-go-application/main.go?ts=4#L89

if ReadInConfig does not return an error, then configFileNotFound would be false, right? since the type assert from nil to viper.ConfigFileNotFoundError would fail?

err := v.ReadInConfig()
_, configFileNotFound := err.(viper.ConfigFileNotFoundError)
if !configFileNotFound {
	emperror.Panic(errors.Wrap(err, "failed to read configuration"))
}

# more stuff, including configuring the logger

if configFileNotFound {
	logger.Warn("configuration file not found")
}

it seems like the code will either always panic, or log a warning.

The viper shows the correct way: https://github.com/spf13/viper?ts=4#reading-config-files

if err := viper.ReadInConfig(); err != nil {
    if _, ok := err.(viper.ConfigFileNotFoundError); ok {
        // Config file not found; ignore error if desired
    } else {
        // Config file was found but another error was produced
    }
}

// Config file found and successfully parsed

Additionally, I don't think this is best practice anyway, since a configuration file should not be required if ENV variables are also an alternative. Maybe I'm also reading the code wrong...

[kohenkatz]

@ghostsquad I was just looking at this too, and it is correct, just hard to read.

If you switch the order of the if statements, you will see that it is actually the same as the viper example. (One important thing to remember is that emperror.Panic halts execution so it will never get to the second if statement.)

If you want to avoid the error, change it to this (with explanatory comments added):

usingConfigFile := false

if c, _ := p.GetString("config"); c != "" {
	v.SetConfigFile(c)
	usingConfigFile = true
}

err := v.ReadInConfig()
// This sets `configFileNotFound` to `true` if the file does not exist
// or `false` if the file exists but there is some other problem with it.
_, configFileNotFound := err.(viper.ConfigFileNotFoundError)
if !configFileNotFound {
	// Since there was a file and it was bad, show an error.
	emperror.Panic(errors.Wrap(err, "failed to read configuration"))
}

# more stuff, including configuring the logger

// Only do this is `usingConfigFile` is true
if usingConfigFile && configFileNotFound {
	// A path was provided to a nonexistent file, warn the user
	logger.Warn("configuration file specified but not found")
}

(That is what I did)

[sagikazarmark]

Admittedly, this kind of assertion is always hard to read, especially with errors (or pointers in generic).

Whether warning is a correct level for a missing config file or not depends on how you operate your applications: most of the time I deploy them to Kubernetes using Helm, so YAML comes as a first class citizen in Helm values, translated into a config file.

But that's just how I do things, I don't consider it a best practice. If you strictly want to follow Twelve Factor apps, using env vars is perfectly fine.

I can think of two possible improvements (not touching the assertion though):

• log the fact that config file was not available as Info instead of Warn. Since Viper will look for a config file automatically and you might want to rely on that behavior, logging only when a config flag is explicitly provided might not be the best option
• invert the condition: write log (with config file path) whenever a config file is loaded. There is a similar behavior in Viper actually, but I'm not a huge fan of it TBH, so I prefer avoiding it. In Viper 2 we might be able to improve these things.

One could also argue that this is actually not really relevant during normal operations and only has a value during debugging, so we could make it a debug log as well.

WDYT?

[kohenkatz]

One could also argue that this is actually not really relevant during normal operations and only has a value during debugging, so we could make it a debug log as well.

There is one production case in which it makes sense to be a warning (or maybe even an error). I am considering the following four cases:

1. The user is using env vars to configure or is using the default config file location. In this case, there is no need to log anything.
2. The config file (either user-specified or default) is invalid. In this case, it will panic with an error message.
3. The user has specified a config file and it is valid. In this case there is also no need to log anything.
4. The user has specified a config file, but the file does not exist. This is the case in which we really need to warn the user.