Add --format argument to application commands#76
Conversation
e253607 to
f21dcf4
Compare
shayshim
left a comment
There was a problem hiding this comment.
apptrust/common/output_test.go line 16 — TestPrintJsonOrTableResponse_JSON doesn't assert on any output
This test passes but asserts nothing about the JSON output. The Json branch writes via log.Output, not the buf writer — so buf is always empty and never read. The only assertions are NoError (always passes) and that the hardcoded sampleOutputJSON constant is valid JSON (trivially true).
Consider capturing log.Output to assert the JSON was actually printed correctly.
shayshim
left a comment
There was a problem hiding this comment.
Duplicate ordered*Keys slices
Several ordered*Keys slices are identical and could be shared within their package:
orderedCreateAppKeysandorderedUpdateAppKeysare byte-for-byte the sameorderedCreateAppVersionKeys,orderedUpdateAppVersionKeys, andorderedUpdateAppVersionSourcesKeysare all identical
Since they live in the same package, a single shared variable would avoid silent drift if the response shape changes and only some copies get updated.
shayshim
left a comment
There was a problem hiding this comment.
apptrust/common/output.go line 44 — fmt.Sprintf("%v", val) renders arrays and objects as Go syntax
val comes from json.Unmarshal into map[string]interface{}, so its runtime type depends on the JSON value:
- Strings, numbers, booleans render correctly with
%v - Arrays render as
[elem1 elem2]instead of["elem1","elem2"] - Objects render as
map[key:value]instead of{"key":"value"}
The current orderedKeys only list scalar fields so this doesn't bite today, but the API response is untyped []byte — there's no guard against a future key pointing to a nested value. Safer alternative:
b, err := json.Marshal(val)
if err != nil {
return err
}
strVal := string(b)
shayshim
left a comment
There was a problem hiding this comment.
go.mod — Direct JFrog dependencies pinned to unreleased commits
Three direct JFrog dependencies are pinned to pseudo-versions (unreleased commits):
build-info-go v1.13.1-0.20260429070557-93b98034d295jfrog-cli-core/v2 v2.60.1-0.20260504054219-ba16d20c7b0fjfrog-client-go v1.55.1-0.20251223101502-1a13a993b0c7
These commits could be force-pushed or may depend on unreleased APIs. Please pin to tagged releases before merging to main.
|
Regarding
Those are not released anymore, instead for the CLI uses directly the master commit, only the CLI is released. |
|
@shayshim I took care of your remarks, let me know if anything else needs to be done. |
|
Make sure to run e2e test - https://github.com/jfrog/jfrog-cli-application/actions/workflows/e2e-tests.yml |
|
Run go fmt |
shayshim
left a comment
There was a problem hiding this comment.
Bug: DefaultFormat: coreformat.Json missing on 8 commands
DefaultFormat: coreformat.Json is set only on version-create. The other 8 commands (app-create, app-update, version-update, version-promote, version-release, version-rollback, version-update-sources, package-bind) are missing it.
With the new PrintResponse, the default case (no --format flag) produces no output — only the log.Info success message. Without DefaultFormat: coreformat.Json, users running these commands without --format will silently get no data back, which is a regression from the previous behavior of always printing JSON.
The comment claiming this "preserves pre-flag behavior" is also incorrect — the old behavior did print raw JSON via log.Output.
shayshim
left a comment
There was a problem hiding this comment.
E2E: Missing output format coverage
The existing version-create async e2e test captures CLI output and parses it as JSON, which is what drove DefaultFormat: coreformat.Json being added there. No equivalent test exists for the other 8 commands, which is why the missing DefaultFormat went unnoticed.
Consider adding e2e coverage for each command that verifies:
- Default behavior (no
--format) produces valid JSON output --format=jsonproduces valid JSON output--format=tableproduces a table with the expected fields
This would catch regressions in output format behavior across the board.
Summary
Adds a
--formatflag to all AppTrust mutating commands so users can choose betweenhuman-readable table output and JSON output suitable for scripting. When the flag is
omitted, no response body is printed — commands continue to emit their existing
log.Info "...successfully"confirmation, preserving pre-flag behavior. Supportedformats are advertised via
SupportedFormatsso the CLI framework auto-completesthe flag.
Commands updated
The following commands now accept
--format=table|json:app-create,app-updateversion-create,version-promote,version-release,version-update,version-update-sources,version-rollbackpackage-bindImplementation notes
apptrust/common/output.go:PrintResponse(data, format, writer, orderedKeys)— dispatches to JSON pretty-printor table rendering based on the requested format, writing to the caller-supplied
io.Writer. None (no--format) is a no-op.PrintTable(data, writer, orderedKeys)— renders the response body as aFIELD/VALUEtable usingtext/tabwriter, honoringorderedKeysto control roworder and skipping empty/nil fields. Non-scalar values (arrays, objects) are
JSON-marshalled so they render as proper JSON rather than Go syntax. Returns early
when the response body is empty.
applications,versions, andpackagesservices now return
[]byteresponse bodies (previously discarded) so commands canformat and print them. Mocks regenerated accordingly.
apptrust/common/keys.go:OrderedAppVersionKeys(used byversion-create,version-update,version-update-sources) andOrderedAppKeys(used by
app-create,app-update).version-promote,version-release,version-rollback,package-bind— each defines its ownordered<Cmd>Keysatthe top of the file.
mainbranch.go vet ./....go fmt ./....