feat(secret_scan): add all-secrets option #996
Conversation
gg-mmill
force-pushed
the
mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans
branch
6 times, most recently
from
4e88915 to
da87671
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
==========================================
- Coverage 92.03% 91.94% -0.10%
==========================================
Files 181 181
Lines 7701 7734 +33
==========================================
+ Hits 7088 7111 +23
- Misses 613 623 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gg-mmill
changed the title
gg-mmill
force-pushed
the
mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans
branch
3 times, most recently
from
a6e5c06 to
3b80646
Compare
Allows to display all secrets, even those excluded. In text output, adds a message when excluded secrets are found, to advertise the feature
Related to previous commit, cassettes needed to be updated because the query args to secret endpoint have changed
gg-mmill
force-pushed
the
mmillet/spi-515-add-all_secrets-option-to-ggshield-secret-scans
branch
from
3b80646 to
f2657b8
Compare
| @@ -139,6 +139,17 @@ def _banlist_detectors_callback( | |||
| ) | |||
|
|
|||
|
|
|||
| _all_secrets = click.option( | |||
| "--all-secrets", | |||
| is_flag=True, | |||
There was a problem hiding this comment.
You need to add a default=None otherwise the option won't work when it appears early on the command line (secret scan path --all-secrets . is OK, but secret --all-secrets scan path . is not).
| is_flag=True, | ||
| help=( | ||
| "Display all secrets, including those that have been ignored for example. " | ||
| "Note that all returned secrets are considered as alerts." |
There was a problem hiding this comment.
What is an "alert" in this context?
| @@ -136,15 +136,28 @@ def file_info( | |||
| filename: str, | |||
| incident_count: int, | |||
| incident_status: IncidentStatus = IncidentStatus.DETECTED, | |||
| number_of_hidden_secrets: int = 0, | |||
There was a problem hiding this comment.
naming: I would name this hidden_secret_count, it's shorter and consistent with incident_count.
| @@ -14,6 +14,8 @@ class FlattenedPolicyBreak(BaseSchema): | |||
| incident_url = fields.String(required=True, dump_default="") | |||
| incident_details = fields.Nested(SecretIncidentSchema) | |||
| known_secret = fields.Bool(required=True, dump_default=False) | |||
| is_excluded = fields.Bool(required=False) | |||
| exclude_reason = fields.String(required=False) | |||
There was a problem hiding this comment.
It's probably a bit late, but I just realized exclude_reason does not sound like correct English. I think it should be exclusion_reason.
| "--all-secrets", | ||
| is_flag=True, | ||
| help=( | ||
| "Display all secrets, including those that have been ignored for example. " |
There was a problem hiding this comment.
I think we need more than this example, otherwise users are going to wonder what secrets would be listed with this option that would not be listed otherwise.
Allows to display all secrets, even those excluded. Also adds a message when excluded secrets are found, to advertise the feature
Context
Allows to display all secrets, even those excluded.
Also adds a message when excluded secrets are found, to advertise the feature
What has been done
--all-secretoptionExclude reasonwhen appropriateValidation
Run ggshield scans on a file containing secrets with ignored / resolved issues, with and without
--all-secretsoptionPR check list
skip-changeloglabel has been added to the PR.