feat: add user feedback for volume attach/detach commands - #46
Conversation
📝 WalkthroughWalkthroughVolume attach and detach now propagate the complete API response envelope and display its ChangesVolume response propagation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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/api/volume/volume.go (1)
175-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the response type used by the exported methods.
AttachandDetachnow return*singleResponse, butsingleResponseis unexported. Callers can use inferred values, but cannot name this type in interfaces, mocks, or function signatures. Rename it toSingleResponseor return another exported response type.Also applies to: 186-192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/api/volume/volume.go` around lines 175 - 183, Export the response type returned by the exported Attach and Detach methods by renaming singleResponse to SingleResponse and updating all references, including method signatures, local variables, and related response uses. Preserve the existing response fields and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/api/volume/volume_test.go`:
- Around line 160-164: Extend the attach response assertions in the relevant
volume tests, alongside the existing Status and Message checks, to verify that
resp.Data preserves the mock values for ID, Slug, and VirtualMachineID. Apply
the same envelope-data assertions to the additional test block referenced by the
comment, covering the new &resp return contract end to end.
---
Nitpick comments:
In `@pkg/api/volume/volume.go`:
- Around line 175-183: Export the response type returned by the exported Attach
and Detach methods by renaming singleResponse to SingleResponse and updating all
references, including method signatures, local variables, and related response
uses. Preserve the existing response fields and behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3c31ca0f-1f9d-4b23-b482-128933870757
📒 Files selected for processing (3)
internal/commands/volume.gopkg/api/volume/volume.gopkg/api/volume/volume_test.go
| if resp.Message != "Attaching block storage." { | ||
| t.Errorf("resp.Message = %q, want %q", resp.Message, "Attaching block storage.") | ||
| } | ||
| if resp.Status != "Success" { | ||
| t.Errorf("resp.Status = %q, want %q", resp.Status, "Success") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that the full response envelope preserves Data.
The mocks populate Data, but these tests only check Status and Message. Add assertions for fields such as resp.Data.ID, resp.Data.Slug, and resp.Data.VirtualMachineID for attach, so the new &resp return contract is covered end to end.
Proposed assertions
if resp.Status != "Success" {
t.Errorf("resp.Status = %q, want %q", resp.Status, "Success")
}
+if resp.Data.ID != "vol-1" {
+ t.Errorf("resp.Data.ID = %q, want %q", resp.Data.ID, "vol-1")
+}
+if resp.Data.Slug != "root-4153" {
+ t.Errorf("resp.Data.Slug = %q, want %q", resp.Data.Slug, "root-4153")
+}Also applies to: 190-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/api/volume/volume_test.go` around lines 160 - 164, Extend the attach
response assertions in the relevant volume tests, alongside the existing Status
and Message checks, to verify that resp.Data preserves the mock values for ID,
Slug, and VirtualMachineID. Apply the same envelope-data assertions to the
additional test block referenced by the comment, covering the new &resp return
contract end to end.
1 participant
Adds feedback on the
volume attachandvolume detachcommands. Previously the commands on success would display blank responsesChanges
statusandmessagenew.mov
Summary by CodeRabbit
Improvements
Tests