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

Add option --enable-all to enable all rules #1311

Closed
wants to merge 1 commit into from

Conversation

dvob
Copy link

@dvob dvob commented Feb 11, 2022

It would be nice to have a way to enable all available rules.
It seems that there are also other people requesting this feature #1056

@wata727
Copy link
Member

wata727 commented Feb 11, 2022

Thank you for your contribution!
Unfortunately, there are still many things to consider in order to achieve this goal.

First, this change only enables some of the Terraform rules built into the TFLint core, not rules provided by plugins. To enable all plugin rules, you need to add an RPC endpoint to plugin and call it from TFLint core.

Second, we can't enable rules that require settings like aws_s3_bucket_name automatically.
https://github.com/terraform-linters/tflint-ruleset-aws/blob/v0.12.0/docs/rules/aws_s3_bucket_name.md

For these reasons, I don't think we should add an option to enable all rules uniformly.
It is a historical reason that Terraform rules are included in the core, and I believe that it is better to cut this out into a ruleset so that it can be controlled in units of rulesets.

@dvob
Copy link
Author

dvob commented Feb 14, 2022

Hi @wata727 thank you for maintaining tflint.

I was looking for a way to find unused variables in Terraform files and found out that this should be possible with tflint via this issue hashicorp/terraform#11412 (comment)
I downloaded tflint but I did not understand why tflint did not report the unused variables. It was not clear to me that only certain rules are enabled by default. Then looked for a way to enable all rules. Whats actually the reason that not all builtin rules are enabled?

In my opinion it would be nice to have the the most common and provider independent checks built into the TFLlint core. This way using tflint would be super simple. You could download tflint and run it in the directory where you have your Terraform files without further configuration. And then, only if you have unwanted warnings or if you want to add additional checks, you can start to configure tflint to disable rules or add plugins.

For me it would be ok if this option would only enable the built in rules but I understand that this could be confusing for people which use plugins. So I understand if you don't want this change in tflint and its OK if you close this PR.

@wata727
Copy link
Member

wata727 commented Feb 14, 2022

How to design the default rules is a difficult issue. Since linter's default rules tend to strongly reflect the opinionated thought of the author, I took the policy of disabling all but the rules that were clearly useful to all users. It focuses on making it easy to introduce by minimizing warnings for already working Terraform configurations.

I learned from my experience in Go that detecting unused declarations is not always useful in a development environment, so I decided to disable this rule. But I still don't know if that was correct...

In the end, the answer I arrived at was a design like ESLint where the user could choose the set of rules themselves. In your example, you can start an inspection right away by installing the "strict" Terraform ruleset, which includes the unused declaration rule.

@gtirloni
Copy link

I'm just trying tflint on a large project and I was surprised that it did not detect anything wrong. It took me some time to realize that rules were not being enabled at all and I had to specify them one by one (either in the config file or command line). Had I not dug deeper into the issue I would have thought tflint didn't do anything at all.

It might be worth to enable at least all core rules that don't require customization from users.

@dvob
Copy link
Author

dvob commented Feb 24, 2022

@wata727 Somehow i missed your last reply.

How do I install the strict rule set?

I think your design decision is perfectly fine. But if you want to solve it like this i suggest that all rules are disabled by default and a warning is presented to the user if no rules are enabled or if no configuration was found.

This way if a user just downloads tflint and expect it to check certain basic rules they get a warning that they have to configure the rules themself. At least for me such a hint would have been very helpful.

@wata727
Copy link
Member

wata727 commented Feb 25, 2022

Thank you for the advice. To be sure, the current UX is not ideal. The biggest problem is that the terraform rules have been built into the core for historical reasons, and I believe that we can improve it to design a better UX.

@wata727
Copy link
Member

wata727 commented Mar 27, 2022

The current design cannot accept this feature and will close it. Thank you for your contribution.

@wata727 wata727 closed this Mar 27, 2022
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.

None yet

3 participants