From e48082548b8cd39dbb80522e9c3f2eb83c56da8f Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Tue, 10 Apr 2018 09:10:43 -0700 Subject: [PATCH 01/10] Filter out from properties of tags object. --- azurerm/tags.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/azurerm/tags.go b/azurerm/tags.go index fdc6c6a0ed13..421c4acf803d 100644 --- a/azurerm/tags.go +++ b/azurerm/tags.go @@ -88,7 +88,10 @@ func flattenAndSetTags(d *schema.ResourceData, tagsMap map[string]*string) { output := make(map[string]interface{}, len(tagsMap)) for i, v := range tagsMap { - output[i] = *v + // Filter out $type from tags object to avoid unexpected change on plan. + if i != "$type" { + output[i] = *v + } } d.Set("tags", output) From 030c3971c3e7acae73002846e189f984a379cf56 Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Tue, 10 Apr 2018 11:38:21 -0700 Subject: [PATCH 02/10] Limit the filtering for $type to rule resource only and add validation logic to avoid $type from end user as tag name. --- azurerm/resource_arm_metric_alertrule.go | 2 +- azurerm/tags.go | 18 ++++++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/azurerm/resource_arm_metric_alertrule.go b/azurerm/resource_arm_metric_alertrule.go index d4eb1b7b67b9..1e80f9ec65d4 100644 --- a/azurerm/resource_arm_metric_alertrule.go +++ b/azurerm/resource_arm_metric_alertrule.go @@ -276,7 +276,7 @@ func resourceArmMetricAlertRuleRead(d *schema.ResourceData, meta interface{}) er d.Set("webhook_action", webhook_actions) } - flattenAndSetTags(d, resp.Tags) + flattenAndSetTags(d, resp.Tags, "$type") return nil } diff --git a/azurerm/tags.go b/azurerm/tags.go index 421c4acf803d..8125f6fc432b 100644 --- a/azurerm/tags.go +++ b/azurerm/tags.go @@ -3,6 +3,7 @@ package azurerm import ( "errors" "fmt" + "strings" "github.com/hashicorp/terraform/helper/schema" ) @@ -44,7 +45,7 @@ func tagValueToString(v interface{}) (string, error) { } } -func validateAzureRMTags(v interface{}, k string) (ws []string, es []error) { +func validateAzureRMTags(v interface{}, f string) (ws []string, es []error) { tagsMap := v.(map[string]interface{}) if len(tagsMap) > 15 { @@ -56,6 +57,10 @@ func validateAzureRMTags(v interface{}, k string) (ws []string, es []error) { es = append(es, fmt.Errorf("the maximum length for a tag key is 512 characters: %q is %d characters", k, len(k))) } + if strings.EqualFold(k, "$type") { + es = append(es, fmt.Errorf("the %q is not allowed as tag name", k)) + } + value, err := tagValueToString(v) if err != nil { es = append(es, err) @@ -79,7 +84,7 @@ func expandTags(tagsMap map[string]interface{}) map[string]*string { return output } -func flattenAndSetTags(d *schema.ResourceData, tagsMap map[string]*string) { +func flattenAndSetTags(d *schema.ResourceData, tagsMap map[string]*string, skipPropNames ...string) { if tagsMap == nil { d.Set("tags", make(map[string]interface{})) return @@ -87,9 +92,14 @@ func flattenAndSetTags(d *schema.ResourceData, tagsMap map[string]*string) { output := make(map[string]interface{}, len(tagsMap)) + // Only first optional parameter will be processed. + skipPropName := "" + if len(skipPropNames) > 0 && len(skipPropNames[0]) > 0 { + skipPropName = skipPropNames[0] + } + for i, v := range tagsMap { - // Filter out $type from tags object to avoid unexpected change on plan. - if i != "$type" { + if !strings.EqualFold(i, skipPropName) { output[i] = *v } } From 46fa676d253502aff47c5a22d8c9ff148abf81c2 Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Tue, 10 Apr 2018 11:48:16 -0700 Subject: [PATCH 03/10] Update acceptance tests to validate the $type in attributes. --- azurerm/resource_arm_metric_alertrule_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/azurerm/resource_arm_metric_alertrule_test.go b/azurerm/resource_arm_metric_alertrule_test.go index d9fef0c28c21..2f8205349cb7 100644 --- a/azurerm/resource_arm_metric_alertrule_test.go +++ b/azurerm/resource_arm_metric_alertrule_test.go @@ -34,6 +34,7 @@ func TestAccAzureRMMetricAlertRule_virtualMachineCpu(t *testing.T) { Check: resource.ComposeTestCheckFunc( testCheckAzureRMMetricAlertRuleExists(resourceName), resource.TestCheckResourceAttr(resourceName, "enabled", "false"), + resource.TestCheckNoResourceAttr(resourceName, "$type"), ), }, }, @@ -54,6 +55,7 @@ func TestAccAzureRMMetricAlertRule_sqlDatabaseStorage(t *testing.T) { Config: config, Check: resource.ComposeTestCheckFunc( testCheckAzureRMMetricAlertRuleExists(resourceName), + resource.TestCheckNoResourceAttr(resourceName, "$type"), ), }, }, From 7613d69970816d40e40fa6a8cda8984b781daec0 Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Tue, 10 Apr 2018 15:38:39 -0700 Subject: [PATCH 04/10] Add more non-exist check for metric alert rule. --- azurerm/import_arm_metric_alertrule_test.go | 3 +++ azurerm/resource_arm_metric_alertrule_test.go | 1 + 2 files changed, 4 insertions(+) diff --git a/azurerm/import_arm_metric_alertrule_test.go b/azurerm/import_arm_metric_alertrule_test.go index 61fa6fa29e3d..2a1996b8682d 100644 --- a/azurerm/import_arm_metric_alertrule_test.go +++ b/azurerm/import_arm_metric_alertrule_test.go @@ -25,6 +25,9 @@ func TestAccAzureRMMetricAlertRule_importVirtualMachineCpu(t *testing.T) { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckNoResourceAttr(resourceName, "$type"), + ), }, }, }) diff --git a/azurerm/resource_arm_metric_alertrule_test.go b/azurerm/resource_arm_metric_alertrule_test.go index 2f8205349cb7..2c155d0f25e6 100644 --- a/azurerm/resource_arm_metric_alertrule_test.go +++ b/azurerm/resource_arm_metric_alertrule_test.go @@ -27,6 +27,7 @@ func TestAccAzureRMMetricAlertRule_virtualMachineCpu(t *testing.T) { Check: resource.ComposeTestCheckFunc( testCheckAzureRMMetricAlertRuleExists(resourceName), resource.TestCheckResourceAttr(resourceName, "enabled", "true"), + resource.TestCheckNoResourceAttr(resourceName, "$type"), ), }, { From bcd0e2fa9531a8238ebef8faae0880b319a63708 Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Mon, 16 Apr 2018 17:59:13 -0700 Subject: [PATCH 05/10] Update the validation logic for test cases. --- azurerm/import_arm_metric_alertrule_test.go | 8 ++++++++ azurerm/resource_arm_metric_alertrule_test.go | 5 +++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/azurerm/import_arm_metric_alertrule_test.go b/azurerm/import_arm_metric_alertrule_test.go index 61fa6fa29e3d..deafc44276f4 100644 --- a/azurerm/import_arm_metric_alertrule_test.go +++ b/azurerm/import_arm_metric_alertrule_test.go @@ -20,11 +20,19 @@ func TestAccAzureRMMetricAlertRule_importVirtualMachineCpu(t *testing.T) { Steps: []resource.TestStep{ { Config: config, + Check: resource.ComposeTestCheckFunc( + testCheckAzureRMMetricAlertRuleExists(resourceName), + resource.TestCheckNoResourceAttr(resourceName, "tags.$type"), + ), }, { ResourceName: resourceName, ImportState: true, ImportStateVerify: true, + Check: resource.ComposeTestCheckFunc( + testCheckAzureRMMetricAlertRuleExists(resourceName), + resource.TestCheckNoResourceAttr(resourceName, "tags.$type"), + ), }, }, }) diff --git a/azurerm/resource_arm_metric_alertrule_test.go b/azurerm/resource_arm_metric_alertrule_test.go index 2f8205349cb7..0df638ff0d8c 100644 --- a/azurerm/resource_arm_metric_alertrule_test.go +++ b/azurerm/resource_arm_metric_alertrule_test.go @@ -27,6 +27,7 @@ func TestAccAzureRMMetricAlertRule_virtualMachineCpu(t *testing.T) { Check: resource.ComposeTestCheckFunc( testCheckAzureRMMetricAlertRuleExists(resourceName), resource.TestCheckResourceAttr(resourceName, "enabled", "true"), + resource.TestCheckNoResourceAttr(resourceName, "tags.$type"), ), }, { @@ -34,7 +35,7 @@ func TestAccAzureRMMetricAlertRule_virtualMachineCpu(t *testing.T) { Check: resource.ComposeTestCheckFunc( testCheckAzureRMMetricAlertRuleExists(resourceName), resource.TestCheckResourceAttr(resourceName, "enabled", "false"), - resource.TestCheckNoResourceAttr(resourceName, "$type"), + resource.TestCheckNoResourceAttr(resourceName, "tags.$type"), ), }, }, @@ -55,7 +56,7 @@ func TestAccAzureRMMetricAlertRule_sqlDatabaseStorage(t *testing.T) { Config: config, Check: resource.ComposeTestCheckFunc( testCheckAzureRMMetricAlertRuleExists(resourceName), - resource.TestCheckNoResourceAttr(resourceName, "$type"), + resource.TestCheckNoResourceAttr(resourceName, "tags.$type"), ), }, }, From 8d508524ace568c365e17f5041eacfeff786a1fe Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Mon, 16 Apr 2018 22:40:55 -0700 Subject: [PATCH 06/10] Move filtering logic into seprated utility function for future reuse. --- azurerm/resource_arm_metric_alertrule.go | 5 ++- azurerm/tags.go | 39 ++++++++++++++++-------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/azurerm/resource_arm_metric_alertrule.go b/azurerm/resource_arm_metric_alertrule.go index 1e80f9ec65d4..887318285422 100644 --- a/azurerm/resource_arm_metric_alertrule.go +++ b/azurerm/resource_arm_metric_alertrule.go @@ -276,7 +276,10 @@ func resourceArmMetricAlertRuleRead(d *schema.ResourceData, meta interface{}) er d.Set("webhook_action", webhook_actions) } - flattenAndSetTags(d, resp.Tags, "$type") + // Return a new tag map after filtering from specified tag list. + tagMap := filterTags(resp.Tags, "$type") + + flattenAndSetTags(d, tagMap) return nil } diff --git a/azurerm/tags.go b/azurerm/tags.go index 8125f6fc432b..f5281b36a3fa 100644 --- a/azurerm/tags.go +++ b/azurerm/tags.go @@ -84,25 +84,38 @@ func expandTags(tagsMap map[string]interface{}) map[string]*string { return output } -func flattenAndSetTags(d *schema.ResourceData, tagsMap map[string]*string, skipPropNames ...string) { - if tagsMap == nil { - d.Set("tags", make(map[string]interface{})) - return +func filterTags(tagsMap map[string]*string, tagNames ...string) map[string]*string { + if len(tagNames) == 0 { + return tagsMap } - output := make(map[string]interface{}, len(tagsMap)) - - // Only first optional parameter will be processed. - skipPropName := "" - if len(skipPropNames) > 0 && len(skipPropNames[0]) > 0 { - skipPropName = skipPropNames[0] + // Build the filter dictionary from passed tag names. + filterDict := make(map[string]bool) + for _, name := range tagNames { + if len(name) > 0 { + filterDict[strings.ToLower(name)] = true + } } - for i, v := range tagsMap { - if !strings.EqualFold(i, skipPropName) { - output[i] = *v + // Filter out tag if it exists(case insensitive) in the dictionary. + tagsRet := make(map[string]*string) + for k, v := range tagsMap { + if !filterDict[strings.ToLower(k)] { + tagsRet[k] = v } } + return tagsRet +} + +func flattenAndSetTags(d *schema.ResourceData, tagMap map[string]*string) { + + // If tagsMap is nil, len(tagsMap) will be 0. + output := make(map[string]interface{}, len(tagMap)) + + for i, v := range tagMap { + output[i] = *v + } + d.Set("tags", output) } From 56c08059f4c236931fb7f906e18c5ca67bbd4f3a Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Mon, 16 Apr 2018 22:43:00 -0700 Subject: [PATCH 07/10] Update the comment for filterTags function. --- azurerm/resource_arm_metric_alertrule.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azurerm/resource_arm_metric_alertrule.go b/azurerm/resource_arm_metric_alertrule.go index 887318285422..35ce4048aaac 100644 --- a/azurerm/resource_arm_metric_alertrule.go +++ b/azurerm/resource_arm_metric_alertrule.go @@ -276,7 +276,7 @@ func resourceArmMetricAlertRuleRead(d *schema.ResourceData, meta interface{}) er d.Set("webhook_action", webhook_actions) } - // Return a new tag map after filtering from specified tag list. + // Return a new tag map filtered by the specified tag names. tagMap := filterTags(resp.Tags, "$type") flattenAndSetTags(d, tagMap) From 044447f0b2cd6f14f25bdeae3d5b4ac0ace353b7 Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Tue, 17 Apr 2018 14:56:09 -0700 Subject: [PATCH 08/10] Remove the validation of $type in common function. --- azurerm/tags.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/azurerm/tags.go b/azurerm/tags.go index 480f6f2e2551..689fa74074d5 100644 --- a/azurerm/tags.go +++ b/azurerm/tags.go @@ -66,10 +66,6 @@ func validateAzureRMTags(v interface{}, f string) (ws []string, es []error) { es = append(es, fmt.Errorf("the maximum length for a tag key is 512 characters: %q is %d characters", k, len(k))) } - if strings.EqualFold(k, "$type") { - es = append(es, fmt.Errorf("the %q is not allowed as tag name", k)) - } - value, err := tagValueToString(v) if err != nil { es = append(es, err) From 0edbdaf6aef801d2c1f064e315f7bfedb2f8055d Mon Sep 17 00:00:00 2001 From: Su Shi <1684739+metacpp@users.noreply.github.com> Date: Tue, 17 Apr 2018 15:14:45 -0700 Subject: [PATCH 09/10] add test case for filterTag function. --- azurerm/tags_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/azurerm/tags_test.go b/azurerm/tags_test.go index 606f056b0758..bf95c29c5d6e 100644 --- a/azurerm/tags_test.go +++ b/azurerm/tags_test.go @@ -94,3 +94,22 @@ func TestExpandARMTags(t *testing.T) { } } } + +func TestFilterARMTags(t *testing.T) { + testData := make(map[string]*string) + valueData := [3]string{"value1", "value2", "value3"} + + testData["key1"] = &valueData[0] + testData["key2"] = &valueData[1] + testData["key3"] = &valueData[2] + + filtered := filterTags(testData, "key1", "key3", "") + + if len(filtered) != 1 { + t.Fatalf("Expected 1 result in filtered tag map, got %d", len(filtered)) + } + + if filtered["key2"] != &valueData[1] { + t.Fatalf("Expected %v in filtered tag map, got %v", valueData[1], *filtered["key2"]) + } +} From df7c9e7a21b47ce90ac934712c6a8601ce7332e2 Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Fri, 20 Apr 2018 14:50:59 +0200 Subject: [PATCH 10/10] Refactoring tags to match the others ``` $ acctests azurerm TestValidateMetricAlertRuleTags === RUN TestValidateMetricAlertRuleTags --- PASS: TestValidateMetricAlertRuleTags (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.025s ``` --- azurerm/resource_arm_metric_alertrule.go | 24 ++++++++- azurerm/resource_arm_metric_alertrule_test.go | 54 +++++++++++++++++++ azurerm/tags.go | 24 --------- 3 files changed, 77 insertions(+), 25 deletions(-) diff --git a/azurerm/resource_arm_metric_alertrule.go b/azurerm/resource_arm_metric_alertrule.go index 82018f065ad3..a8d0150fa6c8 100644 --- a/azurerm/resource_arm_metric_alertrule.go +++ b/azurerm/resource_arm_metric_alertrule.go @@ -4,6 +4,8 @@ import ( "fmt" "log" + "strings" + "github.com/Azure/azure-sdk-for-go/services/monitor/mgmt/2017-05-01-preview/insights" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/helper/validation" @@ -138,7 +140,12 @@ func resourceArmMetricAlertRule() *schema.Resource { }, }, - "tags": tagsForMetricAlertRuleSchema(), + "tags": { + Type: schema.TypeMap, + Optional: true, + Computed: true, + ValidateFunc: validateMetricAlertRuleTags, + }, }, } } @@ -414,3 +421,18 @@ func resourceGroupAndAlertRuleNameFromId(alertRuleId string) (string, string, er return resourceGroup, name, nil } + +func validateMetricAlertRuleTags(v interface{}, f string) (ws []string, es []error) { + // Normal validation required by any AzureRM resource. + ws, es = validateAzureRMTags(v, f) + + tagsMap := v.(map[string]interface{}) + + for k := range tagsMap { + if strings.EqualFold(k, "$type") { + es = append(es, fmt.Errorf("the %q is not allowed as tag name", k)) + } + } + + return ws, es +} diff --git a/azurerm/resource_arm_metric_alertrule_test.go b/azurerm/resource_arm_metric_alertrule_test.go index 0df638ff0d8c..c4e0a2254b21 100644 --- a/azurerm/resource_arm_metric_alertrule_test.go +++ b/azurerm/resource_arm_metric_alertrule_test.go @@ -11,6 +11,60 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" ) +func TestValidateMetricAlertRuleTags(t *testing.T) { + cases := []struct { + Name string + Value map[string]interface{} + ErrCount int + }{ + { + Name: "Single Valid", + Value: map[string]interface{}{ + "hello": "world", + }, + ErrCount: 0, + }, + { + Name: "Single Invalid", + Value: map[string]interface{}{ + "$Type": "hello/world", + }, + ErrCount: 1, + }, + { + Name: "Single Invalid lowercase", + Value: map[string]interface{}{ + "$type": "hello/world", + }, + ErrCount: 1, + }, + { + Name: "Multiple Valid", + Value: map[string]interface{}{ + "hello": "world", + "foo": "bar", + }, + ErrCount: 0, + }, + { + Name: "Multiple Invalid", + Value: map[string]interface{}{ + "hello": "world", + "$type": "Microsoft.Foo/Bar", + }, + ErrCount: 1, + }, + } + + for _, tc := range cases { + _, errors := validateMetricAlertRuleTags(tc.Value, "azurerm_metric_alert_rule") + + if len(errors) != tc.ErrCount { + t.Fatalf("Expected %q to return %d errors but returned %d", tc.Name, tc.ErrCount, len(errors)) + } + } +} + func TestAccAzureRMMetricAlertRule_virtualMachineCpu(t *testing.T) { resourceName := "azurerm_metric_alertrule.test" ri := acctest.RandInt() diff --git a/azurerm/tags.go b/azurerm/tags.go index 689fa74074d5..cbb70069b84d 100644 --- a/azurerm/tags.go +++ b/azurerm/tags.go @@ -34,15 +34,6 @@ func tagsForDataSourceSchema() *schema.Schema { } } -func tagsForMetricAlertRuleSchema() *schema.Schema { - return &schema.Schema{ - Type: schema.TypeMap, - Optional: true, - Computed: true, - ValidateFunc: validateMetricAlertRuleTags, - } -} - func tagValueToString(v interface{}) (string, error) { switch value := v.(type) { case string: @@ -77,21 +68,6 @@ func validateAzureRMTags(v interface{}, f string) (ws []string, es []error) { return ws, es } -func validateMetricAlertRuleTags(v interface{}, f string) (ws []string, es []error) { - // Normal validation required by any AzureRM resource. - ws, es = validateAzureRMTags(v, f) - - tagsMap := v.(map[string]interface{}) - - for k := range tagsMap { - if strings.EqualFold(k, "$type") { - es = append(es, fmt.Errorf("the %q is not allowed as tag name", k)) - } - } - - return ws, es -} - func expandTags(tagsMap map[string]interface{}) map[string]*string { output := make(map[string]*string, len(tagsMap))