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.

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

Adding rule for azurerm resource missing tags #194

Merged

Conversation

guybartal
Copy link
Contributor

Resolves #133 based on the work done in terraform-linters/tflint#617 for AWS.

this PR allows you to get notified for resources with missing required tags:

rule "azurerm_resource_missing_tags" {
  enabled = true
  exclude = [] (list of resource types to exclude, optional)
  tags = ["Environment", "Department"]
}

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@guybartal
Copy link
Contributor Author

@wata727 , @hattan can you please review?

README.md Outdated Show resolved Hide resolved
Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

Nice! Added integration test would be very helpful :)

Left some comments, but mostly looks good. If you can check and fix it, I would like to merge this PR.

integration/basic/.tflint.hcl Outdated Show resolved Hide resolved
rules/azurerm_resource_missing_tags.go Outdated Show resolved Hide resolved
rules/azurerm_resource_missing_tags.go Outdated Show resolved Hide resolved
sort.Strings(missing)
wanted := strings.Join(missing, ", ")
issue := fmt.Sprintf("The resource is missing the following tags: %s.", wanted)
runner.EmitIssue(r, issue, location)
Copy link
Member

Choose a reason for hiding this comment

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

runner.EmitIssue returns an error, so handle the error in this function and handle it in the caller.
https://pkg.go.dev/github.com/terraform-linters/tflint-plugin-sdk/tflint#Runner.EmitIssue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

error handled both in this function and in the caller using runner.EnsureNoError

Copy link
Member

Choose a reason for hiding this comment

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

The EnsureNoError is a helper function for handling warnings returned by EvaluateExpr. So, there is no need to use it here. Return an error and check it in the caller as follows:

diff --git a/rules/azurerm_resource_missing_tags.go b/rules/azurerm_resource_missing_tags.go
index c6fc048..07bcbc7 100644
--- a/rules/azurerm_resource_missing_tags.go
+++ b/rules/azurerm_resource_missing_tags.go
@@ -87,15 +87,16 @@ func (r *AzurermResourceMissingTagsRule) Check(runner tflint.Runner) error {
 				wantType := cty.Map(cty.String)
 				err := runner.EvaluateExpr(attribute.Expr, &resourceTags, &tflint.EvaluateExprOption{WantType: &wantType})
 				err = runner.EnsureNoError(err, func() error {
-					r.emitIssue(runner, resourceTags, config, attribute.Expr.Range())
-					return nil
+					return r.emitIssue(runner, resourceTags, config, attribute.Expr.Range())
 				})
 				if err != nil {
 					return err
 				}
 			} else {
 				logger.Debug("checking", "resource type", resource.Labels[0], "resource name", resource.Labels[1])
-				r.emitIssue(runner, map[string]string{}, config, resource.DefRange)
+				if err := r.emitIssue(runner, map[string]string{}, config, resource.DefRange); err != nil {
+					return err
+				}
 			}
 		}
 	}
@@ -103,7 +104,7 @@ func (r *AzurermResourceMissingTagsRule) Check(runner tflint.Runner) error {
 	return nil
 }
 
-func (r *AzurermResourceMissingTagsRule) emitIssue(runner tflint.Runner, tags map[string]string, config azurermResourceTagsRuleConfig, location hcl.Range) {
+func (r *AzurermResourceMissingTagsRule) emitIssue(runner tflint.Runner, tags map[string]string, config azurermResourceTagsRuleConfig, location hcl.Range) error {
 	var missing []string
 	for _, tag := range config.Tags {
 		if _, ok := tags[tag]; !ok {
@@ -114,8 +115,9 @@ func (r *AzurermResourceMissingTagsRule) emitIssue(runner tflint.Runner, tags ma
 		sort.Strings(missing)
 		wanted := strings.Join(missing, ", ")
 		issue := fmt.Sprintf("The resource is missing the following tags: %s.", wanted)
-		runner.EmitIssue(r, issue, location)
+		return runner.EmitIssue(r, issue, location)
 	}
+	return nil
 }
 
 func stringInSlice(a string, list []string) bool {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, thanks!
fixed.

@@ -0,0 +1 @@
package tags
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if go generate could easily update the list of resources by adding a line like this:

//go:generate go run -tags generators ./generator/main.go

Copy link
Contributor Author

@guybartal guybartal Sep 4, 2022

Choose a reason for hiding this comment

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

go generate comment was added,
I had to put it after the package declaration, otherwise golint asks for special comment format Package tags...
by the way I've noticed that golint is deprecated and should be replaced (golang/go#38968)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we'll need to replace it with golangci-lint or simply turn it off. For rulesets, godoc is not so important, so we may not need to enforce this rule.

project/main.go Outdated Show resolved Hide resolved
@guybartal
Copy link
Contributor Author

Nice! Added integration test would be very helpful :)

Left some comments, but mostly looks good. If you can check and fix it, I would like to merge this PR.

Thanks for the feedback @WATA72 😊
I'll fix the code based on your comments and will update you so you could merge.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@guybartal guybartal requested a review from wata727 September 4, 2022 08:54
Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

Good, please check the error handling lastly.

@guybartal guybartal requested a review from wata727 September 4, 2022 10:20
@guybartal
Copy link
Contributor Author

Good, please check the error handling lastly.

did I fix it correctly?

Copy link
Member

@wata727 wata727 left a comment

Choose a reason for hiding this comment

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

Perfect. Thanks!

@wata727 wata727 merged commit 9ff5d59 into terraform-linters:master Sep 4, 2022
@guybartal
Copy link
Contributor Author

Perfect. Thanks!

Can you please create a new release so we could use this rule?

@guybartal guybartal deleted the azurerm_resource_missing_tags branch September 4, 2022 20:16
@wata727
Copy link
Member

wata727 commented Sep 5, 2022

Sure. I'm planning to release a new version within a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Required tags rule
3 participants