-
Notifications
You must be signed in to change notification settings - Fork 13
Add --format json support for verifycommit and audit commands #318
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This change adds machine-readable JSON output to the verifycommit and audit commands, making it easier to extract data programmatically (e.g., with jq) instead of parsing text output with bash. Changes: - Add common OutputFormat framework (internal/cmd/output.go) - Add --format flag to verifycommit command with JSON support - Add --format flag to audit command with JSON support - Include comprehensive test coverage for both commands The JSON output includes: - verifycommit: success status, commit info, verified SLSA levels - audit: commit results with summary statistics All tests pass and the implementation maintains backward compatibility with existing text output (default format). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
15ebfca to
1c7f7ad
Compare
TomHennen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change I see the value in it. I have a couple of suggestions that might make this easier to maintain long term.
internal/cmd/audit.go
Outdated
| } | ||
|
|
||
| fmt.Printf("Auditing branch %s starting from revision %s\n", auditArgs.branch, latestCommit) | ||
| // For JSON output, collect all results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely comfortable with this arrangement. If I'm reading it properly we've duplicated the audit logic with the textual output, that could lead them to diverge in the future.
Suggestion:
Instead of duplicating the logic, merge them by adding a 'processResult' function that either
- calls printResult
- calls convertAuditResultToJson & appends it to jsonResult.CommitResults.
Then, at the end of the process you can auditArgs.writeJSON(jsonResult) or similar as needed.
Some of the enhancements you made (like the audit summary) might be interesting for the text based output too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 880805e.
internal/cmd/audit_test.go
Outdated
| } | ||
|
|
||
| got := buf.String() | ||
| if got != tt.want { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently comparing JSON output as strings, which isn't always stable.
It might be preferable to use something like DeepEqual which would ignore field order and formatting quirks.
Gemini provided this example
package main
import (
"encoding/json"
"reflect"
"testing"
)
func TestJSONEquality(t *testing.T) {
json1 := []byte(`{"name": "Alice", "age": 30, "hobbies": ["reading", "coding"]}`)
json2 := []byte(`{"age": 30, "name": "Alice", "hobbies": ["coding", "reading"]}`) // Different order
var data1, data2 interface{}
err := json.Unmarshal(json1, &data1)
if err != nil {
t.Fatalf("failed to unmarshal json1: %v", err)
}
err = json.Unmarshal(json2, &data2)
if err != nil {
t.Fatalf("failed to unmarshal json2: %v", err)
}
if !reflect.DeepEqual(data1, data2) {
t.Errorf("JSONs are not semantically equal.\nExpected: %v\nGot: %v", data1, data2)
}
}
Stack Overflow has this one https://stackoverflow.com/a/32409106
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in b2217e6
internal/cmd/verifycommit.go
Outdated
| fmt.Printf("SUCCESS: commit %s on %s verified with %v\n", opts.commit, opts.branch, vsaPred.GetVerifiedLevels()) | ||
| result.VerifiedLevels = vsaPred.GetVerifiedLevels() | ||
|
|
||
| if opts.isJSON() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine, but it also seems odd that opts both know if the output mode is JSON and then the user has to call writeText or writeJSON.
Could it work to have a generic writeResult method that, if the mode was 'JSON' just did what you're looking for, and if not not had specialized output based based on the type of the result (e.g. VerifyCommitResult)?
Perhaps that's too cute? Maybe @puerco has thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just remembered, instead of doing all the specializing within opts it would be better to define a custom method on structs like VerifyCommitResult. If you name the function String it will work automatically with fmt. https://stackoverflow.com/a/13247979.
You can do a similar thing with MarshalJSON that will let you call functions as needed instead of having to define your own ToJSON conversion methods that wind up within the core logic of the code. This might save you some of the effort of defining those duplicate structs and provide a very easy way in opts to write the output as either JSON or Text as needed.
Here's the gemini example
package main
import (
"encoding/json"
"fmt"
"log"
)
// User represents a user with first and last names.
type User struct {
FirstName string
LastName string
}
// MarshalJSON provides a custom implementation for the json.Marshaler interface.
func (u User) MarshalJSON() ([]byte, error) {
// Create a temporary struct to hold the data we want to marshal.
// This avoids infinite recursion by not calling MarshalJSON on the same type.
type alias User // Create a local alias to the type
return json.Marshal(&struct {
FullName string `json:"full_name"`
*alias
}{
FullName: fmt.Sprintf("%s %s", u.FirstName, u.LastName),
alias: (*alias)(&u),
})
}
func main() {
user := User{
FirstName: "Jane",
LastName: "Doe",
}
jsonData, err := json.Marshal(user)
if err != nil {
log.Fatalf("Error marshaling JSON: %v", err)
}
fmt.Printf("Custom JSON: %s\n", string(jsonData))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 019882f.
- Add status constants (statusPassed, statusFailed) to avoid string repetition - Rename writeText to writeTextf following Go printf naming conventions - Fix errcheck by adding nolint comment for intentional error ignore - Auto-fix gci import grouping issues All linting issues now resolved. Tests pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This addresses the code duplication issue raised in PR review. Instead of having separate loops for JSON and text output that could diverge over time, we now use a single loop that handles both cases. Changes: - Initialize JSON result structure before the loop (if needed) - Single loop processes results for both formats - Within loop, conditionally call JSON conversion or text printing - Early termination conditions work for both formats - Summary statistics (passed/failed counts) now tracked for both formats This makes the code more maintainable and ensures JSON and text outputs stay in sync. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Replace string-based JSON comparison with semantic comparison using reflect.DeepEqual. This makes tests more robust against: - Field ordering changes in JSON output - Whitespace/formatting differences - Future changes to JSON encoder settings Changes: - Add shared assertJSONEqual helper function - Convert test expectations from JSON strings to Go structs - Tests now compare semantic meaning rather than exact string format This addresses the code review feedback about brittle string comparisons. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This addresses PR review feedback to use Go idioms for output formatting:
1. Added String() method to VerifyCommitResult
- Implements fmt.Stringer interface for text output
- Eliminates need for manual string formatting at call sites
2. Created writeResult() method in outputOptions
- Automatically selects JSON or text output based on format
- Uses String() method for text when available
- Simplifies call sites from 4-6 lines to 1 line
Benefits:
- More idiomatic Go code using standard interfaces
- Eliminates repetitive if/else checks for output format
- Cleaner, more maintainable code
- Easier to add new output types in future
Example simplification in verifycommit.go:
Before: if opts.isJSON() { ... } else { opts.writeTextf(...) }
After: return opts.writeResult(result)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments about following the command line architecture we are trying to keep across subcommands:
- I would prefer the format option to be a simple string instead of fumbling to/from integers and strings.
- Switching the writer to be returned from a method instead of keeping it as a property in the options set
- Follow the pattern of other subcommands/option sets by adding
AddFlags()andValidate()methods to outputOptions
(more details in the comments below 👇 )
internal/cmd/audit.go
Outdated
| ao.format = OutputFormatText | ||
| cmd.PersistentFlags().Var(&ao.format, "format", "Output format: 'text' (default) or 'json'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, since we are introducing a new output options set , move the flag initialization to an AddFlags() method and call the other embedded lines above:
source-tool/internal/cmd/audit.go
Lines 73 to 74 in af7518e
| ao.branchOptions.AddFlags(cmd) | |
| ao.verifierOptions.AddFlags(cmd) |
Also, simplify this call to a single one using StringVar() (see comment on the output options about turning this into a simple string):
| ao.format = OutputFormatText | |
| cmd.PersistentFlags().Var(&ao.format, "format", "Output format: 'text' (default) or 'json'") | |
| ao.format = OutputFormatText | |
| cmd.PersistentFlags().Var(&ao.format, "format", "Output format: 'text' (default) or 'json'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal/cmd/output.go
Outdated
| "os" | ||
| ) | ||
|
|
||
| type OutputFormat int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious, why do we need to define a custom type for the flag value? Most of the logic in this file seems to handle converting to/from int <> string. Can't we just use a string `['json' | 'text' ] ? The we can drop most of the methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped in 503729f.
internal/cmd/output.go
Outdated
| } | ||
| } | ||
|
|
||
| func (oo *outputOptions) isJSON() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this options set gets embedded in other options, this method should probably have a more descriptive name, for example:
| func (oo *outputOptions) isJSON() bool { | |
| func (oo *outputOptions) outputFormatIsJSON() bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed in 403b1fa.
internal/cmd/output.go
Outdated
| writer io.Writer | ||
| } | ||
|
|
||
| func (oo *outputOptions) init() { | ||
| if oo.writer == nil { | ||
| oo.writer = os.Stdout | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the options set not to store the writer, since the usage pattern is always open -> write -> close, then we should probably switch the init() function to a getWriter() that returns os.Stdout by default. We can expand that to support outputting to a file later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense. Changed in c5c5c5b.
internal/cmd/output.go
Outdated
| } | ||
|
|
||
| // outputOptions provides common output formatting options | ||
| type outputOptions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this set will be embedded in other options set, add a Validate() method and call it from the options set embedding it, for example in audit:
source-tool/internal/cmd/audit.go
Lines 64 to 69 in af7518e
| func (ao *auditOpts) Validate() error { | |
| errs := []error{ | |
| ao.branchOptions.Validate(), | |
| ao.verifierOptions.Validate(), | |
| } | |
| return errors.Join(errs...) |
We should reject from the get go a value that is not json or text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change OutputFormat from an int-based custom type with String(), Set(), and Type() methods to simple string constants. This eliminates unnecessary type conversion logic between int and string. - Remove OutputFormat custom type and its methods - Use string constants: OutputFormatText = "text", OutputFormatJSON = "json" - Update format field in outputOptions from OutputFormat to string - Change flag registration from Var() to StringVar() in audit.go and verifycommit.go - Update tests to use string type instead of OutputFormat type - Remove obsolete tests for OutputFormat.String() and OutputFormat.Set() This addresses code review feedback requesting a simpler string-based approach instead of the int-based enum pattern. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Ralph Bean <[email protected]>
Replace the stored io.Writer property with a getWriter() method that returns os.Stdout. This follows the "open -> write -> close" usage pattern and prepares for future expansion to support file output. - Remove writer field from outputOptions struct - Add getWriter() method that returns os.Stdout - Remove init() method that was only used to initialize writer - Update all methods to call getWriter() instead of using oo.writer - Update test to work without direct writer injection This addresses code review feedback requesting that the writer not be stored as a property, with the option to expand getWriter() later to support file output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Ralph Bean <[email protected]>
Rename the isJSON() method to outputFormatIsJSON() to make it more descriptive when called on embedded option sets. This improves code readability when the method is accessed as `opts.outputFormatIsJSON()` instead of the more ambiguous `opts.isJSON()`. - Rename isJSON() to outputFormatIsJSON() in outputOptions - Update all call sites in audit.go to use new method name - Update test name and assertions to match new method name This addresses code review feedback requesting a more descriptive method name for clarity when outputOptions is embedded in other structs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Ralph Bean <[email protected]>
Add architecture methods to outputOptions to follow the established pattern used by other option sets in the codebase (branchOptions, verifierOptions, etc.). - Add AddFlags() method to initialize format and register the --format flag - Add Validate() method to verify format is either 'text' or 'json' - Add test coverage for Validate() method with valid and invalid inputs - Import cobra package for AddFlags() implementation This prepares outputOptions to be properly integrated into embedding option sets following the command architecture pattern. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Ralph Bean <[email protected]>
Update auditOpts and verifyCommitOptions to properly use the new outputOptions architecture methods instead of directly managing the format flag. - Call outputOptions.AddFlags() in both auditOpts and verifyCommitOptions - Call outputOptions.Validate() in both Validate() methods - Remove direct format flag registration from both files - Remove manual format initialization (now handled by AddFlags()) This completes the refactoring to follow the established command architecture pattern where option sets manage their own flags and validation through AddFlags() and Validate() methods. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Ralph Bean <[email protected]>
Summary
This PR adds machine-readable JSON output to the
verifycommitandauditcommands, making it significantly easier to extract data programmatically (e.g., withjq) instead of parsing text output with complex bash scripts.This addresses the parsing challenges seen in konflux-ci/build-definitions#2867, where bash parsing was needed to extract SLSA levels from VSA output.
Changes
internal/cmd/output.go): CommonOutputFormatenum and utilities for consistent JSON/text outputverifycommitcommand: Added--formatflag with JSON output supportauditcommand: Added--formatflag with JSON output supportJSON Output Structure
verifycommit
{ "success": true, "commit": "abc123", "ref": "main", "ref_type": "branch", "owner": "org", "repository": "repo", "verified_levels": ["SLSA_SOURCE_LEVEL_3"] }audit
{ "owner": "org", "repository": "repo", "branch": "main", "latest_commit": "abc123", "commit_results": [ { "commit": "abc123", "status": "passed", "verified_levels": ["SLSA_SOURCE_LEVEL_3"], "link": "https://github.com/org/repo/commit/abc123" } ], "summary": { "total_commits": 1, "passed_commits": 1, "failed_commits": 0 } }Example Usage
Test plan
--formatflag🤖 Generated with Claude Code