Repository: docker/notary
Pull request #341 (2015-12-07 07:48:01)

Add a function to parse viper
None of these `filename`, `ext` or `configPath` should be in `main`,
they are all just for creating a instance of Viper and then nothing.
Do it in a separate function will make the function `main` more readable.

Signed-off-by: Hu Keping <hukeping@huawei.com>

cyli: @HuKeping Thanks for factoring this out!  It's great to have it be its own function!   Two nitpicks:

1.  It'd be awesome if `notary-server/main.go` could also make use of this function, since it has similar code
2.  Would you mind adding a test for this function?  Since viper [does not seem to expose the config name/extension/path](https://github.com/spf13/viper/blob/master/viper.go#L140), maybe just the following cases:
   - read success:  create temporary json file with one parameter, verify that there is no error reading that config file and getting the single parameter
   - read failure: pass a config file that doesn't exist?

That should hopefully fix the code coverage check.  :)
endophage: Yep, add tests and we'll be happy to merge this. Additionally refactor notary-server/main.go to use it and we'll be extra excited to merge :-)
HuKeping: Tests are ready and also add the change on server side.
cyli: Thank you for your contribution!  :) 

Patch:
diff --git a/cmd/notary-server/main.go b/cmd/notary-server/main.go
index 0577dade..044da761 100644
--- a/cmd/notary-server/main.go
+++ b/cmd/notary-server/main.go
@@ -8,8 +8,6 @@ import (
 	"net/http"
 	_ "net/http/pprof"
 	"os"
-	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/Sirupsen/logrus"
@@ -186,19 +184,9 @@ func main() {
 
 	ctx := context.Background()
 
-	filename := filepath.Base(configFile)
-	ext := filepath.Ext(configFile)
-	configPath := filepath.Dir(configFile)
-
-	mainViper.SetConfigType(strings.TrimPrefix(ext, "."))
-	mainViper.SetConfigName(strings.TrimSuffix(filename, ext))
-	mainViper.AddConfigPath(configPath)
-
-	err := mainViper.ReadInConfig()
-	if err != nil {
-		logrus.Error("Viper Error: ", err.Error())
-		logrus.Error("Could not read config at ", configFile)
-		os.Exit(1)
+	// parse viper config
+	if err := utils.ParseViper(mainViper, configFile); err != nil {
+		logrus.Fatal(err.Error())
 	}
 
 	// default is error level
diff --git a/cmd/notary-signer/main.go b/cmd/notary-signer/main.go
index f609ce9a..ee0ed636 100644
--- a/cmd/notary-signer/main.go
+++ b/cmd/notary-signer/main.go
@@ -12,7 +12,6 @@ import (
 	"net"
 	"net/http"
 	"os"
-	"path/filepath"
 	"strings"
 	"time"
 
@@ -181,17 +180,9 @@ func main() {
 	// when the signer starts print the version for debugging and issue logs later
 	logrus.Infof("Version: %s, Git commit: %s", version.NotaryVersion, version.GitCommit)
 
-	filename := filepath.Base(configFile)
-	ext := filepath.Ext(configFile)
-	configPath := filepath.Dir(configFile)
-
-	mainViper.SetConfigType(strings.TrimPrefix(ext, "."))
-	mainViper.SetConfigName(strings.TrimSuffix(filename, ext))
-	mainViper.AddConfigPath(configPath)
-
-	if err := mainViper.ReadInConfig(); err != nil {
-		logrus.Errorf("Could not read config at :%s, viper error: %v", configFile, err)
-		os.Exit(1)
+	// parse viper config
+	if err := utils.ParseViper(mainViper, configFile); err != nil {
+		logrus.Fatal(err.Error())
 	}
 
 	// default is error level
diff --git a/utils/configuration.go b/utils/configuration.go
index e0cf346d..929482ae 100644
--- a/utils/configuration.go
+++ b/utils/configuration.go
@@ -159,3 +159,19 @@ func SetUpBugsnag(config *bugsnag.Configuration) error {
 	}
 	return nil
 }
+
+// ParseViper tries to parse out a Viper from a configuration file.
+func ParseViper(v *viper.Viper, configFile string) error {
+	filename := filepath.Base(configFile)
+	ext := filepath.Ext(configFile)
+	configPath := filepath.Dir(configFile)
+
+	v.SetConfigType(strings.TrimPrefix(ext, "."))
+	v.SetConfigName(strings.TrimSuffix(filename, ext))
+	v.AddConfigPath(configPath)
+
+	if err := v.ReadInConfig(); err != nil {
+		return fmt.Errorf("Could not read config at :%s, viper error: %v", configFile, err)
+	}
+	return nil
+}


Test patch:
diff --git a/utils/configuration_test.go b/utils/configuration_test.go
index 308f8983..95c80e50 100644
--- a/utils/configuration_test.go
+++ b/utils/configuration_test.go
@@ -335,3 +335,28 @@ func TestParseTLSWithEnvironmentVariables(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, expected, *tlsOpts)
 }
+
+func TestParseViperWithInvalidFile(t *testing.T) {
+	v := viper.New()
+	SetupViper(v, envPrefix)
+
+	err := ParseViper(v, "Chronicle_Of_Dark_Secrets.json")
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "Could not read config")
+}
+
+func TestParseViperWithValidFile(t *testing.T) {
+	file, err := os.Create("/tmp/Chronicle_Of_Dark_Secrets.json")
+	assert.NoError(t, err)
+	defer os.Remove(file.Name())
+
+	file.WriteString(`{"logging": {"level": "debug"}}`)
+
+	v := viper.New()
+	SetupViper(v, envPrefix)
+
+	err = ParseViper(v, "/tmp/Chronicle_Of_Dark_Secrets.json")
+	assert.NoError(t, err)
+
+	assert.Equal(t, "debug", v.GetString("logging.level"))
+}