Conversation
|
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 selected for processing (6)
📝 WalkthroughWalkthroughAdds a new top-level Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as Stop Command
participant Manager as instances.Manager
participant Storage as Storage (file system)
User->>CLI: stop <workspace-id>
CLI->>CLI: PreRunE: read --storage
CLI->>Storage: NewManager(storageDir)
Storage-->>Manager: manager instance
CLI->>Manager: Delete(instanceID)
alt Workspace Found
Manager->>Storage: remove instance record
Manager-->>CLI: success
CLI-->>User: print workspace ID
else Workspace Not Found
Manager-->>CLI: ErrInstanceNotFound
CLI-->>User: error + suggest "workspace list"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. 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: 3
🧹 Nitpick comments (1)
pkg/cmd/workspace_stop_test.go (1)
43-114: Exercise(*workspaceStopCmd).preRundirectly in the unit test.Lines 43-114 still drive
rootCmd.Execute(), so they only recheck Cobra wiring and never assert thatpreRunstoresidand constructsmanageron the command struct itself. A direct unit test here would isolate setup regressions from the E2E path. As per coding guidelines "Command tests should have two types of tests: unit tests that test thepreRunmethod 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_stop_test.go` around lines 43 - 114, The tests currently exercise the Cobra wiring via rootCmd.Execute() but do not unit-test (*workspaceStopCmd).preRun directly; add a unit subtest that constructs the workspace stop command struct (or obtains the *cobra.Command and its underlying *workspaceStopCmd), sets Args to simulate one ID and sets the --storage flag to a temp dir, then call workspaceStopCmd.preRun(cmd, args) directly and assert that workspaceStopCmd.id equals the provided ID and workspaceStopCmd.manager is non-nil (and can be used to find the instance); reference the preRun method and the workspaceStopCmd.id and workspaceStopCmd.manager fields when locating code to change.
🤖 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_stop.go`:
- Around line 46-50: The code constructs a new manager (instances.NewManager)
and immediately performs deletions without first reconciling stale/unreachable
workspace registrations; before calling the delete path (the code that invokes
manager.Delete in workspace_stop.go) invoke the manager's reconciliation routine
(e.g., Manager.Reconcile() or Manager.UnregisterInaccessible() — whichever is
implemented in pkg/instances/manager.go) right after w.manager = manager so that
inaccessible workspaces are unregistered and the subsequent calls to Delete
operate on up-to-date state.
- Around line 56-63: The stop command currently passes the raw argument to
Manager.Delete which only matches by GetID, so implement resolution of the
provided identifier to support both UID and workspace name: first try to find a
workspace whose GetID() equals the input, if not found iterate the manager's
listing (or call a GetByName helper if available) to match GetName() (or
equivalent) and use the found instance's ID when calling Manager.Delete; update
the workspaceStopCmd.run logic (and analogous branch at lines ~75-77) to perform
this lookup before Delete, and update the command Use/help text in the stop
command and corresponding tests to reflect that the command accepts either UID
or name.
- Around line 39-50: The --storage flag value (storageDir) must be normalized to
an absolute path in the command preRun before passing it to
instances.NewManager; import "path/filepath", call filepath.Abs(storageDir)
(handle and wrap any error) and use the absolute path when constructing the
manager (locations: the storageDir variable, preRun logic, and
instances.NewManager call).
---
Nitpick comments:
In `@pkg/cmd/workspace_stop_test.go`:
- Around line 43-114: The tests currently exercise the Cobra wiring via
rootCmd.Execute() but do not unit-test (*workspaceStopCmd).preRun directly; add
a unit subtest that constructs the workspace stop command struct (or obtains the
*cobra.Command and its underlying *workspaceStopCmd), sets Args to simulate one
ID and sets the --storage flag to a temp dir, then call
workspaceStopCmd.preRun(cmd, args) directly and assert that workspaceStopCmd.id
equals the provided ID and workspaceStopCmd.manager is non-nil (and can be used
to find the instance); reference the preRun method and the workspaceStopCmd.id
and workspaceStopCmd.manager fields when locating code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17f8f19b-4f31-43fe-ba8c-f2ea4b20159d
📒 Files selected for processing (6)
pkg/cmd/root.gopkg/cmd/stop.gopkg/cmd/stop_test.gopkg/cmd/workspace.gopkg/cmd/workspace_stop.gopkg/cmd/workspace_stop_test.go
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
hello,
question: I would assume stop would stop the workspace but then I can resume the agent or restart it ? It's like going back to the 'init/stopped' state
I can then list all the agents, including the stopped one and decide or not to delete it. I could have an option delete on stop but I'm not sure it should be the default
if it's deleting the agent should it be a 'delete' or 'remove' option ?
@slemeur WDYT? In https://redhat-internal.slack.com/archives/C09DNJCKZJQ/p1772793346412079?thread_ts=1772791436.021539&cid=C09DNJCKZJQ you are defining a STOP command, but no START or DELETE ones. Do we want to have an intermediary STOPPED level for a workspace? Or all existing workspaces will be running, and we delete them when we stop them? |
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
2 participants
Adds a
workspace stopcommand and astopalias, which removes the workspace from the list of workspacesclose #17