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.

Sign up for GitHub

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

Jump to bottom

handle excluded policy breaks #118

Merged
merged 3 commits into from
Oct 21, 2024
Merged

handle excluded policy breaks #118

merged 3 commits into from
Oct 21, 2024

Conversation

gg-mmill
Copy link
Contributor

@gg-mmill gg-mmill commented Oct 14, 2024 •
edited
Loading

  • fix load_default for incident_url (None instead of False)
  • Handle two new fields in PolicyBreaks, that will be returned by the scan routes.

A bit bothered by using two fields, but using a single exclude_reason does not seem right either.

@gg-mmill gg-mmill force-pushed the mmillet/-/handle_excluded_policy_breaks branch from e0befb9 to a0c1ea9 Compare October 14, 2024 08:20
@gg-mmill
Copy link
Contributor Author

We could validate that exclude_reason is not None only when is_excluded is True, but I'm not sure py-gitguardian is the right place for that, WDYT ?

@gg-mmill gg-mmill force-pushed the mmillet/-/handle_excluded_policy_breaks branch from a0c1ea9 to e1c8faa Compare October 14, 2024 08:23
@gg-mmill gg-mmill self-assigned this Oct 14, 2024
@gg-mmill gg-mmill force-pushed the mmillet/-/handle_excluded_policy_breaks branch from 7351154 to 90a4f88 Compare October 14, 2024 13:50
@gg-mmill
Copy link
Contributor Author

I will also need to make changes to the client, but I'll wait for the feature to be at least available in staging.

@gg-mmill gg-mmill force-pushed the mmillet/-/handle_excluded_policy_breaks branch from 0922fe2 to 97e8106 Compare October 15, 2024 15:37
@gg-mmill
Copy link
Contributor Author

I will also need to make changes to the client, but I'll wait for the feature to be at least available in staging.

@agateau-gg added the parameters to the client, should be ready now.

agateau-gg
agateau-gg approved these changes Oct 21, 2024
View reviewed changes
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.

Looks good!

agateau-gg
agateau-gg requested changes Oct 21, 2024
View reviewed changes
pygitguardian/client.py Outdated
@@ -357,6 +357,7 @@ def content_scan(
self,
document: str,
filename: Optional[str] = None,
all_secrets: Optional[bool] = None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you move the new argument at the end? If we don't then we break the API if the function is passed a value for extra_headers without using a keyword argument for it.

Actually it would be a good idea to add a *, after extra_headers so that we don't have this issue with future arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about multi_content_scan ? Should I make ignore_known_secrets mandatory kwarg ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it like that for now, so as not to introduce breaking changes:

        ignore_known_secrets: Optional[bool] = None,
        *,
        all_secrets: Optional[bool] = None,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense 👍

pygitguardian/client.py Outdated Show resolved Hide resolved
@gg-mmill gg-mmill force-pushed the mmillet/-/handle_excluded_policy_breaks branch from 1749b17 to 5170e1d Compare October 21, 2024 12:16
@gg-mmill gg-mmill force-pushed the mmillet/-/handle_excluded_policy_breaks branch from 5170e1d to f3cbef0 Compare October 21, 2024 12:44
agateau-gg
agateau-gg approved these changes Oct 21, 2024
View reviewed changes
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.

Looks good 👍

@agateau-gg agateau-gg merged commit a03cb02 into master Oct 21, 2024
19 checks passed
@agateau-gg agateau-gg deleted the mmillet/-/handle_excluded_policy_breaks branch October 21, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agateau-gg agateau-gg agateau-gg approved these changes

Assignees

@gg-mmill gg-mmill

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gg-mmill @agateau-gg