Add `--minimum-failure-severity` flag, sets minimum issue severity for non-zero exit #1654

nicolajv · 2023-01-25T09:20:30Z

Resolves #1556

Add the --minimum-severity option allowing only certain severities to result in a non-zero exit code

wata727

Thank you for your contributions. Left some comments.

By the way, the naming may have room for improvement. The --minimum-seveirty is the same as present in tfsec, but tfsec filters the issues. This difference in behavior may confuse users.

We have several options:

Filter issues like tfsec
Rename (e.g. Ben's suggestion is --min-failure, any other good name would be nice)
Assume no confusion and adopt --minimum-severity

What do you think?

cmd/option.go

wata727 · 2023-01-25T16:49:16Z

cmd/inspect.go

+			}
+
+			for _, i := range issues {
+				if i.Rule.Severity() >= minSeverity {


The severity numbers are not intentional and are assigned mechanically, so avoid comparing them.

Well they are defined in order, just arguably backwards, and definitely not the way this code expects:

https://github.com/terraform-linters/tflint-plugin-sdk/blob/3721c4b92974d442bb3b0985f327b1b929a0e579/tflint/issue.go#L4-L13

Error is 0 and Notice is 2.

They're also re-declared which should be resolved:

tflint/tflint/issue.go

Lines 24 to 31 in 7b4785d

const (

// ERROR is possible errors

ERROR Severity = iota

// WARNING doesn't cause problem immediately, but not good

WARNING

// NOTICE is not important, it's mentioned

NOTICE

)

Ah I made a hasty assumption here, my bad! I do think it would make more sense to have them in an order from info to error, which would allow for these kinds of comparisons. For now I've implemented a function to convert them to the values I need, but it of course necessitates changing that method if you were to add new severities... But in general it looks like sweeping changes would be needed to add new severities, since there are switch statements using them all over the codebase.
I also removed the redeclared severity constant and references the SDK everywhere instead. Let me know if this is something that should be saved for another PR 🤪

cmd/inspect.go

integrationtest/cli/cli_test.go

bendrucker · 2023-01-25T18:11:17Z

My thought was minimum-failure-severity would be appropriate (as opposed to my original suggestion with error. I would assume --minimum-severity filters out issues below the specified severity from the output. Error is ambiguous (also the name of a severity) and failure is the best alternative I can up with to describe a non-zero exit code. It's fairly commonly used in high ranking pages in a Google search for bash non-zero exit code, maybe as much as error:

https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html
https://www.cyberciti.biz/faq/bash-get-exit-code-of-command/
https://tldp.org/LDP/abs/html/exit-status.html

bendrucker · 2023-01-25T18:17:17Z

cmd/inspect.go

+			}
+
+			for _, i := range issues {
+				if i.Rule.Severity() >= minSeverity {


Well they are defined in order, just arguably backwards, and definitely not the way this code expects:

https://github.com/terraform-linters/tflint-plugin-sdk/blob/3721c4b92974d442bb3b0985f327b1b929a0e579/tflint/issue.go#L4-L13

Error is 0 and Notice is 2.

They're also re-declared which should be resolved:

tflint/tflint/issue.go

Lines 24 to 31 in 7b4785d

const (

// ERROR is possible errors

ERROR Severity = iota

// WARNING doesn't cause problem immediately, but not good

WARNING

// NOTICE is not important, it's mentioned

NOTICE

)

bendrucker · 2023-01-25T18:22:36Z

cmd/inspect.go

+			var minSeverity tflint.Severity
+			switch strings.ToLower(opts.MinimumSeverity) {
+			case "warning":
+				minSeverity = 1
+			case "error":
+				minSeverity = 2


This should be handled alongside the severity type, e.g. func NewSeverity(s string) (Severity, error). An error would be returned if s is not a recognized severity. Handling it here is too error prone, e.g. if a new severity is added someone has to find and update this code.

I guess with the addition of choice to the struct tag an update is required anyway, but it would still be good to not hardcode this. At a minimum these severity codes should be exported/referenced instead of directly hardcoding the ints.

Like I mentioned above I think adding a new severity would be a task requiring many changes anyway, but I do agree the logic around severity should not be in inspect.go. I the functions which handle the conversion from the string into Severity and into in32 for comparison into the issue.go file. Not sure if this is the best space for it, but that seemed to be the home of severity.
I also removed the redeclaration of severity and changed references to point to the declaration in the SDK.

nicolajv · 2023-01-25T20:29:54Z

Thank you for your contributions. Left some comments.

By the way, the naming may have room for improvement. The --minimum-seveirty is the same as present in tfsec, but tfsec filters the issues. This difference in behavior may confuse users.

We have several options:

Filter issues like tfsec

Rename (e.g. Ben's suggestion is --min-failure, any other good name would be nice)

Assume no confusion and adopt --minimum-severity

What do you think?

I went with minimum-failure-severity for now as suggested by bendrucker. Makes sense to me to avoid the overlap with tfsec, especially as I think this feature should definitely not filter out issues; the use case this is important for is running in CI where you want to fail on a certain level of severity but continue and just print on the lower ones.

wata727

👍

bendrucker

Nice work on all the refactoring!

sergei-ivanov · 2023-02-24T22:00:45Z

Somehow this new flag does not seem to be working as expected in the latest release. If I only have a bunch of warnings, I get return code zero, no matter what I set minimum-failure-severity to. If I have any errors, I always have a non-zero return code.

bendrucker · 2023-02-25T00:13:40Z

Can you open an issue? There are integration tests against this behavior so it's not obvious how it wouldn't be working.

nicolajv added 2 commits January 25, 2023 10:14

Add --minimum-severity option

1d0d78d

Small performance improvement

644af8c

wata727 reviewed Jan 25, 2023

View reviewed changes

bendrucker reviewed Jan 25, 2023

View reviewed changes

Changes from PR review

033d1c0

nicolajv requested review from wata727 and bendrucker and removed request for wata727 and bendrucker January 25, 2023 20:21

wata727 approved these changes Jan 26, 2023

View reviewed changes

bendrucker changed the title ~~Add --minimum-severity option~~ Add --minimum-failure-severity flag, sets minimum issue severity for non-zero exit Jan 27, 2023

bendrucker approved these changes Jan 27, 2023

View reviewed changes

bendrucker merged commit 40689b0 into terraform-linters:master Jan 27, 2023

terraform-linters locked as resolved and limited conversation to collaborators Feb 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--minimum-failure-severity` flag, sets minimum issue severity for non-zero exit #1654

Add `--minimum-failure-severity` flag, sets minimum issue severity for non-zero exit #1654

nicolajv commented Jan 25, 2023

wata727 left a comment

wata727 Jan 25, 2023

bendrucker Jan 25, 2023

nicolajv Jan 25, 2023

bendrucker commented Jan 25, 2023

bendrucker Jan 25, 2023

bendrucker Jan 25, 2023

bendrucker Jan 25, 2023

nicolajv Jan 25, 2023

nicolajv commented Jan 25, 2023

wata727 left a comment

bendrucker left a comment

sergei-ivanov commented Feb 24, 2023

bendrucker commented Feb 25, 2023

	const (
	// ERROR is possible errors
	ERROR Severity = iota
	// WARNING doesn't cause problem immediately, but not good
	WARNING
	// NOTICE is not important, it's mentioned
	NOTICE
	)

Add --minimum-failure-severity flag, sets minimum issue severity for non-zero exit #1654

Add --minimum-failure-severity flag, sets minimum issue severity for non-zero exit #1654

Conversation

nicolajv commented Jan 25, 2023

wata727 left a comment

Choose a reason for hiding this comment

wata727 Jan 25, 2023

Choose a reason for hiding this comment

bendrucker Jan 25, 2023

Choose a reason for hiding this comment

nicolajv Jan 25, 2023

Choose a reason for hiding this comment

bendrucker commented Jan 25, 2023

bendrucker Jan 25, 2023

Choose a reason for hiding this comment

bendrucker Jan 25, 2023

Choose a reason for hiding this comment

bendrucker Jan 25, 2023

Choose a reason for hiding this comment

nicolajv Jan 25, 2023

Choose a reason for hiding this comment

nicolajv commented Jan 25, 2023

wata727 left a comment

Choose a reason for hiding this comment

bendrucker left a comment

Choose a reason for hiding this comment

sergei-ivanov commented Feb 24, 2023

bendrucker commented Feb 25, 2023

Add `--minimum-failure-severity` flag, sets minimum issue severity for non-zero exit #1654

Add `--minimum-failure-severity` flag, sets minimum issue severity for non-zero exit #1654