Skip to content

feat: source_uuid to multiscan #147

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alina-tuholukova-gg
Copy link
Contributor

When source_uuid parameter is provided for the multiscan endpoint, the incidents will be
created for the found secrets

@alina-tuholukova-gg alina-tuholukova-gg requested a review from a team as a code owner June 4, 2025 12:37
@alina-tuholukova-gg alina-tuholukova-gg self-assigned this Jun 4, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 4 lines in your changes missing coverage. Please review.

Project coverage is 95.57%. Comparing base (be13fe7) to head (8b80880).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
pygitguardian/client.py 82.35% 3 Missing ⚠️
pygitguardian/models.py 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   95.77%   95.57%   -0.20%     
==========================================
  Files           5        5              
  Lines        1208     1244      +36     
==========================================
+ Hits         1157     1189      +32     
- Misses         51       55       +4     
Flag Coverage Δ
unittests 95.57% <88.88%> (-0.20%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@alina-tuholukova-gg alina-tuholukova-gg force-pushed the alina/add-source-id-parameter-for-multiscan branch from 89889f1 to 90b4d91 Compare June 4, 2025 12:54
@alina-tuholukova-gg alina-tuholukova-gg force-pushed the alina/add-source-id-parameter-for-multiscan branch 2 times, most recently from 02eb61e to ea1cd34 Compare June 4, 2025 12:58
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, but I am curious about how one does obtain this source_uuid.

@alina-tuholukova-gg alina-tuholukova-gg force-pushed the alina/add-source-id-parameter-for-multiscan branch 2 times, most recently from 839ff83 to 0f89473 Compare June 23, 2025 16:15
@alina-tuholukova-gg
Copy link
Contributor Author

@agateau-gg I added new changes, we are finally using a new endpoint for issue creation, since we predict that the issue creation flow might change (new parameters specific only for issue creations etc). Could you please re-review the MR?

@alina-tuholukova-gg alina-tuholukova-gg force-pushed the alina/add-source-id-parameter-for-multiscan branch from 0f89473 to 8b80880 Compare June 25, 2025 14:07
Comment on lines +544 to +550
def scan_and_create_incidents(
self,
documents: List[Dict[str, str]],
source_uuid: UUID,
extra_headers: Optional[Dict[str, str]] = None,
params: Optional[Dict[str, Any]] = None,
) -> Union[Detail, MultiScanResult]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need that params argument? I think it would be better to have properly typed optional arguments than a pass-through like this.

Also, can you mark optional arguments as keyword-only by adding a * before extra_headers so that we can safely reorder them if need be?

Comment on lines +99 to +133
class DocumentsForIncidentCreationSchema(BaseSchema):
documents = fields.List(fields.Nested(DocumentSchema), required=True)
source_uuid = fields.UUID(required=True)

@post_dump
def transform_filename_to_document_identifier(
self, data: Dict[str, Any], **kwargs: Any
) -> Dict[str, Any]:
"""Transform filename field to document_identifier in the documents list"""
if "documents" in data:
for document in data["documents"]:
if "filename" in document:
document["document_identifier"] = document.pop("filename")
return data


class DocumentsForIncidentCreation(Base):
"""
DocumentsForIncidentCreation is a request object for communicating a list of documents
along with a source UUID to the API for incident creation

Attributes:
documents (List[Document]): list of documents to scan
source_uuid (UUID): UUID identifying the source
"""

SCHEMA = DocumentsForIncidentCreationSchema()

def __init__(self, documents: List[Document], source_uuid: UUID, **kwargs: Any):
super().__init__()
self.documents = documents
self.source_uuid = source_uuid

def __repr__(self) -> str:
return f"documents:{len(self.documents)}, source_uuid:{self.source_uuid}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can avoid adding these classes: scan_and_create_incidents() can do something like this:

payload = {
    "source_uuid": source_uuid,
    "documents": [{
        "document_identifier": x["filename],
        "document": x["document"],
    } for x in documents]
}

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.

3 participants