refactor: use RequiredPlugins to decide if transitive enrichers should be enabled#2526
Conversation
c51cd96 to
42ed804
Compare
| if !actions.TransitiveScanning.Disabled { | ||
| // TODO: Use Enricher.RequiredPlugins to check this generically | ||
| if isRequirementsExtractorEnabled(plugins) { | ||
| if areRequiredPluginsEnabled(plugins, transitivedependencyrequirements.Enricher{}) { |
There was a problem hiding this comment.
This is probably relevant for #2537: I'm building the struct manually rather than using enricher.New because the latter actually does some work initializing API clients and such (which in some cases might even lead to an API request?).
I'm not sure if we can avoid that if we move to using a preset with our current structure
There was a problem hiding this comment.
We should be able to use New(), no network requests are made there. (The http clients just create a struct, the deps.dep gRPC clients are lazily initiated, no connection is opened until the first request is made)
There was a problem hiding this comment.
it seems a little odd, but the best I can think of to deal with this without too much change would be to have some kind of IsForRequirementChecks flag in cpb.PluginConfig, with which ideally plugins should do:
// New creates a new Enricher.
func New(cfg *cpb.PluginConfig) (enricher.Enricher, error) {
if cfg.IsForRequirementChecks {
return Enricher{}, nil
}
// ...
}
🤔
There was a problem hiding this comment.
I'm not quite following, why do we need a separate initialisation for IsRequirements check?
There was a problem hiding this comment.
I was thinking we setup all the plugins, then do a loop and remove plugins that don't have their requirements met. Described in #2537
There was a problem hiding this comment.
yeah I posted that a couple of minutes after you posted your comment saying that we were ok with just always calling New.
I've had a look at the code paths and it looks like at most what we end up doing is potentially loading in some TLS PEM store stuff, but most of it does look very lazy (i.e. assigning a setupThisThing function field to be called later), and with no API calls being made.
Personally I'm still not the biggest fan of how much work it appears that we would be doing, but I don't think there's an easy way around that because Go 🤷
There was a problem hiding this comment.
I think this works for now but longer term I prefer to do all the checks in scalibrplugin.Resolve()
42ed804 to
dcc0d6d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2526 +/- ##
==========================================
- Coverage 67.92% 67.89% -0.03%
==========================================
Files 172 172
Lines 13395 13395
==========================================
- Hits 9098 9094 -4
- Misses 3581 3583 +2
- Partials 716 718 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dcc0d6d to
e5f45d0
Compare
e5f45d0 to
9ba99fe
Compare
Relates to #2537