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

Capability to only run explicitly-provided rules #875

Merged

Conversation

markliederbach
Copy link
Contributor

@markliederbach markliederbach commented Aug 15, 2020

Context

We have a use case where we only want to run specific rules against our terraform repositories, and leave further linting to other workflows. As such, we need a way to disable all default rules, and only allow the ones we've specifically enabled via the config or CLI arguments.

What I Changed

A new argument/config option called --only (boolean) has been added, which is used in the NewRules part of the rules/provider.go to determine whether a rule should be enabled if it wasn't found in the provided config.

CLI example usage

tflint --only --enable-rule aws_instance_invalid_type
  • This will only run the provided rule, and not the other 770 default rules.

Config example usage

config {
  only = true
  # other options here...
}

rule "aws_instance_invalid_type" {
  enabled = true
}
  • Again, will only run the provided rule, and not the other 770 default rules.

Additionally, I've added a warning message when this mode is enabled and no rules were provided.

Tests have also been updated to reflect the new option.

@markliederbach markliederbach changed the title Allow capability to only run explicitly-provided rules Capability to only run explicitly-provided rules Aug 15, 2020
@wata727
Copy link
Member

wata727 commented Aug 16, 2020

Makes sense, but I think the --explicit-rules-mode option feels a bit redundant. How about passing a rule name with an option like --only?

e.g.

tflint --only aws_instance_invalid_type

@markliederbach
Copy link
Contributor Author

I like that, making the change.

@markliederbach
Copy link
Contributor Author

markliederbach commented Aug 16, 2020

Updated, let me know if there's anything else.

Whoops, sorry. I misread. You wanted it to replace the use of --enable-rule.

@markliederbach
Copy link
Contributor Author

@wata727 I've modified the CLI option to accept rules like the following:

tflint --only aws_instance_invalid_type

I've also added a warning message when --enable-rule or --disable-rule are used in addition to --only, because those options will be ignored as a result.

Let me know what you think.

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. I have some suggestions, please review this one.
It is also useful to debugging/developing rules 👍

docs/guides/advanced.md Outdated Show resolved Hide resolved
rules/provider.go Show resolved Hide resolved
@wata727
Copy link
Member

wata727 commented Aug 17, 2020

I also noticed that using the --only option and config file leads to a bit strange behavior. For example:

ec2.tf

resource "aws_instance" "foo" {
  instance_type = "t4.micro"
}

resource "aws_instance" "bar" {
  instance_type = "t1.micro"
}

.tflint.hcl

rule "aws_instance_invalid_type" {
  enabled = true
}
$ tflint --only aws_instance_previous_type
2 issue(s) found:

Error: "t4.micro" is an invalid value as instance_type (aws_instance_invalid_type)

  on ec2.tf line 2:
   2:   instance_type = "t4.micro"

Warning: "t1.micro" is previous generation instance type. (aws_instance_previous_type)

  on ec2.tf line 6:
   6:   instance_type = "t1.micro"

Reference: https://github.com/terraform-linters/tflint/blob/v0.18.0/docs/rules/aws_instance_previous_type.md

In this case, it may be useful to ignore the enabled = true of the config file. What do you think?

In order to do that, we have to change the Config generated from the config file by CLI options, rather than just merging two Config (config file and CLI options). This may require a slightly more complicated implementation.

@markliederbach
Copy link
Contributor Author

In this case, it may be useful to ignore the enabled = true of the config file. What do you think?

In order to do that, we have to change the Config generated from the config file by CLI options, rather than just merging two Config (config file and CLI options). This may require a slightly more complicated implementation.

This should be fixed now.

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 great contribution. I have some suggestions, but these are not blocking to merge.
Merging this pull request may be a little later, as some bug fixes are planned.

}
}
return ret
}
Copy link
Member

Choose a reason for hiding this comment

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

What about simply ignoring the former rule configs? The following is an example:

+ // All based configuration will be ignored
+ if !bDisabledByDefault {
	for k, v := range a {
		ret[k] = v
	}
+ }
for k, v := range b {
	// HACK: If you enable the rule through the CLI instead of the file, its hcl.Body will not contain valid range.
	// @see https://github.com/hashicorp/hcl/blob/v2.5.0/merged.go#L132-L135
+	if prevConfig, exists := a[k]; exists && v.Body.MissingItemRange().Filename == "<empty>" {
-	if prevConfig, exists := ret[k]; exists && v.Body.MissingItemRange().Filename == "<empty>" {
		ret[k] = v
		// Do not override body
		ret[k].Body = prevConfig.Body
	} else {
		ret[k] = v
	}
}

@@ -65,6 +66,23 @@ CLI flag: `--force`

Return zero exit status even if issues found. TFLint returns non-zero exit status by default. See [Exit statuses](../../README.md#exit-statuses).

## `only`
Copy link
Member

Choose a reason for hiding this comment

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

If you write about the key in the configuration file, it may be better to write disabled_by_default here (CLI flag description seems to be --only)

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

2 participants