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

Allow rules to override the declared default severity level #771

Closed
wants to merge 1 commit into from

Conversation

mveitas
Copy link
Contributor

@mveitas mveitas commented May 23, 2020

This adds the ability for rules to override the severity.

An example usage - this will override the rule's severity to Error. By default the severity is declared as "Notice"

rule "terraform_naming_convention" {
  enabled  = true
  severity = "Error"
}

Fixes #715

@mveitas
Copy link
Contributor Author

mveitas commented May 23, 2020

None of the test have been updated as these changes require ever test to now specify the Serverity as part of the assertions for the emitted issues. I realize there are the other formats that need to be supported (json, checkstyle, etc)

Before updating all of the tests to check for the Severity field on the issue, I wanted to get some eyes on this approach. Most of the aws tests will be automatically be updated as they are generated via code. There are a large number of tests that will need to be manually updated

This is an example of the test failure:

 --- FAIL: Test_TerraformWorkspaceRemoteRule/terraform.workspace_in_locals_with_remote_backend (0.00s)
        testing.go:61: Expected issues are not matched:
               tflint.Issues{
                &{
                        Rule:     &terraformrules.TerraformWorkspaceRemoteRule{},
                        Message:  "terraform.workspace should not be used with a 'remote' backend",
            -           Severity: "",
            +           Severity: "Warning",
                        Range:    hcl.Range{Filename: "config.tf", Start: hcl.Pos{Line: 6, Column: 6}, End: hcl.Pos{Line: 6, Column: 25}},
                        Callers:  nil,
                },
              }

@mveitas
Copy link
Contributor Author

mveitas commented May 23, 2020

If this approach works the ability to add a custom message per rule would be able to be accomplished in the same manner: #768

@wata727
Copy link
Member

wata727 commented May 23, 2020

If you want to extend the severity or message, you should probably extend the Rule rather than the Issue. We should also consider how to pass options such as severity to the plugin.

To be honest, I'm wondering if TFLint should allow for severity or message extensions, but even if I do, I have a lot to think about and I've been pending.

@mveitas
Copy link
Contributor Author

mveitas commented May 23, 2020

I'm still getting familiar with both Go language and TFLint code (so excuse my asking so many questions).

I started down that road wasn't able to find a way as all of the information from the configuration is stored in the RuleConfig.

@mveitas
Copy link
Contributor Author

mveitas commented Jun 6, 2020

To be honest, I'm wondering if TFLint should allow for severity or message extensions, but even if I do, I have a lot to think about and I've been pending.

So here is a concrete use case for allowing to override the severity of messages:

Our company has a number of teams that are using Terraform and we recently started to enforce some basic standards with regards to formatting and style. We would love to have a consistent set of rules applied to our code, but this is going to take some time and different teams move at a different pace. We need the flexibility to set severity levels for different teams as they make this journey.

Any further thoughts?

@wata727
Copy link
Member

wata727 commented Jun 7, 2020

Umm...

In my opinion, if you want to extend this, I think it's better to write a ruleset plugin and use it. This is because my plan is that sometime I'd like to cut style rules as a plugin.

On the other hand, given that famous linters such as ESLint provide extensions of severity as configurations, I think that providing such extensions as a feature of TFLint is not such a bad idea.

However, some architectural changes are required to make this happen. First, when initializing the rule, we will need to accept the configuration as an argument. That seems like something we have to do, but nothing has been considered yet.

@wata727
Copy link
Member

wata727 commented Mar 27, 2022

This PR has been out of date since it was opened. I'll close this.

Messages and severity can be overridden by plugins. If you want to fine-tune the impact of style issues, I recommend extending the plugin. You may want to do this in a simpler way than a plugin, so we may consider a new feature in the future.

@wata727 wata727 closed this Mar 27, 2022
@MeNsaaH
Copy link

MeNsaaH commented Sep 28, 2022

@wata727 with this PR closed, how then do we override severity? This looks like a nice to have. Extending plugins is definitely not practical and maintainable.

@bendrucker
Copy link
Member

Please don't start conversations on closed PRs. If you have new information to add, you can add a comment to #715. Otherwise, if you just want to indicate that it's a feature that would be important to you, you can 👍🏻 the original post.

Yes, modifying the plugin is not a simple intervention.

@terraform-linters terraform-linters locked as off-topic and limited conversation to collaborators Sep 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow overriding severity for individual checks
4 participants