feat: add user feedback for volume attach/detach commands - #46
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 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.
ditahkk
left a comment
ditahkk
left a comment
There was a problem hiding this comment.
Thanks. Blank output on success was confusing and your diagnosis is correct. Two inline notes before merge.
Also, we added a DCO sign-off requirement after you opened this PR. See CONTRIBUTING.md. Rebase with --signoff and force-push to pass the new check.
| formatSize(vol.Size), | ||
| vol.VirtualMachineID, | ||
| }} | ||
| headers := []string{"STATUS", "MESSAGE"} |
There was a problem hiding this comment.
The output no longer names the volume or VM, and scripts reading --output json lose the old keys. Since the API returns nothing, consider echoing the slugs the user passed in, or follow the pattern firewall create and portforward create use. Your choice on the shape. Please also note the output change in CHANGELOG.md.
|
|
||
| // Attach attaches a volume to a virtual machine. | ||
| func (s *Service) Attach(ctx context.Context, volumeSlug, vmSlug string) (*Volume, error) { | ||
| func (s *Service) Attach(ctx context.Context, volumeSlug, vmSlug string) (*singleResponse, error) { |
There was a problem hiding this comment.
Same return-type issue as on #45. Attach and Detach now return the unexported singleResponse, and the old signature breaks external callers. An exported result type fixes both. instance.ActionResponse is an existing pattern to mirror if useful.
|
|
||
| svc := volume.NewService(newTestClient(t, srv)) | ||
| vol, err := svc.Attach(context.Background(), "root-4153", "test-vm-1") | ||
| resp, err := svc.Attach(context.Background(), "root-4153", "test-vm-1") |
There was a problem hiding this comment.
The mock still builds a full Volume fixture but nothing asserts on it now. Trim it or assert on it. Fine to skip.
There was a problem hiding this comment.
got it, I'll just assert on it
970d985 to
f954be3
Compare
Signed-off-by: Coker Richard <82083946+cokerrd@users.noreply.github.com>
f954be3 to
77249d2
Compare
|
updates have been pushed |
2 participants
Adds feedback on the
volume attachandvolume detachcommands. Previously the commands on success would display blank responsesChanges
statusandmessagenew.mov
Summary by CodeRabbit
Improvements
Tests