feat: add --experimental-offline-vulnerabilities and --experimental-no-resolve flags
#1342
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1342 +/- ##
==========================================
- Coverage 68.43% 68.43% -0.01%
==========================================
Files 183 183
Lines 17606 17620 +14
==========================================
+ Hits 12049 12058 +9
- Misses 4895 4898 +3
- Partials 662 664 +2 ☔ View full report in Codecov by Sentry. |
cmd/osv-scanner/scan/main.go
Outdated
| if context.Bool("experimental-offline") { | ||
| // Set the corresponding offline flags, unless they were already set explicitly. | ||
| if !context.IsSet("experimental-offline-vulnerabilities") { | ||
| offlineVulns = true | ||
| } |
There was a problem hiding this comment.
The CompareOffline field that experimental-offline-vulnerabilities sets also affects the SkipGit and license scanning in osvscanner.DoScan.
I think it might be a bit clearer to instead set them here similarly, but that could create a subtle change in behaviour in DoScan.
cmd/osv-scanner/scan/main.go
Outdated
| disableTransitive := context.Bool("experimental-no-resolve") | ||
| if context.Bool("experimental-offline") { | ||
| // Set the corresponding offline flags, unless they were already set explicitly. | ||
| if !context.IsSet("experimental-offline-vulnerabilities") { |
There was a problem hiding this comment.
Why do we need to check if they were already set? if someone is setting --experimental-offline, then IMO that should just always disable everything regardless of what other flags are set?
There was a problem hiding this comment.
I guess I was thinking someone has to intentionally pass e.g. --experimental-offline --experimental-offline-vulnerabilities=false, which seems like how you'd want to override the default offline options.
idk if we actually want to allow this, but if not this should probably at least log a warning.
There was a problem hiding this comment.
I think I may understand the use case a bit here: someone wants to disable all network requests but not the ones for vulnerabilities.
Also as Michael mentioned below, CompareOffline also controls others things for example license scanning, maybe we should change it to a struct with a list of booleans? Each boolean field corresponds to one type of request requiring network.
Even further, can we change experimental-offline to be a list of strings taking all or vulnerability,license and all takes precedence over all other.
There was a problem hiding this comment.
NB: I moved this logic to the Action of the offline flag in aa0a734, but I've not addressed this discussion.
There was a problem hiding this comment.
IMO let's keep this simple for now and go with @michaelkedar 's approach.
--experimental-offline should just only be set when somebody wants to turn everything off, and can't be overriden. There's already individual flags to control behaviour (after this PR) for the independent bits.
Re taking a slice of strings instead, we went with that for --call-analysis to take in a list of languages, but I think full list of languages would be much larger than the list of functionality where we can control online/offline. (i.e. we'd have --call-analysis-rust, --call-analysis-go, --call-analysis-python, --call-analysis-{language}, ...)
And since this is all experimental, we can change this later :)
cmd/osv-scanner/scan/main.go
Outdated
| disableTransitive := context.Bool("experimental-no-resolve") | ||
| if context.Bool("experimental-offline") { | ||
| // Set the corresponding offline flags, unless they were already set explicitly. | ||
| if !context.IsSet("experimental-offline-vulnerabilities") { |
There was a problem hiding this comment.
I think I may understand the use case a bit here: someone wants to disable all network requests but not the ones for vulnerabilities.
Also as Michael mentioned below, CompareOffline also controls others things for example license scanning, maybe we should change it to a struct with a list of booleans? Each boolean field corresponds to one type of request requiring network.
Even further, can we change experimental-offline to be a list of strings taking all or vulnerability,license and all takes precedence over all other.
Closes #1339 and closes #1121
Adds flags to use offline mode for vulnerabilities (
--experimental-offline-vulnerabilities) and transitive resolution separately (--experimental-no-resolve)The original
--experimental-offlineflag retains the same behaviour by functionally setting both of these flags.