-
Notifications
You must be signed in to change notification settings - Fork 479
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: make EPSS behave like other data sources #4125
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4125 +/- ##
==========================================
+ Coverage 75.41% 80.79% +5.38%
==========================================
Files 808 820 +12
Lines 11983 12544 +561
Branches 1598 1702 +104
==========================================
+ Hits 9037 10135 +1098
+ Misses 2593 1973 -620
- Partials 353 436 +83
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@anthonyharrison and @Rexbeast2 do you have time to review this one? I can grab a colleague to review but I'd like someone who knows the EPSS code better if possible. I'm working on fixing the "build wheel: test in another PR so you can ignore that failure. (It shouldn't be running on pull requests and needs to be 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.
Don't we still need a call to self.populate_epss() in the loop which stores the EPSS data?
It should be replaced by the default call to epss_source.get_cve_data() (which does the same thing: calls up update_epss(), only without the apparently extraneous database open because we weren't actually storing anything in the db in update_epss() even before?). We're populating cve_data instead of epss_data (which I should have removed) but then we shove it into the storage the same way. |
Note to self: rebased against my branch with the test/ci fixes so hopefully all tests will pass this time. |
I'm going to rebase this now that the CI fixes are in. I think I'd like to add a test to make sure disabling works, but I need to figure out how to do that without having to run basically It would also be nice to have some code to test the epss loading code I changed in cvedb.py, but again, I'm going to have to figure out how to hook into existing tests so that I'm not re-loading the whole database for a single test. Not sure if these tests will make it into this PR, but I'll do a bit of reconnaissance to see how feasible they are and report back before asking for new reviews. |
Rebase is done, will likely try for some tests tomorrow. |
@terriko, I'm not sure if it fits this PR (let me know if not, I can submit a separate issue or probably a PR straight away due to triviality), but while you're at it, would it be possible to add the |
@alext-w Separate issue/pr please! |
Adding a disabled source test for intel#4125, and also made it possible to disable the nvd_api_key to make it easier to avoid nvd2 api calls Signed-off-by: Terri Oda <[email protected]>
Adding a disabled source test for #4125, and also made it possible to disable the nvd_api_key to make it easier to avoid nvd2 api calls Signed-off-by: Terri Oda <[email protected]>
Okay, assuming that the long tests pass, I believe I've resolved the merge issues and other things that needed improvement here and this should be ready for re-review @anthonyharrison |
This will make it so that
-d EPSS
will actually disable the EPSS data source, and should make it fail more gracefully when the source is not working for any reason.Note that the EPSS source may not be working correctly even when not disabled; I'll file a separate issue.