feat(cmd): add JSON output support to init, remove, and list commands#62
Merged
feloy merged 6 commits intokortex-hub:mainfrom Mar 12, 2026
Merged
feat(cmd): add JSON output support to init, remove, and list commands#62feloy merged 6 commits intokortex-hub:mainfrom
feloy merged 6 commits intokortex-hub:mainfrom
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAdds JSON output support to init, workspace list, and workspace remove commands; introduces conversion and JSON error helpers; binds output flags directly to command structs; expands tests for JSON paths; updates documentation with flag-binding best practices, JSON output patterns, and README scenarios; bumps an API dependency version. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cobra cmd)
participant Manager as Manager
participant Storage as Storage
participant Output as JSON/Text Output
CLI->>Manager: create/list/remove workspace (respect --output)
Manager->>Storage: perform storage operations (create/list/delete)
Storage-->>Manager: result / error
Manager-->>CLI: instance / id or error
alt output == "json"
CLI->>Output: format with instanceToWorkspace / WorkspaceId
Output-->>CLI: write JSON (or write api.Error via formatErrorJSON)
else
CLI->>Output: write human-readable text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/conversion.go (1)
68-73: Consider logging if JSON error formatting fails.The error from
formatErrorJSONis silently ignored on line 70. Whileapi.Erroris a simple struct that should always marshal successfully, silently discarding this error could mask unexpected issues.💡 Optional: Log if formatting fails
func outputErrorIfJSON(cmd interface{ OutOrStdout() io.Writer }, output string, err error) error { if output == "json" { - jsonErr, _ := formatErrorJSON(err) + jsonErr, fmtErr := formatErrorJSON(err) + if fmtErr != nil { + // Fallback to simple error string if JSON formatting fails + fmt.Fprintf(cmd.OutOrStdout(), `{"error": "failed to format error as JSON"}`+"\n") + return err + } fmt.Fprintln(cmd.OutOrStdout(), jsonErr) } return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/conversion.go` around lines 68 - 73, In outputErrorIfJSON, stop ignoring the error returned by formatErrorJSON: capture its second return value and if non-nil log or print that formatting error (alongside the original err) to cmd.OutOrStdout() so formatting failures are visible; update the call to formatErrorJSON in outputErrorIfJSON and use cmd.OutOrStdout() (or fmt.Fprintf/Fprintln) to emit a clear message like "error formatting JSON: <formatErr>" when formatErrorJSON returns an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 192-200: The numbered list under the "Best Practices for
Programmatic Usage" header has missing numbers (skips 3 and 5); update the
markdown so the list is sequential (e.g., 1–6) or switch to using automatic
numbering (all items prefixed with "1.") and ensure the items mentioning exit
code, stdout JSON parsing, verbose mode, and handling success/error JSON
structures remain in the same order and content; reference the "Best Practices
for Programmatic Usage" section and the list entries containing "Always check
the exit code", "Parse stdout", "Use verbose mode", and "Handle both success and
error JSON structures" to locate and fix the numbering.
---
Nitpick comments:
In `@pkg/cmd/conversion.go`:
- Around line 68-73: In outputErrorIfJSON, stop ignoring the error returned by
formatErrorJSON: capture its second return value and if non-nil log or print
that formatting error (alongside the original err) to cmd.OutOrStdout() so
formatting failures are visible; update the call to formatErrorJSON in
outputErrorIfJSON and use cmd.OutOrStdout() (or fmt.Fprintf/Fprintln) to emit a
clear message like "error formatting JSON: <formatErr>" when formatErrorJSON
returns an error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91b87890-98d4-438c-9040-63ada7f5413c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
AGENTS.mdREADME.mdgo.modpkg/cmd/conversion.gopkg/cmd/conversion_test.gopkg/cmd/init.gopkg/cmd/init_test.gopkg/cmd/workspace_list.gopkg/cmd/workspace_list_test.gopkg/cmd/workspace_remove.gopkg/cmd/workspace_remove_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/cmd/conversion_test.go (3)
184-252: Consider adding a test case for nil error input.The
formatErrorJSONfunction handles nil errors by returning("", nil). Adding a test case would improve coverage and document this behavior.Suggested test case
t.Run("returns empty string for nil error", func(t *testing.T) { t.Parallel() result, jsonErr := formatErrorJSON(nil) if jsonErr != nil { t.Fatalf("formatErrorJSON failed: %v", jsonErr) } if result != "" { t.Errorf("Expected empty string for nil error, got '%s'", result) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/conversion_test.go` around lines 184 - 252, Add a unit test that verifies formatErrorJSON(nil) returns an empty string and no error: inside TestFormatErrorJSON add a t.Run block named like "returns empty string for nil error" that calls formatErrorJSON(nil), asserts jsonErr == nil, and asserts result == ""; reference the existing TestFormatErrorJSON suite and the formatErrorJSON function to place the new subtest adjacent to the other t.Run cases.
54-54: Handle error fromNewInstanceFromDatato avoid masking test setup failures.Ignoring the error here could lead to confusing test failures or panics if the data is invalid. Check the error to ensure test setup succeeded.
Proposed fix
- instance, _ = instances.NewInstanceFromData(instanceData) + instance, err = instances.NewInstanceFromData(instanceData) + if err != nil { + t.Fatalf("Failed to create instance from data: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/conversion_test.go` at line 54, The call to instances.NewInstanceFromData currently ignores its error return, which can hide test setup failures; update the test to capture the returned error (e.g., err := instances.NewInstanceFromData(instanceData)), assert or fail the test if err != nil (using t.Fatalf or require.NoError), and assign the instance only after the error check so tests fail fast when NewInstanceFromData fails.
104-104: Handle error fromNewInstanceFromData.Same issue as above—check the error to avoid masking setup failures.
Proposed fix
- instance, _ = instances.NewInstanceFromData(instanceData) + instance, err = instances.NewInstanceFromData(instanceData) + if err != nil { + t.Fatalf("Failed to create instance from data: %v", err) + }Apply the same fix at line 143.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/conversion_test.go` at line 104, The call to instances.NewInstanceFromData is ignoring its error return; update the test to capture and handle the error (e.g., instance, err := instances.NewInstanceFromData(instanceData) and fail the test if err != nil or use require.NoError/require.Nil) so setup failures are not masked. Apply this change for the occurrence that currently does "instance, _ = instances.NewInstanceFromData(instanceData)" and the other identical call later in the file that needs the same fix, ensuring you reference NewInstanceFromData and the instance variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/conversion_test.go`:
- Around line 184-252: Add a unit test that verifies formatErrorJSON(nil)
returns an empty string and no error: inside TestFormatErrorJSON add a t.Run
block named like "returns empty string for nil error" that calls
formatErrorJSON(nil), asserts jsonErr == nil, and asserts result == "";
reference the existing TestFormatErrorJSON suite and the formatErrorJSON
function to place the new subtest adjacent to the other t.Run cases.
- Line 54: The call to instances.NewInstanceFromData currently ignores its error
return, which can hide test setup failures; update the test to capture the
returned error (e.g., err := instances.NewInstanceFromData(instanceData)),
assert or fail the test if err != nil (using t.Fatalf or require.NoError), and
assign the instance only after the error check so tests fail fast when
NewInstanceFromData fails.
- Line 104: The call to instances.NewInstanceFromData is ignoring its error
return; update the test to capture and handle the error (e.g., instance, err :=
instances.NewInstanceFromData(instanceData) and fail the test if err != nil or
use require.NoError/require.Nil) so setup failures are not masked. Apply this
change for the occurrence that currently does "instance, _ =
instances.NewInstanceFromData(instanceData)" and the other identical call later
in the file that needs the same fix, ensuring you reference NewInstanceFromData
and the instance variable when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7892867f-e2a6-4b4e-9eb9-29ac63eb2977
📒 Files selected for processing (2)
AGENTS.mdpkg/cmd/conversion_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
benoitf
approved these changes
Mar 12, 2026
Contributor
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
tried list
./kortex-cli list -o json
"items": [
{
"id": "9d6c6720b5868cda8eda30143cc433a41cf9c796946c14eb1c2be9dd2f3d249c",
"name": "kortex-cli-2",
"paths": {
"configuration": "/Users/benoitf/git/kortex/kortex-cli/.kortex",
"source": "/Users/benoitf/git/kortex/kortex-cli"
}
}
]
}
Updates the kortex-cli-api dependency to the latest version. Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds --output flag to init, remove, and list commands enabling JSON output for programmatic consumption. Creates shared conversion helpers to ensure consistent JSON formatting across all commands following the OpenAPI schema from kortex-cli-api package. Key changes: - New conversion helpers in pkg/cmd/conversion.go with outputErrorIfJSON - Init command returns WorkspaceId by default, full Workspace with --verbose - Remove and list commands return appropriate JSON structures - All errors formatted as JSON when --output=json is set - Comprehensive tests and AGENTS.md documentation added Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Adds a new "Scenarios" section demonstrating how to manage workspaces from UIs or scripts using JSON output. Includes a complete workflow with 6 steps showing list, init, remove operations, error handling examples, best practices, and a bash script pattern. Closes kortex-hub#54 Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Replaces hardcoded Unix path "/this/path/does/not/exist" with a cross-platform approach that creates a file in tempDir and uses it as a parent directory. This reliably fails os.MkdirAll on all platforms (Windows, Linux, macOS). Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Fixes Windows test failures by replacing hardcoded Unix paths like "/path/to/source" with t.TempDir(). Enhances AGENTS.md with prominent warnings, common mistakes section, and clear examples of what NOT to do in tests to prevent future cross-platform issues. Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
Contributor
Author
|
closing/reopening to trigger Semantic PR review |
Contributor
|
I noticed that one trick @simonrey1 was doing is to update the title (like one extra space and then remove the space) to trigger semantic PR check again |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds --output flag to init, remove, and list commands enabling JSON output for programmatic consumption. Creates shared conversion helpers to ensure consistent JSON formatting across all commands following the OpenAPI schema from kortex-cli-api package.
Key changes:
Updates the kortex-cli-api dependency to the latest version.
Adds a new "Scenarios" section demonstrating how to manage workspaces from UIs or scripts using JSON output. Includes a complete workflow with 6 steps showing list, init, remove operations, error handling examples, best practices, and a bash script pattern.
Closes #54