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

Add VulnerabilityResult.published field #404

Merged
merged 3 commits into from
Nov 7, 2022

Conversation

orsinium
Copy link
Contributor

@orsinium orsinium commented Nov 5, 2022

Add a new field published into VulnerabilityResult dataclass. The field holds information about when the vulnerability was published. That information is not used in the CLI or anywhere else yet, only emitted (from both PyPA and OSV sources).

Motivation: in a follow-up PR, I'm going to introduce a new CLI flag for filtering out only advisories published in a specific time window. The motivation for that, in turn, is to be able to have multiple CI jobs in user projects, one that warns about new vulnerabilities and another that fails for all vulnerabilities that weren't resolved in time. But that's another story. For now, just hold that information but don't act on it :)

Also, I've switched pip_audit/_service/interface.py to the new style of type annotations. Since you use from __future__ import annotaions, you can use any style of type annotations in any Python versions, type checkers will understand it. I think datetime | None is easier to understand than Optional[datetime].

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Some tiny nits, but LGTM overall! Thanks!

@woodruffw
Copy link
Member

Thanks for adding this field! This will be a nice addition, when plumbed through the different output formats.

Motivation: in a follow-up PR, I'm going to introduce a new CLI flag for filtering out only advisories published in a specific time window.

Would you mind opening a discussion issue for this feature first? So far we've tried to limit the number of "filter-style" CLI options we provide, under the reasoning that users who want filtering should use --format=json (or another machine-readable format) and do filtering as a post-processing step.

Copy link
Member

@di di left a comment

Choose a reason for hiding this comment

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

LGTM aside from unresolved discussion

pip_audit/_service/osv.py Show resolved Hide resolved
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM overall, we should probably clarify the OSV schema question but I'm okay with letting that float if you are @di 🙂

@woodruffw woodruffw merged commit a72604e into pypa:main Nov 7, 2022
@woodruffw
Copy link
Member

Thanks again @orsinium!

@orsinium orsinium deleted the expose-published branch November 8, 2022 07:41
@woodruffw woodruffw mentioned this pull request Nov 25, 2022
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