Skip to content

feat: add user feedback for volume attach/detach commands #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cokerrd wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add user feedback for volume attach/detach commands #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions internal/commands/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,18 +206,13 @@ func newVolumeAttachCmd() *cobra.Command {
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(getTimeout(cmd))*time.Second)
defer cancel()

vol, err := svc.Attach(ctx, volumeSlug, vmSlug)
resp, err := svc.Attach(ctx, volumeSlug, vmSlug)
if err != nil {
return fmt.Errorf("volume attach: %w", err)
}

headers := []string{"SLUG", "NAME", "SIZE", "VM ID"}
rows := [][]string{{
vol.Slug,
vol.Name,
formatSize(vol.Size),
vol.VirtualMachineID,
}}
headers := []string{"STATUS", "MESSAGE"}
rows := [][]string{{resp.Status, resp.Message}}
return printer.PrintTable(headers, rows)
},
}
Expand All @@ -241,7 +236,7 @@ func newVolumeDetachCmd() *cobra.Command {
ctx, cancel := context.WithTimeout(context.Background(), time.Duration(getTimeout(cmd))*time.Second)
defer cancel()

vol, err := svc.Detach(ctx, volumeSlug)
resp, err := svc.Detach(ctx, volumeSlug)
if err != nil {
if apierrors.IsResourceNotFound(err) {
fmt.Fprintf(os.Stderr, "Volume %q not found — already detached or deleted.\n", volumeSlug)
Expand All @@ -250,12 +245,8 @@ func newVolumeDetachCmd() *cobra.Command {
return fmt.Errorf("volume detach: %w", err)
}

headers := []string{"SLUG", "NAME", "SIZE"}
rows := [][]string{{
vol.Slug,
vol.Name,
formatSize(vol.Size),
}}
headers := []string{"STATUS", "MESSAGE"}
rows := [][]string{{resp.Status, resp.Message}}
return printer.PrintTable(headers, rows)
},
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/api/volume/volume.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,24 +172,24 @@ func (s *Service) Create(ctx context.Context, req CreateRequest) (*Volume, error
}

// 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) {
body := AttachRequest{VirtualMachine: vmSlug}
var resp singleResponse
path := fmt.Sprintf("/blockstorages/%s/attach", volumeSlug)
if err := s.client.Post(ctx, path, body, &resp); err != nil {
return nil, fmt.Errorf("attaching block storage %s to VM %s: %w", volumeSlug, vmSlug, err)
}
return &resp.Data, nil
return &resp, nil
}

// Detach detaches a volume from its virtual machine.
func (s *Service) Detach(ctx context.Context, volumeSlug string) (*Volume, error) {
func (s *Service) Detach(ctx context.Context, volumeSlug string) (*singleResponse, error) {
var resp singleResponse
path := fmt.Sprintf("/blockstorages/%s/detach", volumeSlug)
if err := s.client.Post(ctx, path, nil, &resp); err != nil {
return nil, fmt.Errorf("detaching block storage %s: %w", volumeSlug, err)
}
return &resp.Data, nil
return &resp, nil
}

// Delete permanently deletes a block storage volume. The volume must be detached first.
Expand Down
22 changes: 14 additions & 8 deletions pkg/api/volume/volume_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,12 @@ func TestVolumeAttach(t *testing.T) {
json.NewDecoder(r.Body).Decode(&gotBody)
result := volume.Volume{ID: "vol-1", Slug: "root-4153", VirtualMachineID: "vm-1"}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(singleResponse{Status: "Success", Message: "Ok", Data: result})
json.NewEncoder(w).Encode(singleResponse{Status: "Success", Message: "Attaching block storage.", Data: result})
}))
defer srv.Close()

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")
if err != nil {
t.Fatalf("Attach() error = %v", err)
}
Expand All @@ -157,8 +157,11 @@ func TestVolumeAttach(t *testing.T) {
if gotBody["virtual_machine"] != "test-vm-1" {
t.Errorf("body virtual_machine = %v, want %q", gotBody["virtual_machine"], "test-vm-1")
}
if vol.VirtualMachineID != "vm-1" {
t.Errorf("vol.VirtualMachineID = %q, want %q", vol.VirtualMachineID, "vm-1")
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")
Comment on lines +160 to +164

@coderabbitai coderabbitai Bot Jul 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

}
}

Expand All @@ -169,12 +172,12 @@ func TestVolumeDetach(t *testing.T) {
gotPath = r.URL.Path
result := volume.Volume{ID: "vol-1", Slug: "root-4153"}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(singleResponse{Status: "Success", Message: "Ok", Data: result})
json.NewEncoder(w).Encode(singleResponse{Status: "Success", Message: "Detaching block storage.", Data: result})
}))
defer srv.Close()

svc := volume.NewService(newTestClient(t, srv))
vol, err := svc.Detach(context.Background(), "root-4153")
resp, err := svc.Detach(context.Background(), "root-4153")
if err != nil {
t.Fatalf("Detach() error = %v", err)
}
Expand All @@ -184,8 +187,11 @@ func TestVolumeDetach(t *testing.T) {
if gotPath != "/blockstorages/root-4153/detach" {
t.Errorf("path = %q, want %q", gotPath, "/blockstorages/root-4153/detach")
}
if vol.Slug != "root-4153" {
t.Errorf("vol.Slug = %q, want %q", vol.Slug, "root-4153")
if resp.Status != "Success" {
t.Errorf("resp.Status = %q, want %q", resp.Status, "Success")
}
if resp.Message != "Detaching block storage." {
t.Errorf("resp.Message = %q, want %q", resp.Message, "Detaching block storage.")
}
}

Expand Down
Loading