Skip to content

volume delete: allow prefix for ID#24997

Merged
tgross merged 1 commit intomainfrom
dhv-cli-volume-delete
Feb 3, 2025
Merged

volume delete: allow prefix for ID#24997
tgross merged 1 commit intomainfrom
dhv-cli-volume-delete

Conversation

@tgross
Copy link
Copy Markdown
Member

@tgross tgross commented Jan 31, 2025

The volume delete command doesn't allow using a prefix for the volume ID for either CSI or dynamic host volumes. Use a prefix search and wildcard namespace as we do for other CLI commands.

Ref: https://hashicorp.atlassian.net/browse/NET-12057


Example of use:

$ nomad volume create ./internal-plugin.volume.hcl
==> Created host volume internal-plugin with ID aeea91a0-06df-c16e-5403-ff82a2f28fd4
  ✓ Host volume "aeea91a0" ready

    2025-01-31T15:55:14-05:00
    ID        = aeea91a0-06df-c16e-5403-ff82a2f28fd4
    Name      = internal-plugin
    Namespace = default
    Plugin ID = mkdir
    Node ID   = b4611abd-d4a8-c83a-b05e-7d9f5b44a179
    Node Pool = default
    Capacity  = 0 B
    State     = ready
    Host Path = /run/nomad/dev/data/host_volumes/aeea91a0-06df-c16e-5403-ff82a2f28fd4

$ nomad volume delete -type host aeea91a0
Successfully deleted volume "aeea91a0-06df-c16e-5403-ff82a2f28fd4"!

gulducat
gulducat previously approved these changes Jan 31, 2025
Copy link
Copy Markdown
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

lgtm!

Comment thread command/volume_delete.go
Comment thread command/volume_delete.go
Comment on lines +151 to +153
if len(possible) > 0 {
out, err := csiFormatVolumes(possible, false, "")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not seeing test coverage for this, and if it were handled incorrectly (a bug introduced later?), it could cause external persistent state to be lost.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We don't have any CLI unit coverage for the volume delete command and very little meaningful coverage of the CSI HTTP APIs at all for that matter, because of the setup requirements. We pretty much just test the routing and that's about it; the functionality is being covered in the RPC handler tests. The juice isn't worth the squeeze for coverage here.

The `volume delete` command doesn't allow using a prefix for the volume ID for
either CSI or dynamic host volumes. Use a prefix search and wildcard namespace
as we do for other CLI commands.

Ref: https://hashicorp.atlassian.net/browse/NET-12057
Copy link
Copy Markdown
Member

@gulducat gulducat left a comment

Choose a reason for hiding this comment

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

👍

@tgross tgross merged commit 7929939 into main Feb 3, 2025
@tgross tgross deleted the dhv-cli-volume-delete branch February 3, 2025 16:29
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 4, 2025

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants