-
Notifications
You must be signed in to change notification settings - Fork 26
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
Introduce "preset" config #3
Conversation
rules.NewTerraformTypedVariablesRule(), | ||
rules.NewTerraformUnusedDeclarationsRule(), | ||
rules.NewTerraformUnusedRequiredProvidersRule(), | ||
rules.NewTerraformWorkspaceRemoteRule(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unlike the current settings, the following are enabled by default:
terraform_deprecated_index
terraform_required_providers
terraform_required_version
terraform_typed_variables
terraform_unused_declarations
terraform_unused_required_providers
These are the rules that I consider to be recommended even for senior engineers by my standards.
However, there are issues with https://github.com/terraform-linters/tflint/issues/1328 and https://github.com/terraform-linters/tflint/blob/v0.39.3/docs/rules/terraform_unused_declarations.md#how-to-fix, and the checks aren't perfect, so I'm not sure if these should be the default.
@bendrucker What do you think? If you have any other thoughts on the default rules, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd say terraform_unused_required_providers
needs to have the issue of implicit provider passing into modules addressed before it comes on by default. The others seem like good defaults for a "recommended" config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we will not include terraform_unused_required_providers
in the default for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think the concept of ruleset presets works well.
See also terraform-linters/tflint#1056
This PR introduces the concept of "preset" to the ruleset config. The preset is for naming a group of rules so that you can use the rules you need without having to configure each rule individually. The idea was inspired by ESLint's "extends": "eslint:recommended".
https://eslint.org/docs/latest/user-guide/configuring/configuration-files#extending-configuration-files
This enables all rules that were previously disabled by default, and makes all rules available if not set. If you want to enable only some rules as before, set "recommended" to preset.
For compatibility, default to adopt
preset="recommended"
if noplugin "terraform"
is declared. In other words, the rules that are enabled by default aren't much different than they used to be. However, at this time, we will review the rules that correspond to "recommended", so there may be differences in behavior.