-
Notifications
You must be signed in to change notification settings - Fork 478
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: test handling of ~=
in requirements.txt and add it to docs
#3610
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.
The test looks good. I think we could probably tell users a bit more in the docs, so I've added some suggested changes for you with some extra text.
Co-authored-by: Terri Oda <terri@toybox.ca>
Co-authored-by: Terri Oda <terri@toybox.ca>
Thanks for the review @terriko. I've incorporated your suggestions. |
Updating branch for the tests. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3610 +/- ##
==========================================
+ Coverage 79.02% 79.11% +0.09%
==========================================
Files 799 799
Lines 11954 11954
Branches 1603 1603
==========================================
+ Hits 9447 9458 +11
+ Misses 2065 2058 -7
+ Partials 442 438 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 test works, but adding a new dependency to test_requirements.txt and having to install it is going to make this test take longer than it needs to because it'll have to make another network request to install stuff. Can you switch this to use something we're already installing? e.g. it should work just as well if you edit one of the other lines instead of adding lua-cjson. Also, maybe now is a good time to change this test so there are only two things in test_requirements.txt if we're just testing == and ~=? No point in making more system churn if we don't need to. I'd suggest you remove html5lib because I hate their version numbering scheme and just make the test use httplib2
and requests
.
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.
Thanks! This looks like it'll do the trick and might even make things run a bit faster.
…ntel#3610) Co-authored-by: Terri Oda <terri.oda@intel.com>
…ntel#3610) Co-authored-by: Terri Oda <terri.oda@intel.com>
fixes #3581 .
CVE binary tool can already handle version ranges specified with
~=
in the requirements.txt file since it utilizespip install --dry-run
.Whats added by this PR :
test_requirements.txt
file with~=
and updating the test to make sure it's parsed.~=
in requirements.txt can be supported by cve-bin-tool