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

fix: create new version comparison function #3470

Merged
merged 15 commits into from
Nov 16, 2023
Merged

Conversation

terriko
Copy link
Contributor

@terriko terriko commented Oct 25, 2023

We had been using packaging's version parsing tools, but as they move more towards pep440 compliance they aren't as useful for comparing arbitrary versions that may not follow the same scheme. This moves us to our own function. It may need some further tweaking for special cases such as release candidates or dev versions.

This is a replacement for #3430 because cmp_version didn't work out as well as I'd hoped.

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2023

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (143712e) 78.32% compared to head (8044a05) 78.23%.

Files Patch % Lines
cve_bin_tool/version_compare.py 82.92% 10 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3470      +/-   ##
==========================================
- Coverage   78.32%   78.23%   -0.09%     
==========================================
  Files         760      761       +1     
  Lines       11497    11560      +63     
  Branches     1343     1356      +13     
==========================================
+ Hits         9005     9044      +39     
- Misses       2053     2099      +46     
+ Partials      439      417      -22     
Flag Coverage Δ
win-longtests 78.23% <88.33%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@terriko
Copy link
Contributor Author

terriko commented Oct 26, 2023

This new implementation passes all tests now, which is satisfying but also makes me suspicious that I need more tests because there's definitely cases I know I'm missing.

@terriko terriko marked this pull request as ready for review October 26, 2023 18:49
@terriko
Copy link
Contributor Author

terriko commented Oct 26, 2023

Had to fix kerberos reporting. For some reason we were expecting 10 vulns, but even cvedetails is reporting 9 for this version:
https://www.cvedetails.com/version/1235909/MIT-Kerberos-5-1.15.1.html

@terriko
Copy link
Contributor Author

terriko commented Nov 1, 2023

Updating the branch just for the cve-scan job (which was fixed by specifying a version of python)

cve_bin_tool/version_compare.py Show resolved Hide resolved
cve_bin_tool/version_compare.py Show resolved Hide resolved
cve_bin_tool/version_compare.py Show resolved Hide resolved
cve_bin_tool/version_compare.py Show resolved Hide resolved
cve_bin_tool/version_compare.py Show resolved Hide resolved
test/test_version_compare.py Show resolved Hide resolved
test/test_version_compare.py Show resolved Hide resolved
test/test_version_compare.py Show resolved Hide resolved
terriko and others added 15 commits November 15, 2023 17:30
We had been using packaging's version parsing tools, but as they move more
towards pep440 compliance they aren't as useful for comparing arbitrary
versions that may not follow the same scheme.  This moves us to our own
function.  It may need some further tweaking for special cases such as release
candidates or dev versions.

Signed-off-by: Terri Oda <terri.oda@intel.com>
Signed-off-by: Terri Oda <terri@toybox.ca>
Copy link
Contributor Author

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I've resolved everything except the pre-check function idea. I think I might open a separate issue for that and let someone else work on it, if you don't mind. (Mostly because I don't want to take up more of your time and it's been ahrd to find someone to review this code as is...)

cve_bin_tool/version_compare.py Show resolved Hide resolved
@terriko terriko merged commit 9a95eca into intel:main Nov 16, 2023
20 of 21 checks passed
@terriko
Copy link
Contributor Author

terriko commented Nov 16, 2023

Thanks @antoniogi -- I'm very glad to have had a fresh set of eyes on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants