-
Notifications
You must be signed in to change notification settings - Fork 84
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
all: account for language package overwrites #1275
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1275 +/- ##
==========================================
+ Coverage 55.41% 55.73% +0.32%
==========================================
Files 282 278 -4
Lines 17890 17806 -84
==========================================
+ Hits 9914 9925 +11
+ Misses 6934 6839 -95
Partials 1042 1042 ☔ View full report in Codecov by Sentry. |
fda8981
to
a0430f0
Compare
a0430f0
to
6af0edd
Compare
3637e69
to
d10aeed
Compare
Though this does not explicitly touch package scanners, this still may merit a reindex. Thoughts? |
gobin/coalescer.go
Outdated
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 seems to be copy-pasted; why?
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 rest were definite copy/pastes of each other. This one is slightly unique, so I copied over the related change, but kept the fact this is still mildly different from the rest. Do you think this coalescer should just match the rest?
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 updated it to also move this into the language-agnostic coalescer. The parts of this which were unique were not very necessary (I think)
language/coalescer.go
Outdated
// For langauge packages, it is possible the | ||
// packageDB is overwritten. |
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 this package needs to document this assumption, and we need to check that the uses actually work that way.
- gobin
- java
- nodejs
- python
- ruby
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.
just curious why gobin
is already checked
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 read through it to see what it did.
Yeah, needs the version changed in every indexer that's moving to it. |
d10aeed
to
6ed00e5
Compare
6ed00e5
to
1e193fa
Compare
1e193fa
to
221025b
Compare
221025b
to
622034b
Compare
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 like it's in good shape, a few comments.
language/coalescer.go
Outdated
@@ -0,0 +1,70 @@ | |||
// Package language implements structs and functions common between | |||
// programming language indexing implementation. |
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.
implementations?
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.
is this questioning the meaning or telling me to make this plural? If the latter, done. Otherwise, let me know how I can improve this
language/coalescer_test.go
Outdated
} | ||
} | ||
|
||
func TestCoalescer_package_overwrite(t *testing.T) { |
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.
It's kind of an annoying thing to test, I'm thinking maybe it could be table driven so we can add cases if needed but might take some thinking.
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 does test a very specific scenario, and I'm not sure if I can think of other similar tests which would be useful to put here. Is it ok to just leave as-is?
return &coalescer{}, nil | ||
} | ||
|
||
// Coalesce implements [indexer.Coalescer]. |
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 logic here is actually not trivial w/r/t which packages end up in the final index report, I think this docstring could do with more explanation.
Signed-off-by: RTann <[email protected]>
622034b
to
d2a2bd5
Compare
This was originally discovered in StackRox Scanner V2: stackrox/stackrox#7033
StackRox now offers a Scanner based on ClairCore, which also has this same problem. The issue is that ClairCore does not consider the fact that the image build system may decide to overwrite the language package instead of deleting and recreating it.
This was demonstrated in the OCI image
namloc2001/nodesem:a
.Each language's package scanner implements
DefaultRepoScanner
, which means there is only a single (related) repository per layer, and it only exists if the layer has (related) packages. Each language's coalescer is also more-or-less the same (Go's was unique, but the unique parts of it are unnecessary, as those checks will definitely hold true based on how to Ecosystem is set up), so I decided to make a single, shared coalescer for the languages.