From fb0dc4951d90bdd43590fd01484bf8bed8d716b9 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 12 Nov 2015 15:51:39 -0600 Subject: [PATCH] provider/azure: read publish_settings as contents instead of path Building on the work in #3846, shifting the Azure provider's configuration option from `settings_file` to `publish_settings`. --- builtin/providers/azure/provider.go | 72 +++++++++++-------- builtin/providers/azure/provider_test.go | 51 ++++--------- .../docs/providers/azure/index.html.markdown | 20 ++++-- 3 files changed, 73 insertions(+), 70 deletions(-) diff --git a/builtin/providers/azure/provider.go b/builtin/providers/azure/provider.go index 975a93b00..b897d2811 100644 --- a/builtin/providers/azure/provider.go +++ b/builtin/providers/azure/provider.go @@ -3,12 +3,10 @@ package azure import ( "encoding/xml" "fmt" - "io/ioutil" - "os" + "github.com/hashicorp/terraform/helper/pathorcontents" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" - "github.com/mitchellh/go-homedir" ) // Provider returns a terraform.ResourceProvider. @@ -20,6 +18,14 @@ func Provider() terraform.ResourceProvider { Optional: true, DefaultFunc: schema.EnvDefaultFunc("AZURE_SETTINGS_FILE", nil), ValidateFunc: validateSettingsFile, + Deprecated: "Use the publish_settings field instead", + }, + + "publish_settings": &schema.Schema{ + Type: schema.TypeString, + Optional: true, + DefaultFunc: schema.EnvDefaultFunc("AZURE_PUBLISH_SETTINGS", nil), + ValidateFunc: validatePublishSettings, }, "subscription_id": &schema.Schema{ @@ -64,11 +70,14 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) { Certificate: []byte(d.Get("certificate").(string)), } - settingsFile := d.Get("settings_file").(string) - if settingsFile != "" { + publishSettings := d.Get("publish_settings").(string) + if publishSettings == "" { + publishSettings = d.Get("settings_file").(string) + } + if publishSettings != "" { // any errors from readSettings would have been caught at the validate // step, so we can avoid handling them now - settings, _, _ := readSettings(settingsFile) + settings, _, _ := readSettings(publishSettings) config.Settings = settings return config.NewClientFromSettingsData() } @@ -92,37 +101,42 @@ func validateSettingsFile(v interface{}, k string) ([]string, []error) { return warnings, errors } -const settingsPathWarnMsg = ` -settings_file is not valid XML, so we are assuming it is a file path. This -support will be removed in the future. Please update your configuration to use -${file("filename.publishsettings")} instead.` +func validatePublishSettings(v interface{}, k string) (ws []string, es []error) { + value := v.(string) + if value == "" { + return + } -func readSettings(pathOrContents string) (s []byte, ws []string, es []error) { var settings settingsData - if err := xml.Unmarshal([]byte(pathOrContents), &settings); err == nil { - s = []byte(pathOrContents) - return + if err := xml.Unmarshal([]byte(value), &settings); err != nil { + es = append(es, fmt.Errorf("error parsing publish_settings as XML: %s", err)) } - ws = append(ws, settingsPathWarnMsg) - path, err := homedir.Expand(pathOrContents) - if err != nil { - es = append(es, fmt.Errorf("Error expanding path: %s", err)) - return - } - - s, err = ioutil.ReadFile(path) - if err != nil { - es = append(es, fmt.Errorf("Could not read file '%s': %s", path, err)) - } return } -func isFile(v string) (bool, error) { - if _, err := os.Stat(v); err != nil { - return false, err +const settingsPathWarnMsg = ` +settings_file was provided as a file path. This support +will be removed in the future. Please update your configuration +to use ${file("filename.publishsettings")} instead.` + +func readSettings(pathOrContents string) (s []byte, ws []string, es []error) { + contents, wasPath, err := pathorcontents.Read(pathOrContents) + if err != nil { + es = append(es, fmt.Errorf("error reading settings_file: %s", err)) } - return true, nil + if wasPath { + ws = append(ws, settingsPathWarnMsg) + } + + var settings settingsData + if err := xml.Unmarshal([]byte(contents), &settings); err != nil { + es = append(es, fmt.Errorf("error parsing settings_file as XML: %s", err)) + } + + s = []byte(contents) + + return } // settingsData is a private struct used to test the unmarshalling of the diff --git a/builtin/providers/azure/provider_test.go b/builtin/providers/azure/provider_test.go index ca4017aae..d06cf896d 100644 --- a/builtin/providers/azure/provider_test.go +++ b/builtin/providers/azure/provider_test.go @@ -51,12 +51,12 @@ func TestProvider_impl(t *testing.T) { } func testAccPreCheck(t *testing.T) { - if v := os.Getenv("AZURE_SETTINGS_FILE"); v == "" { + if v := os.Getenv("AZURE_PUBLISH_SETTINGS"); v == "" { subscriptionID := os.Getenv("AZURE_SUBSCRIPTION_ID") certificate := os.Getenv("AZURE_CERTIFICATE") if subscriptionID == "" || certificate == "" { - t.Fatal("either AZURE_SETTINGS_FILE, or AZURE_SUBSCRIPTION_ID " + + t.Fatal("either AZURE_PUBLISH_SETTINGS, or AZURE_SUBSCRIPTION_ID " + "and AZURE_CERTIFICATE must be set for acceptance tests") } } @@ -78,6 +78,11 @@ func TestAzure_validateSettingsFile(t *testing.T) { t.Fatalf("Error creating temporary file with XML in TestAzure_validateSettingsFile: %s", err) } defer os.Remove(fx.Name()) + _, err = io.WriteString(fx, "") + if err != nil { + t.Fatalf("Error writing XML File: %s", err) + } + fx.Close() home, err := homedir.Dir() if err != nil { @@ -88,12 +93,11 @@ func TestAzure_validateSettingsFile(t *testing.T) { t.Fatalf("Error creating homedir-based temporary file: %s", err) } defer os.Remove(fh.Name()) - - _, err = io.WriteString(fx, "") + _, err = io.WriteString(fh, "") if err != nil { t.Fatalf("Error writing XML File: %s", err) } - fx.Close() + fh.Close() r := strings.NewReplacer(home, "~") homePath := r.Replace(fh.Name()) @@ -103,8 +107,8 @@ func TestAzure_validateSettingsFile(t *testing.T) { W int // expected count of warnings E int // expected count of errors }{ - {"test", 1, 1}, - {f.Name(), 1, 0}, + {"test", 0, 1}, + {f.Name(), 1, 1}, {fx.Name(), 1, 0}, {homePath, 1, 0}, {"", 0, 0}, @@ -114,10 +118,10 @@ func TestAzure_validateSettingsFile(t *testing.T) { w, e := validateSettingsFile(tc.Input, "") if len(w) != tc.W { - t.Errorf("Error in TestAzureValidateSettingsFile: input: %s , warnings: %#v, errors: %#v", tc.Input, w, e) + t.Errorf("Error in TestAzureValidateSettingsFile: input: %s , warnings: %v, errors: %v", tc.Input, w, e) } if len(e) != tc.E { - t.Errorf("Error in TestAzureValidateSettingsFile: input: %s , warnings: %#v, errors: %#v", tc.Input, w, e) + t.Errorf("Error in TestAzureValidateSettingsFile: input: %s , warnings: %v, errors: %v", tc.Input, w, e) } } } @@ -164,33 +168,8 @@ func TestAzure_providerConfigure(t *testing.T) { err = rp.Configure(terraform.NewResourceConfig(rawConfig)) meta := rp.(*schema.Provider).Meta() if (meta == nil) != tc.NilMeta { - t.Fatalf("expected NilMeta: %t, got meta: %#v", tc.NilMeta, meta) - } - } -} - -func TestAzure_isFile(t *testing.T) { - f, err := ioutil.TempFile("", "tf-test-file") - if err != nil { - t.Fatalf("Error creating temporary file with XML in TestAzure_isFile: %s", err) - } - cases := []struct { - Input string // String path to file - B bool // expected true/false - E bool // expect error - }{ - {"test", false, true}, - {f.Name(), true, false}, - } - - for _, tc := range cases { - x, y := isFile(tc.Input) - if tc.B != x { - t.Errorf("Error in TestAzure_isFile: input: %s , returned: %#v, expected: %#v", tc.Input, x, tc.B) - } - - if tc.E != (y != nil) { - t.Errorf("Error in TestAzure_isFile: input: %s , returned: %#v, expected: %#v", tc.Input, y, tc.E) + t.Fatalf("expected NilMeta: %t, got meta: %#v, settings_file: %q", + tc.NilMeta, meta, tc.SettingsFile) } } } diff --git a/website/source/docs/providers/azure/index.html.markdown b/website/source/docs/providers/azure/index.html.markdown index 5d7afdd20..0c8c09f28 100644 --- a/website/source/docs/providers/azure/index.html.markdown +++ b/website/source/docs/providers/azure/index.html.markdown @@ -33,11 +33,11 @@ resource "azure_instance" "web" { The following arguments are supported: -* `settings_file` - (Optional) Contents of a valid `publishsettings` file, used to - authenticate with the Azure API. You can download the settings file here: - https://manage.windowsazure.com/publishsettings. You must either provide - (or source from the `AZURE_SETTINGS_FILE` environment variable) a settings - file or both a `subscription_id` and `certificate`. +* `publish_settings` - (Optional) Contents of a valid `publishsettings` file, + used to authenticate with the Azure API. You can download the settings file + here: https://manage.windowsazure.com/publishsettings. You must either + provide publish settings or both a `subscription_id` and `certificate`. It + can also be sourced from the `AZURE_PUBLISH_SETTINGS` environment variable. * `subscription_id` - (Optional) The subscription ID to use. If a `settings_file` is not provided `subscription_id` is required. It can also @@ -47,6 +47,16 @@ The following arguments are supported: Azure API. If a `settings_file` is not provided `certificate` is required. It can also be sourced from the `AZURE_CERTIFICATE` environment variable. +These arguments are supported for backwards compatibility, and may be removed +in a future version: + +* `settings_file` - __Deprecated: please use `publish_settings` instead.__ + Path to or contents of a valid `publishsettings` file, used to + authenticate with the Azure API. You can download the settings file here: + https://manage.windowsazure.com/publishsettings. You must either provide + (or source from the `AZURE_SETTINGS_FILE` environment variable) a settings + file or both a `subscription_id` and `certificate`. + ## Testing: The following environment variables must be set for the running of the