Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🧹 Nitpick comments (1)
pkg/cmd/workspace_list_test.go (1)
62-99: ExercisepreRundirectly in these unit-style cases.These subtests only assert that
Execute()returns nil, so they would still pass ifpreRunstopped persistingworkspaceListCmd.outputand the command silently fell back to text output on an empty store. CallingpreRundirectly and checkingmanager/outputwould cover the added state change much more precisely.Based on learnings, "Command tests should have two types of tests: unit tests that test the
preRunmethod directly, and E2E tests that test full command execution viarootCmd.Execute()" and "In unit tests for commands, uset.Run()for subtests within a parent test function and verify struct fields are set correctly afterpreRunexecution".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/workspace_list_test.go` around lines 62 - 99, The tests currently only call rootCmd.Execute(), which doesn't verify preRun mutated command state; instead, instantiate the workspace list command via NewRootCmd() and locate the workspaceListCmd instance, call its preRun method directly (or the method exposed for setup) with a fresh context and args, then assert the command's fields (workspaceListCmd.manager and workspaceListCmd.output) are set as expected for each flag case (no output, --output json, -o json); keep the existing Execute() E2E tests but add these unit-style subtests to validate preRun behavior and ensure manager/output are persisted correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/workspace_list.go`:
- Around line 21-27: preRun currently passes the raw storageDir flag value into
instances.NewManager without normalization; update the preRun handler (the
function that reads the --storage flag) to call filepath.Abs(storageDir) and use
the resulting absolute path (or return/log an error if Abs fails) before passing
it into instances.NewManager so all storage paths are deterministic; reference
the storageDir variable in preRun and the call to instances.NewManager to locate
where to perform the normalization.
- Around line 116-124: The JSON output write ignores possible errors from
fmt.Fprintln; update the code after calling json.MarshalIndent to capture and
return any write error from writing to cmd.OutOrStdout() (i.e., replace the
fmt.Fprintln call with writing that checks the returned error and returns
fmt.Errorf or the raw error), so failures when the stdout consumer closes early
are propagated; keep the existing JSON marshalling error handling around
json.MarshalIndent and ensure the function (where this appears) returns the
write error instead of always returning nil.
---
Nitpick comments:
In `@pkg/cmd/workspace_list_test.go`:
- Around line 62-99: The tests currently only call rootCmd.Execute(), which
doesn't verify preRun mutated command state; instead, instantiate the workspace
list command via NewRootCmd() and locate the workspaceListCmd instance, call its
preRun method directly (or the method exposed for setup) with a fresh context
and args, then assert the command's fields (workspaceListCmd.manager and
workspaceListCmd.output) are set as expected for each flag case (no output,
--output json, -o json); keep the existing Execute() E2E tests but add these
unit-style subtests to validate preRun behavior and ensure manager/output are
persisted correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fcd63f4-6528-43bf-a047-e2afc8b77b7a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpkg/cmd/workspace_list.gopkg/cmd/workspace_list_test.go
Signed-off-by: Philippe Martin <phmartin@redhat.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/cmd/workspace_list.go (1)
116-124:⚠️ Potential issue | 🟡 MinorReturn JSON write failures instead of dropping them.
Line 123 ignores the error from
fmt.Fprintln, so a broken pipe or closed stdout still reports success. Writing throughjson.NewEncoder(...).Encode(...)fixes that and also avoids the extra buffer copy.Suggested fix
- // Marshal to JSON with indentation - jsonData, err := json.MarshalIndent(workspacesList, "", " ") - if err != nil { - return fmt.Errorf("failed to marshal workspaces to JSON: %w", err) - } - - // Output the JSON to stdout - fmt.Fprintln(cmd.OutOrStdout(), string(jsonData)) - return nil + encoder := json.NewEncoder(cmd.OutOrStdout()) + encoder.SetIndent("", " ") + if err := encoder.Encode(workspacesList); err != nil { + return fmt.Errorf("failed to write workspaces as JSON: %w", err) + } + return nil#!/bin/bash set -euo pipefail sed -n '116,124p' pkg/cmd/workspace_list.go rg -n -C2 'fmt\.Fprintln\(cmd\.OutOrStdout\(\), string\(jsonData\)\)' pkg/cmd/workspace_list.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/workspace_list.go` around lines 116 - 124, The code currently marshals workspacesList to a buffer and calls fmt.Fprintln(cmd.OutOrStdout(), string(jsonData)) which ignores write errors; replace this with json.NewEncoder(cmd.OutOrStdout()).Encode(workspacesList) (or call encoder.Encode on the already-created jsonData source) so the write error is returned and not dropped—update the function that performs the output (the block using json.MarshalIndent and fmt.Fprintln) to use json.NewEncoder(cmd.OutOrStdout()).Encode(...) and return any error from Encode instead of discarding it.
🧹 Nitpick comments (1)
pkg/cmd/workspace_list_test.go (1)
62-133: TestpreRun()directly in the unit-test section.These cases currently execute the full command, so a failure in
run()can mask whetherpreRun()handled--outputcorrectly. Please instantiateworkspaceListCmd, callpreRun()directly, and assert the parsed state (output,manager) there; keeprootCmd.Execute()coverage in the E2E block.As per coding guidelines: "Command tests should have two types of tests: unit tests that test the
preRunmethod directly, and E2E tests that test full command execution viarootCmd.Execute()" and "In unit tests for commands, uset.Run()for subtests within a parent test function and verify struct fields are set correctly afterpreRunexecution".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/workspace_list_test.go` around lines 62 - 133, The tests are exercising the full command (rootCmd.Execute()) so failures in run() can hide whether preRun() parsed flags correctly; modify the unit tests to instantiate workspaceListCmd directly, call its preRun(cmd, args) method, and assert the command's fields (output and manager) are set as expected (e.g., output == "json" or error for invalid formats) instead of relying only on rootCmd.Execute(); keep existing E2E Execute() tests separate. Locate workspaceListCmd, its preRun and run methods and replace the subtests that currently call rootCmd.Execute() with calls that construct workspaceListCmd, set Flags/Args as needed, call preRun(), and assert workspaceListCmd.output and workspaceListCmd.manager (or expected error) to validate flag parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/cmd/workspace_list.go`:
- Around line 116-124: The code currently marshals workspacesList to a buffer
and calls fmt.Fprintln(cmd.OutOrStdout(), string(jsonData)) which ignores write
errors; replace this with
json.NewEncoder(cmd.OutOrStdout()).Encode(workspacesList) (or call
encoder.Encode on the already-created jsonData source) so the write error is
returned and not dropped—update the function that performs the output (the block
using json.MarshalIndent and fmt.Fprintln) to use
json.NewEncoder(cmd.OutOrStdout()).Encode(...) and return any error from Encode
instead of discarding it.
---
Nitpick comments:
In `@pkg/cmd/workspace_list_test.go`:
- Around line 62-133: The tests are exercising the full command
(rootCmd.Execute()) so failures in run() can hide whether preRun() parsed flags
correctly; modify the unit tests to instantiate workspaceListCmd directly, call
its preRun(cmd, args) method, and assert the command's fields (output and
manager) are set as expected (e.g., output == "json" or error for invalid
formats) instead of relying only on rootCmd.Execute(); keep existing E2E
Execute() tests separate. Locate workspaceListCmd, its preRun and run methods
and replace the subtests that currently call rootCmd.Execute() with calls that
construct workspaceListCmd, set Flags/Args as needed, call preRun(), and assert
workspaceListCmd.output and workspaceListCmd.manager (or expected error) to
validate flag parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68af2bb5-37f3-4e22-bb10-e82a7e5d35d6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modpkg/cmd/workspace_list.gopkg/cmd/workspace_list_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
2 participants
adds JSON output to the
listcommand, using theWorkspacesListinterface defined in https://github.com/kortex-hub/kortex-cli-api v0.1.3