Merge pull request #165 from stephenfin/issues/164

clientconfig: Ignore OS_CLOUD if explicit cloud config provided

Author: EmilienM

openstack/clientconfig/requests.go

@@ -183,6 +183,10 @@ func LoadPublicCloudsYAML() (map[string]Cloud, error) {
 
 // GetCloudFromYAML will return a cloud entry from a clouds.yaml file.
 func GetCloudFromYAML(opts *ClientOpts) (*Cloud, error) {
+	if opts == nil {
+		opts = new(ClientOpts)
+	}
+
 	if opts.YAMLOpts == nil {
 		opts.YAMLOpts = new(YAMLOpts)
 	}
@@ -197,19 +201,18 @@ func GetCloudFromYAML(opts *ClientOpts) (*Cloud, error) {
 	// Determine which cloud to use.
 	// First see if a cloud name was explicitly set in opts.
 	var cloudName string
-	if opts != nil && opts.Cloud != "" {
+	if opts.Cloud != "" {
 		cloudName = opts.Cloud
-	}
-
-	// Next see if a cloud name was specified as an environment variable.
-	// This is supposed to override an explicit opts setting.
-	envPrefix := "OS_"
-	if opts.EnvPrefix != "" {
-		envPrefix = opts.EnvPrefix
-	}
+	} else {
+		// If not, see if a cloud name was specified as an environment variable.
+		envPrefix := "OS_"
+		if opts.EnvPrefix != "" {
+			envPrefix = opts.EnvPrefix
+		}
 
-	if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
-		cloudName = v
+		if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
+			cloudName = v
+		}
 	}
 
 	var cloud *Cloud
@@ -351,16 +354,17 @@ func AuthOptions(opts *ClientOpts) (*gophercloud.AuthOptions, error) {
 	var cloudName string
 	if opts.Cloud != "" {
 		cloudName = opts.Cloud
-	}
-
-	// Next see if a cloud name was specified as an environment variable.
-	envPrefix := "OS_"
-	if opts.EnvPrefix != "" {
-		envPrefix = opts.EnvPrefix
-	}
+	} else {
+		// If not, see if a cloud name was specified as an environment
+		// variable.
+		envPrefix := "OS_"
+		if opts.EnvPrefix != "" {
+			envPrefix = opts.EnvPrefix
+		}
 
-	if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
-		cloudName = v
+		if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
+			cloudName = v
+		}
 	}
 
 	// If a cloud name was determined, try to look it up in clouds.yaml.

openstack/clientconfig/testing/requests_test.go

@@ -84,6 +84,28 @@ func TestGetCloudFromYAML(t *testing.T) {
 	}
 }
 
+func TestGetCloudFromYAMLOSCLOUD(t *testing.T) {
+	os.Setenv("OS_CLOUD", "california")
+	defer os.Unsetenv("OS_CLOUD")
+
+	clientOpts := &clientconfig.ClientOpts{
+		Cloud: "hawaii",
+	}
+
+	actual, err := clientconfig.GetCloudFromYAML(clientOpts)
+	th.AssertNoErr(t, err)
+	th.AssertDeepEquals(t, &HawaiiCloudYAML, actual)
+}
+
+func TestGetCloudFromYAMLMissingClientOpts(t *testing.T) {
+	os.Setenv("OS_CLOUD", "california")
+	defer os.Unsetenv("OS_CLOUD")
+
+	actual, err := clientconfig.GetCloudFromYAML(nil)
+	th.AssertNoErr(t, err)
+	th.AssertDeepEquals(t, &CaliforniaCloudYAML, actual)
+}
+
 func TestAuthOptionsExplicitCloud(t *testing.T) {
 	os.Unsetenv("OS_CLOUD")
 
@@ -101,6 +123,7 @@ func TestAuthOptionsExplicitCloud(t *testing.T) {
 
 func TestAuthOptionsOSCLOUD(t *testing.T) {
 	os.Setenv("FOO_CLOUD", "hawaii")
+	defer os.Unsetenv("FOO_CLOUD")
 
 	clientOpts := &clientconfig.ClientOpts{
 		EnvPrefix: "FOO_",
@@ -112,8 +135,38 @@ func TestAuthOptionsOSCLOUD(t *testing.T) {
 	}
 
 	th.AssertDeepEquals(t, HawaiiAuthOpts, actual)
+}
+
+func TestAuthOptionsExplicitCloudAndOSCLOUD(t *testing.T) {
+	os.Setenv("FOO_CLOUD", "hawaii")
+	defer os.Unsetenv("FOO_CLOUD")
+
+	clientOpts := &clientconfig.ClientOpts{
+		EnvPrefix: "FOO_",
+		Cloud:     "california",
+	}
 
-	os.Unsetenv("FOO_CLOUD")
+	actual, err := clientconfig.AuthOptions(clientOpts)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// We should have ignored the cloud configuration option
+	th.AssertDeepEquals(t, CaliforniaAuthOpts, actual)
+}
+
+func TestAuthOptionsMissingClientOpts(t *testing.T) {
+	os.Setenv("OS_CLOUD", "hawaii")
+	defer os.Unsetenv("OS_CLOUD")
+
+	actual, err := clientconfig.AuthOptions(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// We should have handled the missing config opts and fallen back to
+	// defaults
+	th.AssertDeepEquals(t, HawaiiAuthOpts, actual)
 }
 
 func TestAuthOptionsCreationFromCloudsYAML(t *testing.T) {
@@ -246,15 +299,12 @@ func TestAuthOptionsCreationFromEnv(t *testing.T) {
 	for cloud, envVars := range allEnvVars {
 		for k, v := range envVars {
 			os.Setenv(k, v)
+			defer os.Unsetenv(k)
 		}
 
 		actualAuthOpts, err := clientconfig.AuthOptions(nil)
 		th.AssertNoErr(t, err)
 		th.AssertDeepEquals(t, expectedAuthOpts[cloud], actualAuthOpts)
-
-		for k := range envVars {
-			os.Unsetenv(k)
-		}
 	}
 }
 
@@ -274,15 +324,12 @@ func TestAuthOptionsCreationFromLegacyEnv(t *testing.T) {
 	for cloud, envVars := range allEnvVars {
 		for k, v := range envVars {
 			os.Setenv(k, v)
+			defer os.Unsetenv(k)
 		}
 
 		actualAuthOpts, err := clientconfig.AuthOptions(nil)
 		th.AssertNoErr(t, err)
 		th.AssertDeepEquals(t, expectedAuthOpts[cloud], actualAuthOpts)
-
-		for k := range envVars {
-			os.Unsetenv(k)
-		}
 	}
 }