worker: run dynamic and static analysis unconditionally #921
Conversation
… errors for one type of analysis cause the other to be skipped (including saving of results) Signed-off-by: Max Fisher <[email protected]>
Signed-off-by: Max Fisher <[email protected]>
| // run both dynamic and static analysis regardless of error status of either | ||
| // and return combined error(s) afterwards, if applicable | ||
| staticResults, _, staticAnalysisErr := worker.RunStaticAnalysis(ctx, pkg, staticSandboxOpts, staticanalysis.All) | ||
| if staticAnalysisErr == nil { | ||
| staticAnalysisErr = worker.SaveStaticAnalysisData(ctx, pkg, resultStores, staticResults) |
There was a problem hiding this comment.
Is there a rationale for changing the order of static analysis and dynamic analysis?
There was a problem hiding this comment.
Originally I only put static after dynamic because it was conditionally run, I think it makes sense to have static analysis running first because
- Static analysis requires downloading the package archive and this may end up being able to be reused for dynamic analysis. The saving analyzed package feature can also potentially reuse this download
- For some reason, static analysis data was saved before dynamic analysis before, so now that the saving is paired with the running, it means that the order of saving results is the same.
- static analysis is usually faster than dynamic analysis
All that said, it shouldn't make a huge difference. If you feel strongly that the order should be kept as is, we can do that.
There was a problem hiding this comment.
All good. I appreciate understanding the rationale and it helps to have context in the PR for the future.
There was a problem hiding this comment.
No worries and I agree it's good to have the context here too :)
| // run both dynamic and static analysis regardless of error status of either | ||
| // and return combined error(s) afterwards, if applicable | ||
| staticResults, _, staticAnalysisErr := worker.RunStaticAnalysis(ctx, pkg, staticSandboxOpts, staticanalysis.All) | ||
| if staticAnalysisErr == nil { | ||
| staticAnalysisErr = worker.SaveStaticAnalysisData(ctx, pkg, resultStores, staticResults) |
There was a problem hiding this comment.
All good. I appreciate understanding the rationale and it helps to have context in the PR for the future.
* worker: run dynamic and static analysis unconditionally and don't let errors for one type of analysis cause the other to be skipped (including saving of results) Signed-off-by: Max Fisher <[email protected]> * run static analysis before dynamic analysis to preserve order of saving Signed-off-by: Max Fisher <[email protected]> --------- Signed-off-by: Max Fisher <[email protected]>
Currently in the worker binary, the sequence of analysis actions is:
However, after each step above if any error has occurred, the function returns immediately and the remaining steps are skipped. It makes more sense to run both dynamic analysis and static analysis without returning from any errors until both analysis types have been attempted.
Thus the new sequence of actions is
(Also inlines the
if err := ... ; err != nil { }form for thenotification.PublishAnalysisCompletionfunction)