Filter out from properties of tags object. #1107

metacpp · 2018-04-10T16:14:58Z

This PR is used to fix #945 while running terraform plan after terraform apply due to "$type" in the tags property map.

tombuildsstuff

hi @metacpp

Thanks for this PR - as far as I know the $type tag is valid/could be used by users in other scenarios, as such can we filter this out just in the explicit resource where it needs to be filtered out? In addition can we add some (acceptance & unit) tests to verify this works as intended

Thanks!

tombuildsstuff · 2018-04-10T17:06:36Z

azurerm/tags.go

+		// Filter out $type from tags object to avoid unexpected change on plan.
+		if i != "$type" {
+			output[i] = *v
+		}


I think it's best to filter this on the individual resource where this needs to be filtered out (as people may have a legitimate use for the $type tag) - can we instead filter this out on the resource before calling this function? e.g.

tagsToOutput := make([]string, 0) for i, v := range resp.Tags { tag = *v // Filter out $type from tags object to avoid unexpected change on plan. if tag != "$type" { tagsToOutput[i] = tag } }

in addition - can we add some validation to ensure that the $type tag can't be added on that resource? We can do this using a ValidateFunc on the tags schema item for this resource :)

I've updated the code and tests.

…n logic to avoid $type from end user as tag name.

metacpp · 2018-04-10T19:07:09Z

@tombuildsstuff The PR is updated and I will share test results later.

metacpp · 2018-04-10T22:29:46Z

Most of failed tests are due to long running, the others has nothing to do with code change.

JunyiYi · 2018-04-13T19:29:05Z

azurerm/tags.go

 	for i, v := range tagsMap {
-		output[i] = *v
+		if !strings.EqualFold(i, skipPropName) {


Why using case insensitive comparison instead of "==" operator here?

"$type" seems to be reserved usage in PLAN, it should be case insensitive. Or, to be safer, we should use case sensitive instead.

👍 for it being case insensitive

JunyiYi · 2018-04-13T19:35:35Z

azurerm/tags.go

 	if tagsMap == nil {
 		d.Set("tags", make(map[string]interface{}))
 		return
 	}

 	output := make(map[string]interface{}, len(tagsMap))

+	// Only first optional parameter will be processed.


What about the rest of the parameters?

This is going to support optional parameter. A better way is to build a map instead. Let me refactor the code.

let's instead revert the changes to this method entirely and leave it in the single resource where this change is needed - this method's already doing too much

tombuildsstuff

hey @metacpp

Thanks for pushing those updates. Given this change is only needed in a single place (and the flattenAndSetTags method is already doing too much) - can we revert the changes to this method and instead limit the filtering of tags so that it's done within the resourceArmMetricAlertRuleRead method? I've left an example of what I mean for this in the review comments

Thanks!

tombuildsstuff · 2018-04-14T08:01:15Z

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")


this function's already doing too much as it is - can we filter out the tags to remove before passing this in? e.g.

tagsToOutput := make([]string, 0) for k, v := range resp.Tags { if strings.EqualFold(k, "$type") { continue } tagsToOutput[k] = v } flattenAndSetTags(d, tagsToOutput, "$type")

To be honest, I don't quite understand why the filtering logic breaks the KISS principle. Anyway, I moved the filtering logic into a utility function supporting tag list. If in the near future, any other resource needs to filter out any tags, it can reuse the logic instead of another copy and paste.

tombuildsstuff · 2018-04-14T08:02:05Z

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"),


this is looking for the field $type rather than the Tag $type - as such this won't work. Instead this wants to check for tags.$type

Good catch.

tombuildsstuff · 2018-04-14T08:02:07Z

azurerm/resource_arm_metric_alertrule_test.go

@@ -54,6 +55,7 @@ func TestAccAzureRMMetricAlertRule_sqlDatabaseStorage(t *testing.T) {
 				Config: config,
 				Check: resource.ComposeTestCheckFunc(
 					testCheckAzureRMMetricAlertRuleExists(resourceName),
+					resource.TestCheckNoResourceAttr(resourceName, "$type"),


this is looking for the field $type rather than the Tag $type - as such this won't work. Instead this wants to check for tags.$type

Good catch.

tombuildsstuff · 2018-04-14T08:02:57Z

azurerm/tags.go

 	if tagsMap == nil {
 		d.Set("tags", make(map[string]interface{}))
 		return
 	}

 	output := make(map[string]interface{}, len(tagsMap))

+	// Only first optional parameter will be processed.


let's instead revert the changes to this method entirely and leave it in the single resource where this change is needed - this method's already doing too much

metacpp · 2018-04-17T06:26:46Z

Updated the PR and reran the related tests without any regression:

=== RUN TestAccAzureRMMetricAlertRule_sqlDatabaseStorage
--- PASS: TestAccAzureRMMetricAlertRule_sqlDatabaseStorage (204.23s)
PASS
=== RUN TestAccAzureRMMetricAlertRule_importVirtualMachineCpu
--- PASS: TestAccAzureRMMetricAlertRule_importVirtualMachineCpu (500.87s)
PASS
=== RUN TestAccAzureRMMetricAlertRule_virtualMachineCpu
--- PASS: TestAccAzureRMMetricAlertRule_virtualMachineCpu (560.33s)
PASS
ok ../azurerm/test-azurerm 560.355s

Kicked off another comprehensive testing on Teamcity.

tombuildsstuff · 2018-04-17T13:58:21Z

azurerm/tags.go

@@ -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))
+		}


given this method is shared across all tags (and this validation is only specific to the metrics_alertrule resource) - can we make a specific validation method for the that resource? we can call the main validation function, and then check that $type isn't included?

Interesting, it seems that this file is inconsistent between local and remote. Let me check it and refresh it.

@tombuildsstuff
I did some investigation for $type while using it in tag naming for metric_alertrule resource:

You can't directly use $type as naming of key in TF, you need to use "$type".

"$type" is not acceptable by API:

Error: Error applying plan: 1 error(s) occurred: * azurerm_metric_alertrule.critical_cpu: 1 error(s) occurred: * azurerm_metric_alertrule.critical_cpu: insights.AlertRulesClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="UnsupportedRequestContent" Message="Request content is not well formed or supported."

In general, I feel that $type is not supposed as customized naming, it's reserved for internal usage, no matter from TF or Azure ARM API.

I understand that you're worried that we're forcing TF user to follow a more strict naming for all the resources, but, it also introduces inconsistency among all the resources. User will get confuse that sometimes it's allowed to use "$type" as tag name, the other times it's not allowed for some "unknown" story.

If we still want to limit the change for this special resource, we will need to create a new tag schema instead. Please see my updated PR, I don't it's worth taking this change.

That's true for this specific API but isn't the case across the whole of Azure, for instance I'm able to create a resource with $type as the key (note: this is double-escaped for HCL):

resource "azurerm_resource_group" "test" { name = "tharvey-dev3" location = "West US" tags { "$$type" = "hi" } }

in the case where this value isn't allowed we'll add validation logic to Terraform to the specific resources to handle this - but in general the key $type is a permitted value and as such we shouldn't obstruct that across the board.

Please review my latest change, it seems that even if the PR is updated, the comment is still shown on old commit.

…on and scope it only for metric alert rule.

metacpp · 2018-04-17T22:17:45Z

Added one unit test and ran it successfully.
=== RUN TestExpandARMTags
--- PASS: TestExpandARMTags (0.00s)

metacpp · 2018-04-18T18:56:42Z

@tombuildsstuff does it look good to you for this PR?

metacpp · 2018-04-19T16:45:34Z

@tombuildsstuff ran all the related tests and passed.

``` $ acctests azurerm TestValidateMetricAlertRuleTags === RUN TestValidateMetricAlertRuleTags --- PASS: TestValidateMetricAlertRuleTags (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.025s ```

tombuildsstuff

hi @metacpp

Thanks for pushing those updates. @metacpp I've pushed a commit to refactor this to match the other conventions but this otherwise now LGTM 👍 df7c9e7

Thanks!

tombuildsstuff · 2018-04-20T15:23:06Z

Tests pass:

ghost · 2020-03-31T15:06:41Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

Filter out from properties of tags object.

e480825

metacpp requested review from tombuildsstuff, JunyiYi and WodansSon April 10, 2018 16:15

metacpp added the service/monitoring label Apr 10, 2018

metacpp added this to the 1.3.3 milestone Apr 10, 2018

metacpp self-assigned this Apr 10, 2018

tombuildsstuff suggested changes Apr 10, 2018

View reviewed changes

tombuildsstuff added the enhancement label Apr 10, 2018

metacpp added 2 commits April 10, 2018 11:38

Limit the filtering for $type to rule resource only and add validatio…

030c397

…n logic to avoid $type from end user as tag name.

Update acceptance tests to validate the $type in attributes.

46fa676

Add more non-exist check for metric alert rule.

7613d69

JunyiYi reviewed Apr 13, 2018

View reviewed changes

tombuildsstuff suggested changes Apr 14, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Apr 14, 2018

metacpp added 3 commits April 16, 2018 17:59

Update the validation logic for test cases.

bcd0e2f

Move filtering logic into seprated utility function for future reuse.

8d50852

Update the comment for filterTags function.

56c0805

tombuildsstuff modified the milestones: 1.3.3, 1.4.0 Apr 17, 2018

tombuildsstuff reviewed Apr 17, 2018

View reviewed changes

metacpp added 3 commits April 17, 2018 13:48

Move validation of $type in customized tag naming into utility functi…

dac0ca1

…on and scope it only for metric alert rule.

Remove the validation of $type in common function.

044447f

add test case for filterTag function.

0edbdaf

metacpp requested a review from paultyng April 18, 2018 18:56

metacpp removed the waiting-response label Apr 18, 2018

tombuildsstuff removed the request for review from paultyng April 19, 2018 10:10

Refactoring tags to match the others

df7c9e7

``` $ acctests azurerm TestValidateMetricAlertRuleTags === RUN TestValidateMetricAlertRuleTags --- PASS: TestValidateMetricAlertRuleTags (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.025s ```

tombuildsstuff approved these changes Apr 20, 2018

View reviewed changes

tombuildsstuff merged commit ebba5af into master Apr 20, 2018

tombuildsstuff deleted the fix_alertrule branch April 20, 2018 15:47

tombuildsstuff added a commit that referenced this pull request Apr 20, 2018

Updating to include #1107

23e5915

tombuildsstuff added service/monitor and removed service/monitoring labels Apr 27, 2018

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter out from properties of tags object. #1107

Filter out from properties of tags object. #1107

metacpp commented Apr 10, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Apr 10, 2018

metacpp Apr 10, 2018

metacpp commented Apr 10, 2018

metacpp commented Apr 10, 2018 •

edited

Loading

JunyiYi Apr 13, 2018

metacpp Apr 14, 2018

tombuildsstuff Apr 14, 2018

JunyiYi Apr 13, 2018

metacpp Apr 14, 2018

tombuildsstuff Apr 14, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 14, 2018

metacpp Apr 17, 2018 •

edited

Loading

tombuildsstuff Apr 14, 2018

metacpp Apr 17, 2018

tombuildsstuff Apr 14, 2018

metacpp Apr 17, 2018

tombuildsstuff Apr 14, 2018

metacpp commented Apr 17, 2018

tombuildsstuff Apr 17, 2018

metacpp Apr 17, 2018

metacpp Apr 17, 2018 •

edited

Loading

tombuildsstuff Apr 19, 2018

metacpp Apr 19, 2018

metacpp commented Apr 17, 2018

metacpp commented Apr 18, 2018

metacpp commented Apr 19, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff commented Apr 20, 2018

ghost commented Mar 31, 2020

Filter out from properties of tags object. #1107

Filter out from properties of tags object. #1107

Conversation

metacpp commented Apr 10, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp commented Apr 10, 2018

metacpp commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metacpp commented Apr 17, 2018

metacpp commented Apr 18, 2018

metacpp commented Apr 19, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Apr 20, 2018

ghost commented Mar 31, 2020

metacpp commented Apr 10, 2018 •

edited

Loading

metacpp commented Apr 10, 2018 •

edited

Loading

metacpp Apr 17, 2018 •

edited

Loading

metacpp Apr 17, 2018 •

edited

Loading

metacpp commented Apr 19, 2018 •

edited

Loading