fix: config file non-fatal edge cases, tests (#216)

* fix: non-fatal error possible in Load()
* test: test all config file edge cases

pkg/config/config.go

@@ -69,10 +69,8 @@ func LoadFile() error {
 	}
 	fileInfo, statErr := os.Stat(path)
 	if statErr != nil {
-		return fmt.Errorf("error getting credentials: nor $%s, $%s, $%s set, nor config file: %s",
-			sdk.IonosUsernameEnvVar, sdk.IonosPasswordEnvVar, sdk.IonosTokenEnvVar, statErr.Error())
+		return statErr
 	}
-
 	perm := fileInfo.Mode().Perm()
 	permNumberBase10 := int64(perm)
 	strBase10 := strconv.FormatInt(permNumberBase10, 8)
@@ -86,23 +84,28 @@ func LoadFile() error {
 	}
 
 	viper.SetConfigFile(viper.GetString(constants.ArgConfig))
-	err := viper.ReadInConfig()
-	if err != nil {
-		return err
-	}
-	return nil
+	return viper.ReadInConfig()
 }
 
-// Load collects config data from the config file, using environment variables as fallback.
+// Load binds environment variables (IONOS_USERNAME, IONOS_PASSWORD) to viper, and attempts
+// to read config file for setting fallbacks for these newly-bound viper vars
 func Load() (err error) {
 	_ = viper.BindEnv(Username, sdk.IonosUsernameEnvVar)
 	_ = viper.BindEnv(Password, sdk.IonosPasswordEnvVar)
 	_ = viper.BindEnv(Token, sdk.IonosTokenEnvVar)
 	_ = viper.BindEnv(ServerUrl, sdk.IonosApiUrlEnvVar)
 
-	err = LoadFile()
+	err = LoadFile() // Use config file as a fallback for any of the above variables. Could be used only for api-url
+
+	if viper.IsSet(Token) || (viper.IsSet(Username) && viper.IsSet(Password)) {
+		// Error thrown by LoadFile is recoverable in this case.
+		// We don't want to throw an error e.g. if the user only uses the config file for api-url,
+		// or if he has IONOS_TOKEN, or IONOS_USERNAME and IONOS_PASSWORD exported as env vars and no config file at all
+		return nil
+	}
 
-	return err
+	return fmt.Errorf("%w: Please export %s, or %s and %s, or do ionosctl login to generate a config file",
+		err, sdk.IonosTokenEnvVar, sdk.IonosUsernameEnvVar, sdk.IonosPasswordEnvVar)
 }
 
 func WriteFile() error {

pkg/config/config_test.go

@@ -12,19 +12,172 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func TestUsingJustTokenEnvVar(t *testing.T) {
+	os.Clearenv()
+	viper.Reset()
+
+	viper.SetConfigFile(os.DevNull)
+	viper.Set(constants.ArgConfig, os.DevNull)
+
+	assert.NoError(t, os.Setenv(sdk.IonosTokenEnvVar, "tok"))
+	assert.NoError(t, Load())
+	assert.Equal(t, "tok", viper.GetString(Token))
+	assert.Equal(t, "", viper.GetString(Username))
+	assert.Equal(t, "", viper.GetString(Password))
+	assert.Equal(t, "", viper.GetString(ServerUrl))
+}
+
+func TestTokEnvWithUserPassConfigBackup(t *testing.T) {
+	// Useful for API routes which don't accept bearer tokens, or custom IonosCTL commands (Image Upload)
+	os.Clearenv()
+	viper.Reset()
+
+	assert.NoError(t, os.Setenv(sdk.IonosTokenEnvVar, "tok"))
+	path := filepath.Join("..", "testdata", "config_user_pass.json") // TODO: These files should be created and then destroyed by the tests
+	viper.SetConfigFile(path)
+	viper.Set(constants.ArgConfig, path)
+	assert.NoError(t, os.Chmod(path, 0600))
+	assert.NoError(t, Load())
+
+	assert.Equal(t, "tok", viper.GetString(Token))
+	assert.Equal(t, "[email protected]", viper.GetString(Username))
+	assert.Equal(t, "test", viper.GetString(Password))
+	assert.Equal(t, "", viper.GetString(ServerUrl))
+}
+
+func TestTokEnvWithFullConfig(t *testing.T) {
+	// Config token should not override env var token
+	os.Clearenv()
+	viper.Reset()
+
+	assert.NoError(t, os.Setenv(sdk.IonosTokenEnvVar, "tok"))
+	path := filepath.Join("..", "testdata", "config.json") // TODO: These files should be created and then destroyed by the tests
+	viper.SetConfigFile(path)
+	viper.Set(constants.ArgConfig, path)
+	assert.NoError(t, os.Chmod(path, 0600))
+	assert.NoError(t, Load())
+
+	assert.Equal(t, "tok", viper.GetString(Token))
+	assert.Equal(t, "[email protected]", viper.GetString(Username))
+	assert.Equal(t, "test", viper.GetString(Password))
+	assert.Equal(t, "https://api.ionos.com", viper.GetString(ServerUrl))
+}
+
+func TestEnvVarsHavePriority(t *testing.T) {
+	// Make sure env vars not overriden by config file
+	os.Clearenv()
+	viper.Reset()
+
+	assert.NoError(t, os.Setenv(sdk.IonosTokenEnvVar, "env_tok"))
+	assert.NoError(t, os.Setenv(sdk.IonosUsernameEnvVar, "env_user"))
+	assert.NoError(t, os.Setenv(sdk.IonosPasswordEnvVar, "env_pass"))
+	assert.NoError(t, os.Setenv(sdk.IonosApiUrlEnvVar, "env_url"))
+	path := filepath.Join("..", "testdata", "config.json") // TODO: These files should be created and then destroyed by the tests
+	viper.SetConfigFile(path)
+	viper.Set(constants.ArgConfig, path)
+	assert.NoError(t, os.Chmod(path, 0600))
+	assert.NoError(t, Load())
+
+	assert.Equal(t, "env_tok", viper.GetString(Token))
+	assert.Equal(t, "env_user", viper.GetString(Username))
+	assert.Equal(t, "env_pass", viper.GetString(Password))
+	assert.Equal(t, "env_url", viper.GetString(ServerUrl))
+}
+
+func TestAuthErr(t *testing.T) {
+	os.Clearenv()
+	viper.Reset()
+
+	viper.SetConfigFile(os.DevNull)
+	viper.Set(constants.ArgConfig, os.DevNull)
+
+	assert.NoError(t, os.Setenv(sdk.IonosUsernameEnvVar, "env_user"))
+	assert.NoError(t, os.Setenv(sdk.IonosApiUrlEnvVar, "env_url"))
+
+	assert.Error(t, Load()) // Need password or token
+
+	assert.Equal(t, "env_user", viper.GetString(Username))
+	assert.Equal(t, "env_url", viper.GetString(ServerUrl))
+}
+
+func TestUsingJustUsernameAndPasswordEnvVar(t *testing.T) {
+	os.Clearenv()
+	viper.Reset()
+
+	viper.SetConfigFile(os.DevNull)
+	viper.Set(constants.ArgConfig, os.DevNull)
+
+	assert.NoError(t, os.Setenv(sdk.IonosUsernameEnvVar, "user"))
+	assert.NoError(t, os.Setenv(sdk.IonosPasswordEnvVar, "pass"))
+	assert.NoError(t, Load())
+	assert.Equal(t, "", viper.GetString(Token))
+	assert.Equal(t, "user", viper.GetString(Username))
+	assert.Equal(t, "pass", viper.GetString(Password))
+	assert.Equal(t, "", viper.GetString(ServerUrl))
+}
+
+func TestBadConfigPerms(t *testing.T) {
+	os.Clearenv()
+	viper.Reset()
+
+	path := filepath.Join("..", "testdata", "config.json") // TODO: These files should be created and then destroyed by the tests
+	viper.SetConfigFile(path)
+	viper.Set(constants.ArgConfig, path)
+	assert.NoError(t, os.Chmod(path, 0000)) // no read perms
+	assert.Error(t, Load())
+
+	assert.Equal(t, "", viper.GetString(Token))
+	assert.Equal(t, "", viper.GetString(Username))
+	assert.Equal(t, "", viper.GetString(Password))
+	assert.Equal(t, "", viper.GetString(ServerUrl))
+}
+
+func TestUsingJustTokenConfig(t *testing.T) {
+	os.Clearenv()
+	viper.Reset()
+
+	path := filepath.Join("..", "testdata", "config_just_token.json") // TODO: These files should be created and then destroyed by the tests
+	viper.SetConfigFile(path)
+	viper.Set(constants.ArgConfig, path)
+	assert.NoError(t, os.Chmod(path, 0600))
+	assert.NoError(t, Load())
+
+	assert.Equal(t, "tok", viper.GetString(Token))
+	assert.Equal(t, "", viper.GetString(Username))
+	assert.Equal(t, "", viper.GetString(Password))
+	assert.Equal(t, "", viper.GetString(ServerUrl))
+}
+
+func TestUsingJustUsernameAndPasswordConfig(t *testing.T) {
+	os.Clearenv()
+	viper.Reset()
+
+	path := filepath.Join("..", "testdata", "config_user_pass.json") // TODO: These files should be created and then destroyed by the tests
+	viper.SetConfigFile(path)
+	viper.Set(constants.ArgConfig, path)
+	assert.NoError(t, os.Chmod(path, 0600))
+	assert.NoError(t, Load())
+
+	assert.Equal(t, "", viper.GetString(Token))
+	assert.Equal(t, "[email protected]", viper.GetString(Username))
+	assert.Equal(t, "test", viper.GetString(Password))
+	assert.Equal(t, "", viper.GetString(ServerUrl))
+}
+
 func TestGetServerUrl(t *testing.T) {
 	os.Clearenv()
 	viper.Reset()
 
 	// use env
 	assert.NoError(t, os.Setenv(sdk.IonosApiUrlEnvVar, "url"))
-	_ = Load() // ignore error since we just want to load the URL
-	assert.Equal(t, "url", GetServerUrl())
+	err := Load()
+	assert.Error(t, err) // Error because neither token nor user & pass set
+	assert.Equal(t, "url", viper.GetString(ServerUrl))
 
 	// from config
 	os.Clearenv()
 	viper.Reset()
-	viper.SetConfigFile(filepath.Join("..", "testdata", "config.json"))
+	viper.SetConfigFile(filepath.Join("..", "testdata", "config.json")) // TODO: These files should be created and then destroyed by the tests
 	viper.Set(constants.ArgConfig, filepath.Join("..", "testdata", "config.json"))
 	assert.NoError(t, os.Chmod(filepath.Join("..", "testdata", "config.json"), 0600))
 	assert.NoError(t, Load())
@@ -44,7 +197,7 @@ func TestLoadFile(t *testing.T) {
 	os.Clearenv()
 	viper.Reset()
 
-	viper.SetConfigFile(filepath.Join("..", "testdata", "config.json"))
+	viper.SetConfigFile(filepath.Join("..", "testdata", "config.json")) // TODO: These files should be created and then destroyed by the tests
 	viper.Set(constants.ArgConfig, filepath.Join("..", "testdata", "config.json"))
 	assert.NoError(t, os.Chmod(filepath.Join("..", "testdata", "config.json"), 0600))
 	assert.NoError(t, LoadFile())
@@ -58,7 +211,7 @@ func TestLoadEnvFallback(t *testing.T) {
 	os.Clearenv()
 	viper.Reset()
 
-	viper.SetConfigFile(filepath.Join("..", "testdata", "config.json"))
+	viper.SetConfigFile(filepath.Join("..", "testdata", "config.json")) // TODO: These files should be created and then destroyed by the tests
 	viper.Set(constants.ArgConfig, filepath.Join("..", "testdata", "config.json"))
 	assert.NoError(t, os.Setenv(sdk.IonosUsernameEnvVar, "user"))
 	assert.NoError(t, os.Setenv(sdk.IonosPasswordEnvVar, "pass"))

pkg/testdata/config_just_token.json

@@ -0,0 +1,3 @@
+{
+  "userdata.token": "tok"
+}

pkg/testdata/config_user_pass.json

@@ -0,0 +1,4 @@
+{
+  "userdata.name": "[email protected]",
+  "userdata.password": "test"
+}