Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter out from properties of tags object. #1107

Merged
merged 11 commits into from
Apr 20, 2018
Merged

Filter out from properties of tags object. #1107

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e480825
Filter out from properties of tags object.
metacpp Apr 10, 2018
030c397
Limit the filtering for $type to rule resource only and add validatio…
metacpp Apr 10, 2018
46fa676
Update acceptance tests to validate the $type in attributes.
metacpp Apr 10, 2018
7613d69
Add more non-exist check for metric alert rule.
metacpp Apr 10, 2018
bcd0e2f
Update the validation logic for test cases.
metacpp Apr 17, 2018
8d50852
Move filtering logic into seprated utility function for future reuse.
metacpp Apr 17, 2018
56c0805
Update the comment for filterTags function.
metacpp Apr 17, 2018
dac0ca1
Move validation of $type in customized tag naming into utility functi…
metacpp Apr 17, 2018
044447f
Remove the validation of $type in common function.
metacpp Apr 17, 2018
0edbdaf
add test case for filterTag function.
metacpp Apr 17, 2018
df7c9e7
Refactoring tags to match the others
tombuildsstuff Apr 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion azurerm/resource_arm_metric_alertrule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@metacpp metacpp Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return nil
}
Expand Down
2 changes: 2 additions & 0 deletions azurerm/resource_arm_metric_alertrule_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ func TestAccAzureRMMetricAlertRule_virtualMachineCpu(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMMetricAlertRuleExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "enabled", "false"),
resource.TestCheckNoResourceAttr(resourceName, "$type"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

),
},
},
Expand Down
19 changes: 16 additions & 3 deletions azurerm/tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package azurerm
import (
"errors"
"fmt"
"strings"

"github.com/hashicorp/terraform/helper/schema"
)
Expand Down Expand Up @@ -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 {
Expand All @@ -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))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@metacpp metacpp Apr 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. You can't directly use $type as naming of key in TF, you need to use "$type".
  2. "$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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


value, err := tagValueToString(v)
if err != nil {
es = append(es, err)
Expand All @@ -79,16 +84,24 @@ 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
}

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

// Only first optional parameter will be processed.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the rest of the parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

skipPropName := ""
if len(skipPropNames) > 0 && len(skipPropNames[0]) > 0 {
skipPropName = skipPropNames[0]
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for it being case insensitive

output[i] = *v
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code and tests.

}

d.Set("tags", output)
Expand Down