[PLUTO-1396] Handle tool errors gracefully #75
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
cmd/analyze.go
Outdated
| } | ||
|
|
||
| for toolName, err := range failedTools { | ||
| utils.AddErrorRun(&mergedSarif, toolName, err.Error()) |
There was a problem hiding this comment.
In my head, each tool has the responsability of returning its error and output to its SARIF if that is the format. Meaning, that on this method, I would not expect to have to mess with their SARIF outputs, does it make sense? 🤔
So the error that the tool propagates, is the error that we show on the STDOUT, but the SARIF would not necessarily be an extra one with that error.
I guess the question/concern is, if ESLint still creates a SARIF with an error, it would be nice to use that SARIF. Do you know if ESLint or other tools create a SARIF with information in case of failures?
There was a problem hiding this comment.
The eslint does indeed capture some errors and outputs it in sarif, and current implementation lets eslint handle its own errors and report them in SARIF format but if it was unable to run, we would handle the catastrophic failures where the tool couldnt't run at all.
Definitely open to refining this logic to maybe separate "tool-reported" vs "wrapper-reported" errors, or do the intention was not to even capture those? 🤔
There was a problem hiding this comment.
Yeah let's sync tomorrow, for me, especially for tools that already handle some things well on their own SARIF, would avoid creating generic error SARIF. Would aim to share the errors on the STDOUT for those 'fatal problems' where the tool didn't even ran.
cmd/analyze.go
Outdated
| runTool(workDirectory, toolName, args, tmpFile) | ||
| if err := runTool(workDirectory, toolName, args, tmpFile); err != nil { | ||
| log.Printf("Tool failed to run: %s: %v\n", toolName, err) | ||
| continue |
There was a problem hiding this comment.
I would see us not having these continues. I thing that the 'append' of sarifs, should be ready to receive a file that does not exist or something.
But would like not to miss any possible generated sarif from the tool
There was a problem hiding this comment.
You're right, will remove 👍
cmd/analyze.go
Outdated
| runTool(workDirectory, toolName, args, outputFile) | ||
| if err := runTool(workDirectory, toolName, args, outputFile); err != nil { | ||
| log.Printf("Tool failed to run: %s: %v\n", toolName, err) | ||
| continue |
There was a problem hiding this comment.
I guess this one does not do anything and ca be removed right? or?
| if err := runTool(workDirectory, toolName, args, tmpFile); err != nil { | ||
| log.Printf("Tool failed to run: %s: %v\n", toolName, err) | ||
| } | ||
| sarifOutputs = append(sarifOutputs, tmpFile) |
There was a problem hiding this comment.
Assuming that this line works even if tmpFile foes not exist, LGTM!
There was a problem hiding this comment.
Yes, in the mergesarif function non existent files should be handled 👍
data, err := os.ReadFile(file)
if err != nil {
if os.IsNotExist(err) {
// Skip if file doesn't exist (tool might have failed)
continue
}
return fmt.Errorf("failed to read SARIF file %s: %w", file, err)
}```
This pull request includes several changes to improve error handling in the analysis tools and ensure that errors are properly propagated and logged. The most important changes include modifying the analysis functions to return errors, updating the
runToolfunction to handle these errors, and adding error handling in the tool runners.Error handling improvements:
cmd/analyze.go: ModifiedrunEslintAnalysis,runTrivyAnalysis,runPmdAnalysis, andrunPylintAnalysisfunctions to return errors instead of logging fatal errors directly. UpdatedrunToolfunction to handle these errors and log appropriate messages. [1] [2] [3]Tool runner updates:
tools/eslintRunner.go: ModifiedRunEslintfunction to return an error if the command fails, except when ESLint returns 1 due to finding issues. [1] [2]tools/pmdRunner.go: UpdatedRunPmdfunction to return a formatted error message if the command fails.tools/pylintRunner.go: Adjusted error handling inRunPylintfunction to return a formatted error message.tools/trivyRunner.go: ModifiedRunTrivyfunction to return a formatted error message if the command fails.