Add dynamic resource autocompletion to Velero CLI#9720
Add dynamic resource autocompletion to Velero CLI#9720Joeavaikath wants to merge 5 commits intovelero-io:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9720 +/- ##
==========================================
+ Coverage 60.97% 61.00% +0.02%
==========================================
Files 384 385 +1
Lines 36609 36667 +58
==========================================
+ Hits 22322 22368 +46
- Misses 12678 12688 +10
- Partials 1609 1611 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Shell completion is a standard UX feature in Kubernetes CLI tooling. | ||
| Tools like `kubectl`, `oc`, and `helm` all provide dynamic completions that query the cluster to suggest resource names. | ||
| Velero's `velero completion` command generates static completion scripts using cobra's v1 API (`GenBashCompletion`), which only completes command and flag names. | ||
| Cobra v1.8.1 (Velero's current version) supports dynamic completion via `ValidArgsFunction` on `cobra.Command` and `RegisterFlagCompletionFunc` for flag values, but Velero does not use either. |
There was a problem hiding this comment.
| Cobra v1.8.1 (Velero's current version) supports dynamic completion via `ValidArgsFunction` on `cobra.Command` and `RegisterFlagCompletionFunc` for flag values, but Velero does not use either. | |
| Cobra v1.8.1 (Velero's current import) supports dynamic completion via `ValidArgsFunction` on `cobra.Command` and `RegisterFlagCompletionFunc` for flag values, but Velero does not use either. |
| return nil, cobra.ShellCompDirectiveNoFileComp | ||
| } | ||
| backups := new(velerov1api.BackupList) | ||
| if err := kbClient.List(context.TODO(), backups, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil { |
There was a problem hiding this comment.
These should be context.WithTimeout or deriving a context from the command's context via cmd.Context() which cobra populates and which typically gets canceled on SIGINT.
There was a problem hiding this comment.
fyi: optimized the generics further. See how each wrapper return is a single line.
There was a problem hiding this comment.
I guess this is using interface func more than generics.. but alas.. less dupe :)
c3c92a1 to
95c37d3
Compare
Add ValidArgsFunction for 20 commands that take resource names as positional args (backup, restore, schedule, BSL, VSL, repo) and RegisterFlagCompletionFunc for 5 flags that reference resources (--from-backup, --from-schedule, --storage-location, etc). Completion functions query the cluster via KubebuilderClient and fail silently if unreachable, matching oc behavior. Switch bash completion from v1 to v2 (GenBashCompletionV2) which is required for ValidArgsFunction support. Signed-off-by: Joseph <jvaikath@redhat.com>
Proposes adding ValidArgsFunction and RegisterFlagCompletionFunc to all Velero CLI commands that accept existing resource names, covering 20 commands and 5 flags across 6 resource types. Signed-off-by: Joseph <jvaikath@redhat.com>
…dd unit tests for completion logic Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
95c37d3 to
c6a8680
Compare
There was a problem hiding this comment.
Pull request overview
Adds dynamic shell autocompletion for Velero CLI resource names by querying the Kubernetes API at completion time (instead of relying solely on static completion scripts), and updates bash completion generation to Cobra’s v2 generator so custom completion callbacks are invoked.
Changes:
- Introduces reusable completion functions that list Velero resources (backups, restores, schedules, BSL/VSL, repositories) and filter by the typed prefix.
- Wires these functions into relevant commands via
ValidArgsFunctionand into selected flags viaRegisterFlagCompletionFunc. - Switches bash completion generation to
GenBashCompletionV2, and adds tests + a design document + changelog entry.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/cli/snapshotlocation/set.go | Adds ValidArgsFunction for VolumeSnapshotLocation name completion. |
| pkg/cmd/cli/snapshotlocation/get.go | Adds ValidArgsFunction for VolumeSnapshotLocation name completion. |
| pkg/cmd/cli/schedule/unpause.go | Adds ValidArgsFunction for Schedule name completion. |
| pkg/cmd/cli/schedule/pause.go | Adds ValidArgsFunction for Schedule name completion. |
| pkg/cmd/cli/schedule/get.go | Adds ValidArgsFunction for Schedule name completion. |
| pkg/cmd/cli/schedule/describe.go | Adds ValidArgsFunction for Schedule name completion. |
| pkg/cmd/cli/schedule/delete.go | Adds ValidArgsFunction for Schedule name completion. |
| pkg/cmd/cli/restore/logs.go | Adds ValidArgsFunction for Restore name completion. |
| pkg/cmd/cli/restore/get.go | Adds ValidArgsFunction for Restore name completion. |
| pkg/cmd/cli/restore/describe.go | Adds ValidArgsFunction for Restore name completion. |
| pkg/cmd/cli/restore/delete.go | Adds ValidArgsFunction for Restore name completion. |
| pkg/cmd/cli/restore/create.go | Adds flag completion for --from-backup and --from-schedule. |
| pkg/cmd/cli/repo/get.go | Adds ValidArgsFunction for BackupRepository name completion. |
| pkg/cmd/cli/completion_functions_test.go | Adds unit tests for the completion helper and wrappers. |
| pkg/cmd/cli/completion_functions.go | New shared implementation for dynamic resource-name completion via controller-runtime client. |
| pkg/cmd/cli/completion/completion.go | Switches bash completion generation to GenBashCompletionV2. |
| pkg/cmd/cli/backuplocation/set.go | Adds ValidArgsFunction for BackupStorageLocation name completion. |
| pkg/cmd/cli/backuplocation/get.go | Adds ValidArgsFunction for BackupStorageLocation name completion. |
| pkg/cmd/cli/backuplocation/delete.go | Adds ValidArgsFunction for BackupStorageLocation name completion. |
| pkg/cmd/cli/backup/logs.go | Adds ValidArgsFunction for Backup name completion. |
| pkg/cmd/cli/backup/get.go | Adds ValidArgsFunction for Backup name completion. |
| pkg/cmd/cli/backup/download.go | Adds ValidArgsFunction for Backup name completion. |
| pkg/cmd/cli/backup/describe.go | Adds ValidArgsFunction for Backup name completion. |
| pkg/cmd/cli/backup/delete.go | Adds ValidArgsFunction for Backup name completion. |
| pkg/cmd/cli/backup/create.go | Adds flag completion for --from-schedule, --storage-location, --volume-snapshot-locations. |
| design/cli-dynamic-resource-completion_design.md | Documents the approach, scope, and Cobra generator implications. |
| changelogs/unreleased/9720-Joeavaikath | Adds an unreleased changelog entry for the feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) | ||
| defer cancel() | ||
| freshList := list.DeepCopyObject().(kbclient.ObjectList) |
There was a problem hiding this comment.
The completion request context is always derived from context.Background(). It would be better to derive from cmd.Context() (when available) so shell completion can be cancelled promptly, and to avoid holding onto work after the caller exits. Also, the type assertion on list.DeepCopyObject() can panic if a non-ObjectList slips in; consider using an ok check and failing silently (nil completions) instead of panicking.
| ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) | |
| defer cancel() | |
| freshList := list.DeepCopyObject().(kbclient.ObjectList) | |
| parentCtx := context.Background() | |
| if cmd != nil && cmd.Context() != nil { | |
| parentCtx = cmd.Context() | |
| } | |
| ctx, cancel := context.WithTimeout(parentCtx, 3*time.Second) | |
| defer cancel() | |
| freshObject := list.DeepCopyObject() | |
| freshList, ok := freshObject.(kbclient.ObjectList) | |
| if !ok { | |
| return nil, cobra.ShellCompDirectiveNoFileComp | |
| } |
| var filtered []string | ||
| for _, item := range items { | ||
| accessor, err := meta.Accessor(item) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| if name := accessor.GetName(); strings.HasPrefix(name, toComplete) { | ||
| filtered = append(filtered, name) | ||
| } | ||
| } | ||
| return filtered, cobra.ShellCompDirectiveNoFileComp | ||
| } |
There was a problem hiding this comment.
The returned completion names are currently in whatever order the API server/client returns list items, which is not guaranteed to be stable. Sorting the filtered slice before returning will make completions deterministic (and typically alphabetical), improving UX and preventing order-dependent tests from becoming flaky.
| got, directive := completionFn(&cobra.Command{}, nil, tc.toComplete) | ||
|
|
||
| assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive) | ||
| assert.Equal(t, tc.want, got) |
There was a problem hiding this comment.
This test uses assert.Equal on slices, which makes it order-dependent. Since Kubernetes list ordering isn't guaranteed (and the fake client implementation could change), consider sorting got/want before comparing or using assert.ElementsMatch to avoid flaky tests.
| assert.Equal(t, tc.want, got) | |
| assert.ElementsMatch(t, tc.want, got) |
| _ = c.RegisterFlagCompletionFunc("from-schedule", cli.CompleteScheduleNames(f)) | ||
| _ = c.RegisterFlagCompletionFunc("storage-location", cli.CompleteBackupStorageLocationNames(f)) | ||
| _ = c.RegisterFlagCompletionFunc("volume-snapshot-locations", cli.CompleteVolumeSnapshotLocationNames(f)) | ||
|
|
There was a problem hiding this comment.
volume-snapshot-locations is a StringSlice flag (comma-separated in a single shell word). The current completion function only matches prefixes from the start of toComplete, so completing a second/third value like --volume-snapshot-locations loc1,lo<TAB> will not suggest anything. Consider registering a small wrapper completion function for this flag that splits toComplete on commas, completes only the last segment, and re-attaches the already-typed prefix (optionally also filtering out already-selected locations).
Summary
Adds dynamic shell completion for Velero CLI commands, replacing static
completion with live resource lookups from the cluster.
backup, restore, schedule, BSL, VSL, and repository names
ValidArgsFunctionon get/describe/delete/logs commandsand
RegisterFlagCompletionFuncon create command flags(e.g.
--from-schedule,--storage-location,--from-backup)GenBashCompletionV2)to support custom completion functions
Does your change fix a particular issue?
Please indicate you've done the following:
make new-changelog) or comment/kind changelog-not-requiredon this PR.site/content/docs/main.