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

feat: Added default tags functionality #489

Merged
merged 7 commits into from
Jun 13, 2023

Conversation

JorgeReus
Copy link
Contributor

Default Tags functionality added into the missing tags rule

I've added the functionality and tests to allow using default tags and not come across into errors. I work with a lot of repos in which we comply with the tagging standard using default_tags.

This functionality will help us to detect uncompliant resources before we deploy them (we use cloud custodian)

Instead of creating an extra rule, I updated the existing one, this due to the keep only one rule regarding missing tags and allow others to use this feature without adding another rule to the tflint config.

LMK if any changes need to be made

fixes: #192

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.

Thank you for your contribution.
Left some comments. Feel free to ask questions.

rules/aws_resource_missing_tags.go Outdated Show resolved Hide resolved
rules/aws_resource_missing_tags.go Outdated Show resolved Hide resolved
rules/aws_resource_missing_tags.go Show resolved Hide resolved
rules/aws_resource_missing_tags.go Outdated Show resolved Hide resolved
rules/aws_resource_missing_tags.go Outdated Show resolved Hide resolved
rules/aws_resource_missing_tags_test.go Show resolved Hide resolved
@nitrocode
Copy link

Thank you @wata727 for reviewing this! And thank you @JorgeReus for adding this. Please see the comments above. I would love to see this in the next version of this ruleset as many people will start using default_tags now that the feature is going to be heavily adopted thanks to the aws 5.x provider changes. 😄

@JorgeReus
Copy link
Contributor Author

Hey there!

Thanks for the review @wata727, didn't had much time last week to fix the PR, but hopefully I will this one.

@nitrocode Sure! Will do my best.

- Removed deprecated runner.EnsureNoError calls
- Removed explicit pass of types to EvaluateExpr calls
@JorgeReus
Copy link
Contributor Author

JorgeReus commented Jun 8, 2023

Done fixing the comments, thanks @wata727!

@bendrucker bendrucker requested a review from wata727 June 9, 2023 00:06
@JorgeReus JorgeReus marked this pull request as draft June 10, 2023 23:52
@JorgeReus JorgeReus force-pushed the feat/default_tags branch 2 times, most recently from 5f97fb9 to 2a3e7ba Compare June 11, 2023 00:01
@JorgeReus
Copy link
Contributor Author

Done for the second round of checks! Still familiarizing with the runner API, thanks!

BTW, I downloaded the rules/models/aws-sdk-go submodule and runned go generate, LMK if I need to rebase to erase those files from the PR, was trying to test locally with make install, but tflint was nagging about to finding the rule.

@JorgeReus JorgeReus marked this pull request as ready for review June 11, 2023 00:47
@wata727
Copy link
Member

wata727 commented Jun 12, 2023

LMK if I need to rebase to erase those files from the PR

Yes, we don't want to include irrelevant changes in this PR, so please rebase or clean up the diff.

@JorgeReus JorgeReus marked this pull request as draft June 12, 2023 17:18
@JorgeReus JorgeReus force-pushed the feat/default_tags branch 3 times, most recently from 2f5ddc3 to 467e17e Compare June 12, 2023 17:32
- Added the RaiseErr field in the test table struct, this is due to
  having the ability of checking errors apart of helper issues, also
  checked if the provider existed using the mentioned mechanism
- Erased unecessary error checking when decoding provider config
@JorgeReus
Copy link
Contributor Author

LMK if I need to rebase to erase those files from the PR

Yes, we don't want to include irrelevant changes in this PR, so please rebase or clean up the diff.

Done with the git wizardry, and thanks again for the review!

LMK if more changes need to be made!

@JorgeReus JorgeReus marked this pull request as ready for review June 12, 2023 17:57
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 👍

@wata727 wata727 merged commit 89c6ed5 into terraform-linters:master Jun 13, 2023
@JorgeReus JorgeReus deleted the feat/default_tags branch June 13, 2023 15:00
@nitrocode
Copy link

Awesome! Nice work all!

@nitrocode
Copy link

nitrocode commented Jun 13, 2023

Hmm this test fails after this PR merged https://github.com/terraform-linters/tflint-ruleset-aws/actions/runs/5256902432/jobs/9498886637

Generating rule for `aws_launch_configuration.spot_price`
panic: `aws_launch_configuration.vpc_classic_link_security_groups` not found in the Terraform schema

goroutine 1 [running]:
main.fetchSchema({0xc0001ecf18, 0x18}, {0xc0001ea960, 0x20}, 0xc00373fd20?, {0x66d220?})
	/home/runner/work/tflint-ruleset-aws/tflint-ruleset-aws/rules/models/generator/main.go:109 +0x3e8
main.main()
	/home/runner/work/tflint-ruleset-aws/tflint-ruleset-aws/rules/models/generator/main.go:81 +0x7c8
exit status 2

Unsure if it's related to this PR or something else in the code base.

Edit: nvm, it looks like it failed in the last 3 PR merges - this one and 2 dependabot PRs #498 #497

https://github.com/terraform-linters/tflint-ruleset-aws/commits/master

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.

Feat => v2:aws_resource_missing_tags errors when tags are provided via default_tags mechanism
3 participants