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

feat(general): Allow skipping multiple checks in a single line #6622

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

shoshiGit
Copy link
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

This pull request enhances Checkov to support skipping multiple checks in a single line for Terraform configurations. Currently, individual skip comments are required for each check, which can be cumbersome. This enhancement allows specifying multiple checks to skip in a single line.

Fixes # #5381

Changes made:

Added functionality to parse multiple checks in the checkov:skip comment.
Updated documentation to reflect the new capability.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes

Copy link
Contributor

@ChanochShayner ChanochShayner left a comment

Choose a reason for hiding this comment

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

Very Nice! 🥇


def test(self) -> None:
# given
test_files_dir = Path(__file__).parent / "a example skip"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
test_files_dir = Path(__file__).parent / "a example skip"
test_files_dir = Path(__file__).parent / "a_example_skip"

Please use one word in the file names.

report = Runner().run(root_folder=str(test_files_dir), runner_filter=RunnerFilter(checks=[]))

# then
summary = report.get_summary()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please assert the skipped part in the reports by resource -
We want to be sure default and skip_invalid are with one skip, and skip_more_than_one is with 2 skips.

location = "azurerm_resource_group.example.location"
account_tier = "Standard"
account_replication_type = "GRS"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if you could create another resource that we are skipping multiple checks (maybe all the checks that the resource should fail on).

@ChanochShayner
Copy link
Contributor

Another point -
Please do the new regex and comment extraction behind an environment variable -

should_allow_multi_checks_skip = strtobool(os.getenv('CHECKOV_ALLOW_SKIP_MULTIPLE_ONE_LINE', 'False'))

And do every change by condition if should_allow_multi_checks_skip is True.

"""
should_allow_multi_checks_skip = bool(os.getenv('CHECKOV_ALLOW_SKIP_MULTIPLE_ONE_LINE', 'False'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
should_allow_multi_checks_skip = bool(os.getenv('CHECKOV_ALLOW_SKIP_MULTIPLE_ONE_LINE', 'False'))
should_allow_multi_checks_skip = strtobool(os.getenv('CHECKOV_ALLOW_SKIP_MULTIPLE_ONE_LINE', 'False'))

bool('False') equal to True.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants