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

chore: move secret ignoring logic inside the scanner #1016

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

Conversation

gg-mmill
Copy link
Contributor

@gg-mmill gg-mmill commented Nov 20, 2024

Context

Currently, there are logic related to ignoring secrets in two places:

  • in the secret scanner
  • in the output handlers

What has been done

Move all the logic to the scanner:

  • "ignore known issues" now works in the same way than other ignore featues
  • Result objects now contain the list of PolicyBreaks (instead of ScanResult), as well as a dictionary containing the counts of ignored PolicyBreak by reason
  • As a result, there is no more result filtering in output handlers - they can still access the options and count of ignored issues if needed

Validation

Need to add tests for the scanner Done
Will also perform some manual validation

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@gg-mmill gg-mmill requested a review from a team as a code owner November 20, 2024 16:35
@gg-mmill gg-mmill self-assigned this Nov 20, 2024
@gg-mmill gg-mmill changed the title chore: move secret ignoring logic inside the scans chore: move secret ignoring logic inside the scanner Nov 20, 2024
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 96.77419% with 2 lines in your changes missing coverage. Please review.

Project coverage is 91.91%. Comparing base (b82d7ca) to head (f63ed44).

Files with missing lines Patch % Lines
...ticals/secret/output/secret_text_output_handler.py 90.90% 1 Missing ⚠️
...gshield/verticals/secret/secret_scan_collection.py 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1016      +/-   ##
==========================================
- Coverage   92.04%   91.91%   -0.13%     
==========================================
  Files         181      181              
  Lines        7704     7683      -21     
==========================================
- Hits         7091     7062      -29     
- Misses        613      621       +8     
Flag Coverage Δ
unittests 91.91% <96.77%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@gg-mmill gg-mmill force-pushed the mmillet/-/refactor_secret_ignoring branch 5 times, most recently from 5875d48 to 00144cd Compare November 22, 2024 09:40
@gg-mmill gg-mmill force-pushed the mmillet/-/refactor_secret_ignoring branch from 00144cd to 00dc593 Compare November 22, 2024 09:46
Copy link
Collaborator

@agateau-gg agateau-gg left a comment

Choose a reason for hiding this comment

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

Minor remarks, but this looks much cleaner! ✨

copy_result = copy.deepcopy(scan_result)
ignored_matches = [IgnoredMatch(name="", match=x) for x in ignores]
remove_ignored_from_result(copy_result, ignored_matches)
# print("CONTENT", scan_result.policy_breaks[0].matches[0].match)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 🧹

assert prev != tmpdir
with cd(tmpdir):
assert os.getcwd() == tmpdir
assert os.getcwd() == prev
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this is here, but more tests is good :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was in tests/unit/verticals/secret/test_scan.py, that I removed

ggshield/core/filter.py Outdated Show resolved Hide resolved
ggshield/verticals/secret/secret_scan_collection.py Outdated Show resolved Hide resolved
@property
def has_secrets(self) -> bool:
return (self.new_secrets_count + self.known_secrets_count) > 0
self.all_results = list(self.get_all_results())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not a fan of having results in both self.results and in self.all_results, do we really need this new member?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can do without - I feel like the interface of SecretScanCollection is kinda flawed no matter what we do though, because of how it's mixing up results, nested scan results, etc.

Changed to total_policy_breaks_count -> more correct, as Results can now be empty of policy breaks

@gg-mmill gg-mmill force-pushed the mmillet/-/refactor_secret_ignoring branch 3 times, most recently from b464bc7 to 58e2ed4 Compare November 22, 2024 16:29
@gg-mmill gg-mmill force-pushed the mmillet/-/refactor_secret_ignoring branch from 58e2ed4 to f63ed44 Compare November 22, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants