test: rework configuration file testing

Signed-off-by: Adrien Barreau <adrien.barreau@ovhcloud.com>

ovh/configuration.go

@@ -4,20 +4,21 @@ import (
 	"fmt"
 	"os"
 	"os/user"
-	"path/filepath"
 	"strings"
 
 	"gopkg.in/ini.v1"
 )
 
-// Use variables for easier test overload
-var (
-	systemConfigPath = "/etc/ovh.conf"
-	userConfigPath   = "/.ovh.conf" // prefixed with homeDir
-	localConfigPath  = "./ovh.conf"
-)
+var configPaths = []string{
+	// System wide configuration
+	"/etc/ovh.com",
+	// Configuration in user's home
+	"~/.ovh.conf",
+	// Configuration in local folder
+	"./ovh.conf",
+}
 
-// currentUserHome attempts to get current user's home directory
+// currentUserHome attempts to get current user's home directory.
 func currentUserHome() (string, error) {
 	usr, err := user.Current()
 	if err != nil {
@@ -31,14 +32,43 @@ func currentUserHome() (string, error) {
 	return usr.HomeDir, nil
 }
 
-// appendConfigurationFile only if it exists. We need to do this because
-// ini package will fail to load configuration at all if a configuration
-// file is missing. This is racy, but better than always failing.
-func appendConfigurationFile(cfg *ini.File, path string) {
-	if file, err := os.Open(path); err == nil {
-		defer file.Close()
-		_ = cfg.Append(path)
+// configPaths returns configPaths, with ~/ prefix expanded.
+func expandConfigPaths() []interface{} {
+	paths := []interface{}{}
+
+	// Will be initialized on first use
+	var home string
+	var homeErr error
+
+	for _, path := range configPaths {
+		if strings.HasPrefix(path, "~/") {
+			// Find home if needed
+			if home == "" && homeErr == nil {
+				home, homeErr = currentUserHome()
+			}
+			// Ignore file in HOME if we cannot find it
+			if homeErr != nil {
+				continue
+			}
+
+			path = home + path[1:]
+		}
+
+		paths = append(paths, path)
+	}
+
+	return paths
+}
+
+// loadINI builds a ini.File from the configuration paths provided in configPaths.
+// It's a helper for loadConfig.
+func loadINI() (*ini.File, error) {
+	paths := expandConfigPaths()
+	if len(paths) == 0 {
+		return ini.Empty(), nil
 	}
+
+	return ini.LooseLoad(paths[0], paths[1:]...)
 }
 
 // loadConfig loads client configuration from params, environments or configuration
@@ -58,13 +88,10 @@ func appendConfigurationFile(cfg *ini.File, path string) {
 func (c *Client) loadConfig(endpointName string) error {
 	// Load configuration files by order of increasing priority. All configuration
 	// files are optional. Only load file from user home if home could be resolve
-	cfg := ini.Empty()
-	appendConfigurationFile(cfg, systemConfigPath)
-	if home, err := currentUserHome(); err == nil {
-		userConfigFullPath := filepath.Join(home, userConfigPath)
-		appendConfigurationFile(cfg, userConfigFullPath)
+	cfg, err := loadINI()
+	if err != nil {
+		return fmt.Errorf("cannot load configuration: %w", err)
 	}
-	appendConfigurationFile(cfg, localConfigPath)
 
 	// Canonicalize configuration
 	if endpointName == "" {

ovh/configuration_test.go

@@ -1,139 +1,90 @@
 package ovh
 
 import (
-	"io/ioutil"
-	"os"
 	"testing"
 
 	"github.com/maxatome/go-testdeep/td"
 )
 
-//
-// Utils
-//
-
-var home string
-
-func setup() {
-	systemConfigPath = "./ovh.unittest.global.conf"
-	userConfigPath = "/.ovh.unittest.user.conf"
-	localConfigPath = "./ovh.unittest.local.conf"
-	home, _ = currentUserHome()
-}
+const (
+	systemConf       = "testdata/system.ini"
+	userPartialConf  = "testdata/userPartial.ini"
+	userConf         = "testdata/user.ini"
+	localPartialConf = "testdata/localPartial.ini"
+	localWithURLConf = "testdata/localWithURL.ini"
+	doesNotExistConf = "testdata/doesNotExist.ini"
+	invalidINIConf   = "testdata/invalid.ini"
+	errorConf        = "testdata"
+)
 
-func teardown() {
-	os.Remove(systemConfigPath)
-	os.Remove(home + userConfigPath)
-	os.Remove(localConfigPath)
+func setConfigPaths(t testing.TB, paths ...string) {
+	old := configPaths
+	configPaths = paths
+	t.Cleanup(func() { configPaths = old })
 }
 
-//
-// Tests
-//
-
 func TestConfigFromFiles(t *testing.T) {
-	assert, require := td.AssertRequire(t)
-
-	// Write each parameter to one different configuration file
-	// This is a simple way to test precedence
-
-	// Prepare
-	err := ioutil.WriteFile(systemConfigPath, []byte(`
-[ovh-eu]
-application_key=system
-application_secret=system
-consumer_key=system
-`), 0660)
-	require.CmpNoError(err)
-
-	err = ioutil.WriteFile(home+userConfigPath, []byte(`
-[ovh-eu]
-application_secret=user
-consumer_key=user
-`), 0660)
-	require.CmpNoError(err)
-
-	err = ioutil.WriteFile(localConfigPath, []byte(`
-[ovh-eu]
-consumer_key=local
-`), 0660)
-	require.CmpNoError(err)
-
-	// Clear
-	t.Cleanup(func() {
-		_ = ioutil.WriteFile(systemConfigPath, []byte(``), 0660)
-		_ = ioutil.WriteFile(home+userConfigPath, []byte(``), 0660)
-		_ = ioutil.WriteFile(localConfigPath, []byte(``), 0660)
-	})
+	setConfigPaths(t, systemConf, userPartialConf, localPartialConf)
 
 	client := Client{}
-	err = client.loadConfig("ovh-eu")
-	require.CmpNoError(err)
-	assert.Cmp(client, td.Struct(Client{
+	err := client.loadConfig("ovh-eu")
+	td.Require(t).CmpNoError(err)
+	td.Cmp(t, client, td.Struct(Client{
 		AppKey:      "system",
 		AppSecret:   "user",
 		ConsumerKey: "local",
 	}))
 }
 
 func TestConfigFromOnlyOneFile(t *testing.T) {
-	assert, require := td.AssertRequire(t)
-
-	// ini package has a bug causing it to ignore all subsequent configuration
-	// files if any could not be loaded. Make sure that workaround... works.
-
-	// Prepare
-	err := os.Remove(systemConfigPath)
-	require.CmpNoError(err)
-
-	err = ioutil.WriteFile(home+userConfigPath, []byte(`
-[ovh-eu]
-application_key=user
-application_secret=user
-consumer_key=user
-`), 0660)
-	require.CmpNoError(err)
-
-	// Clear
-	t.Cleanup(func() {
-		_ = ioutil.WriteFile(home+userConfigPath, []byte(``), 0660)
-	})
+	setConfigPaths(t, userConf)
 
 	client := Client{}
-	err = client.loadConfig("ovh-eu")
-	require.CmpNoError(err)
-	assert.Cmp(client, td.Struct(Client{
+	err := client.loadConfig("ovh-eu")
+	td.Require(t).CmpNoError(err)
+	td.Cmp(t, client, td.Struct(Client{
 		AppKey:      "user",
 		AppSecret:   "user",
 		ConsumerKey: "user",
 	}))
 }
 
-func TestConfigFromEnv(t *testing.T) {
-	assert, require := td.AssertRequire(t)
+func TestConfigFromNonExistingFile(t *testing.T) {
+	setConfigPaths(t, doesNotExistConf)
 
-	err := ioutil.WriteFile(systemConfigPath, []byte(`
-[ovh-eu]
-application_key=fail
-application_secret=fail
-consumer_key=fail
-`), 0660)
-	require.CmpNoError(err)
+	client := Client{}
+	err := client.loadConfig("ovh-eu")
+	td.CmpString(t, err, `missing application key, please check your configuration or consult the documentation to create one`)
+}
 
-	t.Cleanup(func() {
-		_ = ioutil.WriteFile(systemConfigPath, []byte(``), 0660)
-	})
+func TestConfigFromInvalidINIFile(t *testing.T) {
+	setConfigPaths(t, invalidINIConf)
+
+	client := Client{}
+	err := client.loadConfig("ovh-eu")
+	td.CmpString(t, err, "cannot load configuration: unclosed section: [ovh\n")
+}
+
+func TestConfigFromInvalidFile(t *testing.T) {
+	setConfigPaths(t, errorConf)
+
+	client := Client{}
+	err := client.loadConfig("ovh-eu")
+	td.CmpString(t, err, "cannot load configuration: BOM: read testdata: is a directory")
+}
+
+func TestConfigFromEnv(t *testing.T) {
+	setConfigPaths(t, userConf)
 
 	t.Setenv("OVH_ENDPOINT", "ovh-eu")
 	t.Setenv("OVH_APPLICATION_KEY", "env")
 	t.Setenv("OVH_APPLICATION_SECRET", "env")
 	t.Setenv("OVH_CONSUMER_KEY", "env")
 
-	// Test
 	client := Client{}
-	err = client.loadConfig("")
-	require.CmpNoError(err)
-	assert.Cmp(client, td.Struct(Client{
+	err := client.loadConfig("")
+	td.Require(t).CmpNoError(err)
+	td.Cmp(t, client, td.Struct(Client{
 		AppKey:      "env",
 		AppSecret:   "env",
 		ConsumerKey: "env",
@@ -142,12 +93,12 @@ consumer_key=fail
 }
 
 func TestConfigFromArgs(t *testing.T) {
-	assert, require := td.AssertRequire(t)
+	setConfigPaths(t, userConf)
 
 	client := Client{AppKey: "param", AppSecret: "param", ConsumerKey: "param"}
 	err := client.loadConfig("ovh-eu")
-	require.CmpNoError(err)
-	assert.Cmp(client, td.Struct(Client{
+	td.Require(t).CmpNoError(err)
+	td.Cmp(t, client, td.Struct(Client{
 		AppKey:      "param",
 		AppSecret:   "param",
 		ConsumerKey: "param",
@@ -158,27 +109,11 @@ func TestConfigFromArgs(t *testing.T) {
 func TestEndpoint(t *testing.T) {
 	assert, require := td.AssertRequire(t)
 
-	err := ioutil.WriteFile(systemConfigPath, []byte(`
-[ovh-eu]
-application_key=ovh
-application_secret=ovh
-consumer_key=ovh
-
-[https://api.example.com:4242]
-application_key=example.com
-application_secret=example.com
-consumer_key=example.com
-`), 0660)
-	require.CmpNoError(err)
-
-	// Clear
-	t.Cleanup(func() {
-		_ = ioutil.WriteFile(systemConfigPath, []byte(``), 0660)
-	})
+	setConfigPaths(t, localWithURLConf)
 
 	// Test: by name
 	client := Client{}
-	err = client.loadConfig("ovh-eu")
+	err := client.loadConfig("ovh-eu")
 	require.CmpNoError(err)
 	assert.Cmp(client, td.Struct(Client{
 		AppKey: "ovh",
@@ -210,16 +145,15 @@ func TestMissingParam(t *testing.T) {
 	td.CmpString(t, err, `missing application secret, please check your configuration or consult the documentation to create one`)
 }
 
-//
-// Main
-//
-
-// TestMain changes the location of configuration files. We need
-// this to avoid any interference with existing configuration
-// and non-root users
-func TestMain(m *testing.M) {
-	setup()
-	code := m.Run()
-	teardown()
-	os.Exit(code)
+func TestConfigPaths(t *testing.T) {
+	home, err := currentUserHome()
+	td.Require(t).CmpNoError(err)
+
+	setConfigPaths(t, "", "file", "file.ini", "dir/file.ini", "~/file.ini", "~typo.ini")
+
+	td.Cmp(t, home, td.Not(td.HasSuffix("/")))
+	td.Cmp(t,
+		expandConfigPaths(),
+		[]interface{}{"", "file", "file.ini", "dir/file.ini", home + "/file.ini", "~typo.ini"},
+	)
 }

ovh/testdata/invalid.ini

@@ -0,0 +1,2 @@
+[ovh
+consumer_key=local

ovh/testdata/localPartial.ini

@@ -0,0 +1,2 @@
+[ovh-eu]
+consumer_key=local

ovh/testdata/localWithURL.ini

@@ -0,0 +1,9 @@
+[ovh-eu]
+application_key=ovh
+application_secret=ovh
+consumer_key=ovh
+
+[https://api.example.com:4242]
+application_key=example.com
+application_secret=example.com
+consumer_key=example.com

ovh/testdata/system.ini

@@ -0,0 +1,4 @@
+[ovh-eu]
+application_key=system
+application_secret=system
+consumer_key=system

ovh/testdata/user.ini

@@ -0,0 +1,4 @@
+[ovh-eu]
+application_key=user
+application_secret=user
+consumer_key=user

ovh/testdata/userPartial.ini

@@ -0,0 +1,3 @@
+[ovh-eu]
+application_secret=user
+consumer_key=user