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 skipped dependencies to audit summary #141

Closed
tetsuo-cpp opened this issue Nov 28, 2021 · 3 comments · Fixed by #145
Closed

Add skipped dependencies to audit summary #141

tetsuo-cpp opened this issue Nov 28, 2021 · 3 comments · Fixed by #145
Assignees
Labels
component:output-formats Supported output formats enhancement New feature or request

Comments

@tetsuo-cpp
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The pip-audit summary should give a full picture of what happened in the audit. At the moment, pip-audit logs warnings whenever it skips over a dependency (due to an invalid version, can't be found on PyPI, etc), however the summary doesn't contain this information. If pip-audit can't audit a particular dependency, this should be reflected in the summary/JSON output somehow.

Describe the solution you'd like

There should be some indicator or section in the summary output containing the details of skipped dependencies.

@tetsuo-cpp
Copy link
Contributor Author

Hmm, it's a bit difficult to do this cleanly. The problem is essentially that if a dependency gets skipped for some reason, we need to propagate this all the way up to the top level of the API. Here are of the ideas I went through:

Make a "skipped dependency" type and optionally return it at each layer of the API

The obvious thing to do but it's invasive. Every level of the API needs to return something like Iterator[Union[Dependency, SkippedDependency]] or List[Union[Tuple[Dependency, List[VulnerabilityResult]]], SkippedDependency].

Make a larger type that involves vulnerability results and a list of skipped dependencies

Something like

@dataclass(frozen=True)
class VulnerabilityResults:
    vulns: List[Tuple[Dependency, List[VulnerabilityResult]]]
    skipped_deps: List[SkippedDep]

Doesn't really work well because we return Iterator a lot and stream results through the API which is useful for our spinner mechanism.

Hijack the AuditState type to register skipped dependencies

If we fail to audit a dependency, maybe we can do something like:

self.state.add_skipped_dep(dep_name)

And then pass the list of skipped dependencies to the formatting layer at the end. That way, our interfaces can look the same and we can reuse this "state" object that we pass around everywhere. Maybe a bit of a kludge since I'm repurposing the object to track skipped deps on top of the general audit state?

At this point I'm leaning towards the last option but open to opinions.

@woodruffw
Copy link
Member

woodruffw commented Nov 29, 2021

Yeah, this is tricky -- I'm inclined to say that we should change our Dependency model into a parent class, with children like this:

Dependency -> ResolvedDependency
Dependency -> SkippedDependency

Where ResolvedDependency will essentially be the current Dependency, and SkippedDependency will be something like:

class SkippedDependency(Dependency):
    # inherits `name` and `canonicalized_name()`
    
    skip_reason: SkipReason

...where SkipReason is maybe just str, or possibly an enum.Enum of well-defined reasons for skipping the dependency (e.g. an invalid version, a user has explicitly asked to skip a dep via a CLI option that we haven't implemented yet, etc.).

This complicates downstream consumption slightly, but it avoids using the AuditState as a hacky information channel. Thoughts?

@woodruffw
Copy link
Member

More braindump: we could have the top-level Depenency class also provide an is_skipped() interface, to simplify downstream handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:output-formats Supported output formats enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants