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

New rule: aws_provider_missing_tags #633

Merged
merged 10 commits into from
May 4, 2024

Conversation

bootswithdefer
Copy link
Contributor

Adds an aws_provider_missing_tags rule, which requires specific tags for AWS provider default_tags blocks. The aws_resource_missing_tags rule is not sufficient, it enforces the set of tags that end up on the resource, but it doesn't enforce the practice of applying them via default_tags instead of resource tags. Using default tags results in more DRY terraform.

I would see this being used in conjunction with aws_resource_missing_tags, where aws_provider_missing_tags is used for common tags and aws_resource_missing_tags is used for more resource-specific tags (like Name).

"github.com/terraform-linters/tflint-plugin-sdk/tflint"
"github.com/terraform-linters/tflint-ruleset-aws/project"
"github.com/zclconf/go-cty/cty"
"golang.org/x/exp/slices"
Copy link
Member

Choose a reason for hiding this comment

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

The slices package is already available as a standard package in Go 1.21.
https://pkg.go.dev/slices@go1.21

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


// Name returns the rule name
func (r *AwsProviderMissingTagsRule) Name() string {
return "aws_provider_missing_tags"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe aws_provider_missing_default_tags is a better name. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

Expected: helper.Issues{
{
Rule: NewAwsProviderMissingTagsRule(),
Message: "The provider `default` is missing the `default_tags` block",
Copy link
Member

Choose a reason for hiding this comment

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

You may be wondering what this "default" is if you haven't set up an alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the output, though it's kind of awkward

sort.Strings(missing)
wanted := strings.Join(missing, ", ")
issue := fmt.Sprintf("The provider 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.

Please check the error return value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

var providerTags []string
attr, ok := block.Body.Attributes[providerTagsAttributeName]
if !ok {
r.emitIssue(runner, providerTags, config, provider.DefRange)
Copy link
Member

Choose a reason for hiding this comment

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

block.DefRange would be better than provider.DefRange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no block, this case is for when default_tags doesn't exist.

}

// Check tags
r.emitIssue(runner, providerTags, config, provider.DefRange)
Copy link
Member

Choose a reason for hiding this comment

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

attr.Range would be better than provider.DefRange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed


## Why

You want to set a standardized set of tags for your AWS resources via the provider default tags.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more helpful to mention its use with the aws_resource_missing_tags rule in specific scenarios.
At least, this is what I thought about this rule first:

  • Why not aws_resource_missing_tags rule?
  • What is the difference from aws_resource_missing_tags rule?
  • When should I use this rule?

It's a good idea to include answers to these questions in the "Why" section. Perhaps the explanation in the PR will be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fleshed out the doc

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.

Looks good. Thanks!

@wata727 wata727 merged commit 51d759d into terraform-linters:master May 4, 2024
8 checks passed
@max-rocket-internet
Copy link

I love this rule, thanks @bootswithdefer 💖

@max-rocket-internet
Copy link

@wata727 could create a new tag so we can use it? AFAIK we can't add a plugin with a commit ID as the version?

@wata727
Copy link
Member

wata727 commented Jun 8, 2024

v0.32.0 has been released.
https://github.com/terraform-linters/tflint-ruleset-aws/releases/tag/v0.32.0

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.

3 participants