-
Notifications
You must be signed in to change notification settings - Fork 377
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
feat: support composite-based package overrides #1214
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1214 +/- ##
==========================================
+ Coverage 67.64% 67.78% +0.13%
==========================================
Files 174 174
Lines 16721 16759 +38
==========================================
+ Hits 11311 11360 +49
+ Misses 4778 4770 -8
+ Partials 632 629 -3 ☔ View full report in Codecov by Sentry. |
72de007
to
df9ea40
Compare
(note that the docs should probably be updated, but I want to get confirmation that we're happy with this general change first) |
Note that this should also be usable to resolve #1124 - I think technically you could do that right now by e.g. "consider all packages in the |
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.
LGTM!
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.
LGTM! good idea making it more general. I'm not sure about using the models.PackageVulns type here, but that's probably something to consider in the v2 rework of the types, (we have too many types that roughly represent the same thing currently), so happy to merge as is.
pkg/osvscanner/osvscanner.go
Outdated
@@ -967,7 +967,17 @@ func filterIgnoredPackages(r reporter.Reporter, packages []scannedPackage, confi | |||
out := make([]scannedPackage, 0, len(packages)) | |||
for _, p := range packages { | |||
configToUse := configManager.Get(r, p.Source.Path) | |||
if ignore, ignoreLine := configToUse.ShouldIgnorePackageVersion(p.Name, p.Version, string(p.Ecosystem)); ignore { | |||
if ignore, ignoreLine := configToUse.ShouldIgnorePackage( |
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.
nit: Separate this out from the if statement as it is quite long, and just have it on it's own line.
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.
done - I went with keeping the function in the if and using a variable for the struct as I think that's a bit more correct: in the long-run, we expect to be passing a whole "thing" to this function (like what we're already doing in vulnerability_result.go
), so it's the models.PackageVuln
part that's a temp workaround
dc31d47
to
7979aa2
Compare
```toml | ||
# ignore everything | ||
[[PackageOverrides]] | ||
ignore = true |
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 the use case for this just scanning for licenses? In that case can we specify that in the comment above.
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'm not quite sure what you mean - I've only changed the logic around when an override should be applied to a particular package, not the supported "actions"; so the behaviour should be whatever ignore
did previously...?
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.
fwiw, my expectation is that this enables #1155
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 meant why someone would want to ignore everything. This was before I remembered that it only applies to the file in the same directory, so this will be pretty useful. Can you change the comment above this to specify that this is ignoring everything in the same directory?
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.
(we should probably also change ignore to ignoreVulns, and deprecate ignore, as it is ambiguous as to whether we are still doing license scanning (we are) (turns out it does not), we can do that in a separate PR though. I'll make an issue for this.)
I think we're on exactly the same page - overall, my view here is naming wise this suffers because of already known issues which are a major epic to be addressed in v2 and that the more important thing is this change introduces a showcase of how the configuration could be improved that is very useful to have landed before v2 so it can be part of shaping v2 itself. A good example of that imo is with files: it could be very reasonable to add the ability to ignore specific files but that's hard because |
98a1141
to
0791430
Compare
0791430
to
b8f6209
Compare
This rewrites the package overrides logic to be composition based, granting a lot more flexibility:
While some of these might seem a bit extreme, ultimately I think this is probably the way to go as the logic itself is very straightforward and it gives a lot more power to the people.
Since
config
is a public package, I've had to deprecated the related existing public methods and there's a bit of naming & structural yuck but I figure that's not a big deal since v2 is right around the corner and again the logic itself is very straightforward.Resolves #1211
Resolves #1155