Skip to content

Conversation

michaelehab
Copy link
Collaborator

* Add GitHub OSV Live V2 Importer

* Add tests for the GitHub OSV Live V2 Importer

* Tested functionally using the Live Evaluation API in #1969

Signed-off-by: Michael Ehab Mikhail <[email protected]>
Signed-off-by: Michael Ehab Mikhail <[email protected]>
@michaelehab michaelehab changed the title Add GitHub OSV Live V2 Importer Pipeline #1904 Add GitHub OSV Live V2 Importer Pipeline Aug 18, 2025
Copy link
Collaborator

@ziadhany ziadhany left a comment

Choose a reason for hiding this comment

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

@michaelehab Nice work! Just a few nits for your consideration.

Comment on lines +179 to +180
if resp.status_code != 200:
return []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if resp.status_code != 200:
return []
response.raise_for_status()

date_str = adv.get("published") or adv.get("modified")

if date_str:
from datetime import datetime
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep all imports at the top, unless it’s a heavy library that is only used once. In that case, a local import is acceptable.

Suggested change
from datetime import datetime

Comment on lines +149 to +150
except Exception:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use specific exceptions for clarity and safety

return purl.name


def fetch_github_osv_advisories_for_purl(purl: PackageURL):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Simple and concise

Suggested change
def fetch_github_osv_advisories_for_purl(purl: PackageURL):
def fetch_osv_advisories(purl: PackageURL):

Comment on lines +174 to +176
pkg = {"ecosystem": ecosystem, "name": _osv_package_name(purl)}
# Query by package to get all advisories for that package; we filter GHSA below.
body = {"package": pkg}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pkg = {"ecosystem": ecosystem, "name": _osv_package_name(purl)}
# Query by package to get all advisories for that package; we filter GHSA below.
body = {"package": pkg}
body = {"package": {"ecosystem": ecosystem, "name": _osv_package_name(purl)}}


for adv in self.advisories:
adv_id = adv.get("id")
advisory_url = build_github_repo_advisory_url(adv, adv_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use consistent naming: choose either full names or abbreviations, but stay consistent

Suggested change
advisory_url = build_github_repo_advisory_url(adv, adv_id)
adv_url = build_github_repo_advisory_url(adv, adv_id)

}


def build_github_repo_advisory_url(adv: dict, adv_id: Optional[str]) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add some unit tests for the function build_github_repo_advisory_url.

from datetime import datetime

try:
dt = datetime.fromisoformat(date_str.replace("Z", "+00:00"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try using dateparser? I think it can simplify the logic there.

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.

2 participants