-
Notifications
You must be signed in to change notification settings - Fork 588
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
chore: go-rpmdb update #1757
chore: go-rpmdb update #1757
Conversation
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Benchmark Test ResultsBenchmark results from the latest changes vs base branch
|
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Thanks @kzantow! - I think this one is important enough too that we get @wagoodman also to give it some 👀 if there is something obvious I missed or if there is another dependency he might have been thinking about going with |
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.
Actually I need to check one more thing but don't have a laptop handy
Probably worth checking that grype works as expected when updated with this version of syft also just due to the SQL driver registration logic. I remember there was something weird happening there and that's why we use our own sqlite fork in grype (to get around the registration in init logic) |
@@ -3,6 +3,8 @@ package integration | |||
import ( | |||
"testing" | |||
|
|||
_ "modernc.org/sqlite" |
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 registers the sqlite driver only in test code, which wont get compiled into the final syft binary (which means the test will pass but syft will be unable to open sqlite RPMDBs since there will not be a registered driver)
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.
I pushed code that keeps this import, but I've moved it to the main package, this way library users can choose their own sqlite driver (we shouldn't be registering drivers that have side effects on the stdlib within our lib)
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.
I pushed another commit that keeps the import in main and this integration test (since this package does not use main it wouldn't have a sqlite driver).
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
This will be an apparent breaking change for library users since syft used to wire up the sqlite driver into the stdlib via the go-rpmdb lib, however, this is no longer the case with this update, so lib users will need to register their own driver. This is a silent failure since not importing the driver will not result in a panic or error returned. I've added a log line to explicitly check if the sqlite driver is registered and log a warning if it is not. |
Signed-off-by: Alex Goodman <alex.goodman@anchore.com>
* main: chore(deps): bump modernc.org/sqlite from 1.21.2 to 1.22.0 (#1758) chore: go-rpmdb update (#1757) chore(deps): bump github.com/CycloneDX/cyclonedx-go from 0.7.1-0.20221222100750-41a1ac565cce to 0.7.1 (#1706) fix: Improve pnpm support (#1752) feat: Add template func `hasField` (#1754) fix: only cache java packages and not source content (#1750) Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com>
Signed-off-by: Christopher Phillips <christopher.phillips@anchore.com> Signed-off-by: Alex Goodman <alex.goodman@anchore.com> Co-authored-by: Alex Goodman <alex.goodman@anchore.com>
Summary
Notes
main.go
implementation for the rpmdb functionality to still work correctlyalmalinux
and found the correct packages for the RPMDB cataloger, but wanted to double check with the team before making this change