-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(python): add support for uv dev and optional dependencies #8134
Conversation
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
@@ -27,7 +27,7 @@ The following table provides an outline of the features Trivy offers. | |||
| pip | requirements.txt | - | Include | - | ✓ | ✓ | | |||
| Pipenv | Pipfile.lock | ✓ | Include | - | ✓ | Not needed | | |||
| Poetry | poetry.lock | ✓ | Exclude | ✓ | - | Not needed | | |||
| uv | uv.lock | ✓ | Exclude | ✓ | - | Not needed | | |||
| uv | uv.lock | ✓ | Include | ✓ | - | Not needed | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why this page doesn't mention --include-dev-deps
like Node.js, but we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use link for Include
as in nodejs page?
// apk add jq | ||
// uv pip list --format json |jq -c 'sort_by(.name) | .[] | {"ID": (.name + "@" + .version), "Name": .name, "Version": .version}' | sed 's/$/,/' | sed 's/\"\([^"]*\)\":/\1:/g' | ||
|
||
// add a root project | ||
// fill in the relationships between the packages | ||
uvNormal = []ftypes.Package{ | ||
{ID: "normal@0.1.0", Name: "normal", Version: "0.1.0", Relationship: ftypes.RelationshipRoot}, | ||
{ID: "httpx@0.28.1", Name: "httpx", Version: "0.28.1", Relationship: ftypes.RelationshipDirect}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package is not marked as a development dependency. Is it correct? I'm concerned transitive dependencies introduced by direct development dependencies are not marked correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
httpx
is not a development dependency: uv add httpx==0.28.1 --extra socks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we newly introduce this dependency? I thought the test case was updated for optional or development dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we need it to test extra packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I should have added about test cases in the description. Yes, I added some more test cases:
- An optional dependency in the root package
- Direct dependency with an extra dependency that is optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks.
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
@nikpivkin Can you rebase the main branch? I think the linter will fail on |
I'm working on it right now |
@@ -27,7 +27,7 @@ The following table provides an outline of the features Trivy offers. | |||
| pip | requirements.txt | - | Include | - | ✓ | ✓ | | |||
| Pipenv | Pipfile.lock | ✓ | Include | - | ✓ | Not needed | | |||
| Poetry | poetry.lock | ✓ | Exclude | ✓ | - | Not needed | | |||
| uv | uv.lock | ✓ | Exclude | ✓ | - | Not needed | | |||
| uv | uv.lock | ✓ | Include | ✓ | - | Not needed | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use link for Include
as in nodejs page?
@@ -104,6 +104,8 @@ urllib3==1.26.15 | |||
`requirements.txt` files don't contain information about dependencies used for development. | |||
Trivy could detect vulnerabilities on the development packages, which not affect your production environment. | |||
|
|||
By default, Trivy doesn't report development dependencies. Use the `--include-dev-deps` flag to include them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivy doesn't support --include-dev-deps
flag for pip
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 55fb02f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use link for Include as in nodejs page?
By the way, why are include/exclude
used to link the table and sections and not the package manager names from the first column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, we don't show Dev dependencies.
This link is intended to draw users attention to this and direct them to the section about the file, where they can see that --include-dev-deps
flag needs to be used.
@@ -122,6 +124,8 @@ Trivy could detect vulnerabilities on the development packages, which not affect | |||
|
|||
License detection is not supported for `Pipenv`. | |||
|
|||
By default, Trivy doesn't report development dependencies. Use the `--include-dev-deps` flag to include them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 55fb02f
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
| pip | requirements.txt | - | [Include](#pip) | - | ✓ | ✓ | | ||
| Pipenv | Pipfile.lock | ✓ | [Include](#pipenv) | - | ✓ | Not needed | | ||
| Poetry | poetry.lock | ✓ | [Exclude](#poetry) | ✓ | - | Not needed | | ||
| uv | uv.lock | ✓ | [Include](#uv) | ✓ | - | Not needed | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you only need to add links if the --include-dev-deps
flag is used for the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In nodejs all package managers have links. I think the documentation should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we should update the nodejs documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In nodejs all package managers have links. I think the documentation should be consistent.
I missed that a link was added for Bun
. We can remove the link for that.
But for other files - yarn
, npm
and pnpm
support --include-dev-deps
. That's why they have links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll open a PR for that.
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -27,7 +27,7 @@ The following table provides an outline of the features Trivy offers. | |||
| pip | requirements.txt | - | Include | - | ✓ | ✓ | | |||
| Pipenv | Pipfile.lock | ✓ | Include | - | ✓ | Not needed | | |||
| Poetry | poetry.lock | ✓ | Exclude | ✓ | - | Not needed | | |||
| uv | uv.lock | ✓ | Exclude | ✓ | - | Not needed | | |||
| uv | uv.lock | ✓ | Include | ✓ | - | Not needed | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should write the default behavior, like Node.js. Or am I missing something?
| uv | uv.lock | ✓ | Include | ✓ | - | Not needed | | |
| uv | uv.lock | ✓ | Exclude | ✓ | - | Not needed | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed 5d65e2f
Signed-off-by: nikpivkin <nikita.pivkin@smartforce.io>
Description
This PR adds support for optional and dev dependencies. Optional dependencies are handled the same way for poetry. The optional dependencies of the root package are added as
direct
and the rest asindirect
.Without
--include-dev-deps
With
--include-dev-deps
pyptoject.toml
:Checklist