-
Notifications
You must be signed in to change notification settings - Fork 12
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 resolution state for packages with modules #937
Conversation
@@ -6,7 +6,7 @@ func init() { | |||
RegisterMigration(migrate.Migration{ | |||
ID: 19, | |||
Up: migrate.Queries([]string{ | |||
`ALTER TABLE vuln_v2 ADD COLUMN IF NOT EXISTS package_resolution_state TEXT NOT NULL DEFAULT ''`, | |||
`ALTER TABLE vuln_v2 ADD COLUMN IF NOT EXISTS resolution_state TEXT`, |
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.
no need to make this NOT NULL
. Also, change the name to something more concise
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.
One of the tests failed when I was trying to remove "NOT NULL". I am guessing it's because not all package has an resolution state. But it will be great if we can remove that constraint.
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.
Yeah, there are 1777588
rows in vuln_v2
with empty resolution_state
:D . I don't think we need to worry about NOT NULL
since Go would turn it into an empty string upon insert and read
@@ -174,6 +174,7 @@ func md5Vuln(v *database.RHELv2Vulnerability, pkgInfoIdx, pkgIdx, cpeIdx int) [] | |||
b.WriteString(pkg.Module) | |||
b.WriteString(pkg.Arch) | |||
b.WriteString(cpe) | |||
b.WriteString(pkg.ResolutionState) |
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 should be part of the md5 hash
Images are ready for the commit at 912aeaf. To use the images, use the tag |
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.
Looks good to me. As long as the tests pass, it should be good to go.
We were originally unsure what something like
maven:3.6/maven
meant when looking at a package's resolution state, so we ignored anything before the/
. It turns out, this indicates the package (maven
) plus its module (maven:3.6
), so this PR ensures we correctly attribute the resolution state to the correct package/module combo.This PR also includes various related cleaning and a fix for the md5 hash when inserting a vulnerability into the DB