Skip to content

Commit d2e2338

Browse files
hi-leihi-lei
authored
Require --all for filter flags (--status, --hostname) in batch operations (#31)
Filter flags like --status and --hostname now require --all to be explicitly set, making batch intent clear. Previously --hostname alone could trigger batch mode, which was confusing. Also fixes lint issues and adds tests.
1 parent 79cf7b7 commit d2e2338

6 files changed

Lines changed: 106 additions & 29 deletions

File tree

‎internal/verda-cli/cmd/vm/action.go‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -149,10 +149,22 @@ func NewCmdAction(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command
149149

150150
//nolint:gocyclo // Interactive CLI command with prompts, confirmation, and spinner — inherently complex.
151151
func runAction(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams, opts *actionOptions) error {
152+
// Validate: --status and --hostname are filters that require --all.
153+
if !opts.All && (opts.Status != "" || opts.Hostname != "") {
154+
flags := []string{}
155+
if opts.Status != "" {
156+
flags = append(flags, "--status")
157+
}
158+
if opts.Hostname != "" {
159+
flags = append(flags, "--hostname")
160+
}
161+
return fmt.Errorf("%s can only be used with --all", strings.Join(flags, " and "))
162+
}
163+
152164
// In agent mode, --id and --action are required.
153165
if f.AgentMode() {
154166
var missing []string
155-
if opts.InstanceID == "" && !opts.All && opts.Hostname == "" {
167+
if opts.InstanceID == "" && !opts.All {
156168
missing = append(missing, "--id")
157169
}
158170
if opts.Action == "" {
@@ -163,8 +175,8 @@ func runAction(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStream
163175
}
164176
}
165177

166-
// Batch mode: --all or --hostname routes to batch execution.
167-
if opts.All || opts.Hostname != "" {
178+
// Batch mode: --all routes to batch execution.
179+
if opts.All {
168180
return runBatchAction(cmd, f, ioStreams, opts)
169181
}
170182

‎internal/verda-cli/cmd/vm/batch.go‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,12 @@ import (
1515
)
1616

1717
// runBatchAction validates batch flags and executes the action against all
18-
// matching instances. It is called from runAction when --all or --hostname is set.
19-
//
20-
//nolint:gocyclo // Batch CLI command with validation, filtering, confirmation, and execution — inherently complex.
18+
// matching instances. It is called from runAction when --all is set.
19+
// --status and --hostname act as filters (AND logic) to narrow the target set.
2120
func runBatchAction(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams, opts *actionOptions) error {
22-
// Validation: --all/--hostname cannot be combined with --id or a positional instance ID.
21+
// Validation: --all cannot be combined with --id or a positional instance ID.
2322
if opts.InstanceID != "" {
24-
return errors.New("cannot combine --all/--hostname with --id or positional instance ID")
23+
return errors.New("cannot combine --all with --id or positional instance ID")
2524
}
2625

2726
// Validation: --with-volumes is only valid for delete.
@@ -31,11 +30,7 @@ func runBatchAction(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOS
3130

3231
// Agent mode requires --yes for batch operations (always destructive at scale).
3332
if f.AgentMode() && !opts.Yes {
34-
label := opts.Action + " --all"
35-
if opts.Hostname != "" {
36-
label = opts.Action + " --hostname"
37-
}
38-
return cmdutil.NewConfirmationRequiredError(label)
33+
return cmdutil.NewConfirmationRequiredError(opts.Action + " --all")
3934
}
4035

4136
client, err := f.VerdaClient()

‎internal/verda-cli/cmd/vm/batch_test.go‎

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ func TestBatchRejectsAllWithID(t *testing.T) {
3030
if err == nil {
3131
t.Fatal("expected error when combining --all with --id")
3232
}
33-
if !strings.Contains(err.Error(), "cannot combine --all/--hostname with --id") {
33+
if !strings.Contains(err.Error(), "cannot combine --all with --id") {
3434
t.Fatalf("unexpected error: %v", err)
3535
}
3636
}
@@ -50,11 +50,58 @@ func TestBatchRejectsAllWithPositionalArg(t *testing.T) {
5050
if err == nil {
5151
t.Fatal("expected error when combining --all with positional arg")
5252
}
53-
if !strings.Contains(err.Error(), "cannot combine --all/--hostname with --id") {
53+
if !strings.Contains(err.Error(), "cannot combine --all with --id") {
5454
t.Fatalf("unexpected error: %v", err)
5555
}
5656
}
5757

58+
func TestFiltersRequireAll(t *testing.T) {
59+
t.Parallel()
60+
61+
var buf bytes.Buffer
62+
ioStreams := cmdutil.IOStreams{Out: &buf, ErrOut: &buf}
63+
f := &cmdutil.TestFactory{AgentModeOverride: true}
64+
65+
tests := []struct {
66+
name string
67+
args []string
68+
want string
69+
}{
70+
{
71+
name: "hostname without all",
72+
args: []string{"vm", "shutdown", "--hostname", "test-*"},
73+
want: "--hostname can only be used with --all",
74+
},
75+
{
76+
name: "status without all",
77+
args: []string{"vm", "shutdown", "--status", "running"},
78+
want: "--status can only be used with --all",
79+
},
80+
{
81+
name: "both filters without all",
82+
args: []string{"vm", "shutdown", "--status", "running", "--hostname", "test-*"},
83+
want: "--status and --hostname can only be used with --all",
84+
},
85+
}
86+
87+
for _, tt := range tests {
88+
t.Run(tt.name, func(t *testing.T) {
89+
t.Parallel()
90+
root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true}
91+
root.AddCommand(NewCmdVM(f, ioStreams))
92+
root.SetArgs(tt.args)
93+
94+
err := root.Execute()
95+
if err == nil {
96+
t.Fatal("expected error for filter without --all")
97+
}
98+
if !strings.Contains(err.Error(), tt.want) {
99+
t.Fatalf("expected %q, got: %v", tt.want, err)
100+
}
101+
})
102+
}
103+
}
104+
58105
func TestBatchRejectsWithVolumesOnNonDelete(t *testing.T) {
59106
t.Parallel()
60107

‎internal/verda-cli/cmd/vm/shortcuts.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,9 @@ func newShortcutCmd(f cmdutil.Factory, ioStreams cmdutil.IOStreams, def shortcut
6565

6666
cmd.Flags().StringVar(&opts.InstanceID, "id", "", "Instance ID (alternative to positional argument)")
6767
cmd.Flags().BoolVar(&opts.Yes, "yes", false, "Skip confirmation for destructive actions")
68-
cmd.Flags().BoolVar(&opts.All, "all", false, "Target all instances matching filters")
69-
cmd.Flags().StringVar(&opts.Status, "status", "", "Filter instances by status (e.g., running, offline)")
70-
cmd.Flags().StringVar(&opts.Hostname, "hostname", "", "Filter instances by hostname glob pattern (e.g., \"test-*\")")
68+
cmd.Flags().BoolVar(&opts.All, "all", false, "Target all instances (use with --status/--hostname to filter)")
69+
cmd.Flags().StringVar(&opts.Status, "status", "", "Filter by status, requires --all (e.g., running, offline)")
70+
cmd.Flags().StringVar(&opts.Hostname, "hostname", "", "Filter by hostname glob pattern, requires --all (e.g., \"test-*\")")
7171
cmd.Flags().BoolVar(&opts.WithVolumes, "with-volumes", false, "Also delete all attached volumes (delete only)")
7272
opts.Wait.AddFlags(cmd.Flags(), true)
7373

@@ -91,7 +91,10 @@ verda vm %s
9191
verda vm %s --all --status %s
9292
9393
# Batch: %s instances matching hostname pattern
94-
verda vm %s --hostname "test-*"`, def.Short, name, name, strings.ToLower(strings.SplitN(def.Short, " ", 2)[0]), name, exampleStatus(def.Action), strings.ToLower(strings.SplitN(def.Short, " ", 2)[0]), name)
94+
verda vm %s --all --hostname "test-*"
95+
96+
# Batch: combine filters (AND logic)
97+
verda vm %s --all --status %s --hostname "test-*"`, def.Short, name, name, strings.ToLower(strings.SplitN(def.Short, " ", 2)[0]), name, exampleStatus(def.Action), strings.ToLower(strings.SplitN(def.Short, " ", 2)[0]), name, name, exampleStatus(def.Action))
9598

9699
if def.Action == verda.ActionDelete {
97100
examples += fmt.Sprintf(`

‎internal/verda-cli/cmd/volume/delete.go‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -55,14 +55,19 @@ func NewCmdDelete(f cmdutil.Factory, ioStreams cmdutil.IOStreams) *cobra.Command
5555
}
5656

5757
cmd.Flags().StringVar(&opts.VolumeID, "id", "", "Volume ID (alternative to positional argument)")
58-
cmd.Flags().BoolVar(&opts.All, "all", false, "Target all volumes matching filters")
59-
cmd.Flags().StringVar(&opts.Status, "status", "", "Filter volumes by status (e.g., detached, attached)")
58+
cmd.Flags().BoolVar(&opts.All, "all", false, "Target all volumes (use with --status to filter)")
59+
cmd.Flags().StringVar(&opts.Status, "status", "", "Filter by status, requires --all (e.g., detached, attached)")
6060
cmd.Flags().BoolVar(&opts.Yes, "yes", false, "Skip confirmation for destructive actions")
6161

6262
return cmd
6363
}
6464

6565
func runDelete(cmd *cobra.Command, f cmdutil.Factory, ioStreams cmdutil.IOStreams, opts *deleteOptions) error {
66+
// Validate: --status is a filter that requires --all.
67+
if !opts.All && opts.Status != "" {
68+
return errors.New("--status can only be used with --all")
69+
}
70+
6671
// Validate: --all cannot combine with --id or positional arg.
6772
if opts.All && opts.VolumeID != "" {
6873
return errors.New("cannot combine --all with --id or positional volume ID")
@@ -138,19 +143,13 @@ func runSingleVolumeDelete(ctx context.Context, f cmdutil.Factory, ioStreams cmd
138143
}
139144

140145
func runInteractiveVolumeDelete(ctx context.Context, f cmdutil.Factory, ioStreams cmdutil.IOStreams, client *verda.Client, opts *deleteOptions) error {
141-
// Load volumes with optional status filter.
146+
// Load all volumes (--status filter is only valid with --all, validated earlier).
142147
var sp interface{ Stop(string) }
143148
if status := f.Status(); status != nil {
144149
sp, _ = status.Spinner(ctx, "Loading volumes...")
145150
}
146151

147-
var volumes []verda.Volume
148-
var err error
149-
if opts.Status != "" {
150-
volumes, err = client.Volumes.ListVolumesByStatus(ctx, opts.Status)
151-
} else {
152-
volumes, err = client.Volumes.ListVolumes(ctx)
153-
}
152+
volumes, err := client.Volumes.ListVolumes(ctx)
154153
if sp != nil {
155154
sp.Stop("")
156155
}

‎internal/verda-cli/cmd/volume/delete_test.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package volume
22

33
import (
44
"bytes"
5+
"strings"
56
"testing"
67

78
"github.com/spf13/cobra"
@@ -60,6 +61,26 @@ func TestDeleteAgentModeRequiresYes(t *testing.T) {
6061
}
6162
}
6263

64+
func TestDeleteStatusRequiresAll(t *testing.T) {
65+
t.Parallel()
66+
67+
var buf bytes.Buffer
68+
ioStreams := cmdutil.IOStreams{Out: &buf, ErrOut: &buf}
69+
f := cmdutil.NewTestFactory(nil)
70+
71+
root := &cobra.Command{Use: "verda", SilenceUsage: true, SilenceErrors: true}
72+
root.AddCommand(NewCmdVolume(f, ioStreams))
73+
root.SetArgs([]string{"volume", "delete", "--status", "detached"})
74+
75+
err := root.Execute()
76+
if err == nil {
77+
t.Fatal("expected error for --status without --all")
78+
}
79+
if !strings.Contains(err.Error(), "--status can only be used with --all") {
80+
t.Fatalf("unexpected error: %v", err)
81+
}
82+
}
83+
6384
func TestDeleteHasExpectedFlags(t *testing.T) {
6485
t.Parallel()
6586

0 commit comments

Comments
 (0)