Analyze run all pluto 1385 #64
Conversation
- Moved the `mergeSarifOutputs` function from `analyze.go` to `utils/sarif.go` for better code organization.
- Updated the `analyze.go` file to use the new utility function for merging SARIF outputs.
- Enhanced the `Rule` struct in the utils package to use `map[string]interface{}` for properties.
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 |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
utils/sarif.go:180
- Consider handling the error returned by json.MarshalIndent in createEmptySarifReport to avoid potential silent failures.
sarifData, _ := json.MarshalIndent(emptyReport, "", " ")
cmd/analyze.go:294
- [nitpick] Changing the unsupported tool handling from a fatal error to a warning may lead to silent failures; please verify that this behavior is intentional or consider handling it by returning an error.
log.Printf("Warning: Unsupported tool: %s\n", toolName)
cmd/analyze.go
Outdated
| func init() { | ||
| analyzeCmd.Flags().StringVarP(&outputFile, "output", "o", "", "Output file for analysis results") | ||
| analyzeCmd.Flags().StringVarP(&toolToAnalyze, "tool", "t", "", "Which tool to run analysis with") | ||
| analyzeCmd.Flags().StringVarP(&toolToAnalyze, "tool", "t", "", "Optional: Specific tool to run analysis with. If not specified, all configured tools will be run") |
There was a problem hiding this comment.
I mean, we don't need to specify that arguments are optional, just like the Output file as an example
cmd/analyze.go
Outdated
| // If a specific tool is specified, only run that tool | ||
| if toolToAnalyze != "" { | ||
| log.Printf("Running %s...\n", toolToAnalyze) | ||
| runTool(workDirectory, toolToAnalyze, args, outputFile) |
There was a problem hiding this comment.
Suggestion, can we do something like this, so we can only have one flow and one code to deal with 'running a list of tools:
tools := config.Config.Tools()
if toolToAnalyze != "" {
tools = toolToAnalyze
}
cmd/analyze.go
Outdated
| log.Fatal("Trying to run unsupported tool: ", toolToAnalyze) | ||
| log.Println("Running all configured tools...") | ||
|
|
||
| if outputFormat == "sarif" && outputFile != "" { |
There was a problem hiding this comment.
I am expecting that running something with --format sarif > out.sarif to be similar to --format sarif --output out.sarif, is is a fair expectation? Meaning, that if we do any strategy of merging when it is a SARIF, it should not depend if the user specifies a file or not
- Renamed `toolToAnalyze` to `toolsToAnalyzeParam` for clarity. - Updated the logic to allow specifying a single tool for analysis or running all configured tools if none is specified. - Improved error handling for cases with no configured tools.
When running like:
cli-v2 analyze --format sarif --output results.sarif then all tools declared in codacy yaml will run and output will be merged into one sarif file