-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 Pipfile Analyzer #2877
Add Pipfile Analyzer #2877
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.
Thank you for such a complete PR! Just one minor suggestions - unless you can think of any other way to obtain vendor information from pip...
core/src/main/java/org/owasp/dependencycheck/analyzer/PipfileAnalyzer.java
Show resolved
Hide resolved
@jeremylong do you know why the test failed? Looking into the logs it does not seem a problem with the commit. |
Apparently I broke the build. If you |
The idea is to 'git rebase main' the branch 'add-pipfile-support' and drop the last commit (ab94853)? |
ab94853
to
7533e9a
Compare
I think I did it. Now, the PR does not have an evidence for the VENDOR. Is it mandatory? I don't see that line in the PipAnalyzer for example. It only has these two lines: |
core/src/main/java/org/owasp/dependencycheck/analyzer/PipfileAnalyzer.java
Show resolved
Hide resolved
I just put the line adding the vendor evidence back. As to what happened - you likely accepted my change request in the github UI and did not pull the update back in before rebasing. Seriously - that you for such a complete PR! |
I'm sorry because I don't have a lot of experience with git and I got lost with the rebase. Now I see your commit in the pull request and the tests passing. Is everything ok now? Is there anything else that I have to do? |
Yup - everything is good. Thanks for the PR! |
Description of Change
Add a basic Pipfile Analyzer.
Have test cases been added to cover the new functionality?
yes