-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: Packagesprops support #5605
feat: Packagesprops support #5605
Conversation
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.
Hello @yuriShafet
Thanks for your work!
Left few comments.
Also 2 moments:
- please take a look at
purl
package. I think you need to add new type. - can you add small integration test to repo_test?
@DmitriyLewen
It seems packagesprops logic is not registered somewhere. |
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.
@yuriShafet
Thanks for your work!
To save us time, I did a little refactoring and added a few comments.
Can you check and confirm that it is working correctly?
@@ -34,6 +34,7 @@ On the other hand, when the target is a post-build artifact, like a container im | |||
| [.NET](dotnet.md) | packages.lock.json | ✅ | ✅ | ✅ | ✅ | | |||
| | packages.config | ✅ | ✅ | ✅ | ✅ | | |||
| | .deps.json | ✅ | ✅ | ✅ | ✅ | | |||
| | *Packages.props[^11] | ✅ | ✅ | ✅ | ✅ | |
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.
We divide all language files into pre-build
and post-build
- https://aquasecurity.github.io/trivy/v0.47/docs/coverage/language/#supported-languages
Perhaps we should only scan Packages.props files in fs/repo mode?
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.
It is unlikely that packages.props
will appear in post build phase, but I expect from packages.config
would show the same behavior.
If we support packages.config
in post-build phase, we should also support packages.props
in post-build phase.
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 the way, same thing could be said about packages.lock.json
. I think there will be mostly .dll files in post-build.
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.
Also, If I use my image for development purposes (For example I pre-build an image with the source code to run unit tests or some static code analysis), it is actually make sense to look for these artifacts
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 are right.
Looks like we need to use pre-build for all supported NuGet files.
But then NuGet packages will not be scanned in image
mode.
Okay, let's use same logic for *Packages.props
.
pkg/fanal/analyzer/language/dotnet/packagesprops/packagesprops.go
Outdated
Show resolved
Hide resolved
@DmitriyLewen It seems that after your refactor all tests pass. Thanks. Are there any more questions/comments? |
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.
@yuriShafet Thanks for your work and help!
I don't have more questions.
@knqyf263 I approved this PR. Take a look, when you have time, please.
Description
Adding support and documentation for Directory.packages.props and packages.props.
I am not 100% sure about documentation about transitive dependencies support.
Related issues
*Packages.props
files. #5403Related PRs
example of work:
Checklist