Skip to content

Commit 95c37d3

Browse files
kaovilaikaovilai
committed
Refactor completion function to use apimachinery's meta helpers and add unit tests for completion logic
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
1 parent 057bff7 commit 95c37d3

2 files changed

Lines changed: 221 additions & 68 deletions

File tree

‎pkg/cmd/cli/completion_functions.go‎

Lines changed: 22 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"time"
2323

2424
"github.com/spf13/cobra"
25+
"k8s.io/apimachinery/pkg/api/meta"
2526
kbclient "sigs.k8s.io/controller-runtime/pkg/client"
2627

2728
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
@@ -31,24 +32,31 @@ import (
3132
// completionFunc is the function signature for cobra's ValidArgsFunction.
3233
type completionFunc = func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective)
3334

34-
// completeNames is a generic helper that builds a completion function for any
35-
// Velero list type. The caller provides a constructor for the list object and a
36-
// callback that extracts resource names from it.
37-
func completeNames[L kbclient.ObjectList](f client.Factory, newList func() L, getNames func(L) []string) completionFunc {
35+
// completeNames builds a completion function for any Velero list type.
36+
// It extracts resource names via apimachinery's meta helpers.
37+
func completeNames(f client.Factory, list kbclient.ObjectList) completionFunc {
3838
return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
3939
kbClient, err := f.KubebuilderClient()
4040
if err != nil {
4141
return nil, cobra.ShellCompDirectiveNoFileComp
4242
}
4343
ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second)
4444
defer cancel()
45-
list := newList()
46-
if err := kbClient.List(ctx, list, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil {
45+
freshList := list.DeepCopyObject().(kbclient.ObjectList)
46+
if err := kbClient.List(ctx, freshList, &kbclient.ListOptions{Namespace: f.Namespace()}); err != nil {
47+
return nil, cobra.ShellCompDirectiveNoFileComp
48+
}
49+
items, err := meta.ExtractList(freshList)
50+
if err != nil {
4751
return nil, cobra.ShellCompDirectiveNoFileComp
4852
}
4953
var filtered []string
50-
for _, name := range getNames(list) {
51-
if strings.HasPrefix(name, toComplete) {
54+
for _, item := range items {
55+
accessor, err := meta.Accessor(item)
56+
if err != nil {
57+
continue
58+
}
59+
if name := accessor.GetName(); strings.HasPrefix(name, toComplete) {
5260
filtered = append(filtered, name)
5361
}
5462
}
@@ -57,79 +65,25 @@ func completeNames[L kbclient.ObjectList](f client.Factory, newList func() L, ge
5765
}
5866

5967
func CompleteBackupNames(f client.Factory) completionFunc {
60-
return completeNames(f,
61-
func() *velerov1api.BackupList { return new(velerov1api.BackupList) },
62-
func(l *velerov1api.BackupList) []string {
63-
names := make([]string, len(l.Items))
64-
for i, b := range l.Items {
65-
names[i] = b.Name
66-
}
67-
return names
68-
},
69-
)
68+
return completeNames(f, &velerov1api.BackupList{})
7069
}
7170

7271
func CompleteRestoreNames(f client.Factory) completionFunc {
73-
return completeNames(f,
74-
func() *velerov1api.RestoreList { return new(velerov1api.RestoreList) },
75-
func(l *velerov1api.RestoreList) []string {
76-
names := make([]string, len(l.Items))
77-
for i, r := range l.Items {
78-
names[i] = r.Name
79-
}
80-
return names
81-
},
82-
)
72+
return completeNames(f, &velerov1api.RestoreList{})
8373
}
8474

8575
func CompleteScheduleNames(f client.Factory) completionFunc {
86-
return completeNames(f,
87-
func() *velerov1api.ScheduleList { return new(velerov1api.ScheduleList) },
88-
func(l *velerov1api.ScheduleList) []string {
89-
names := make([]string, len(l.Items))
90-
for i, s := range l.Items {
91-
names[i] = s.Name
92-
}
93-
return names
94-
},
95-
)
76+
return completeNames(f, &velerov1api.ScheduleList{})
9677
}
9778

9879
func CompleteBackupStorageLocationNames(f client.Factory) completionFunc {
99-
return completeNames(f,
100-
func() *velerov1api.BackupStorageLocationList { return new(velerov1api.BackupStorageLocationList) },
101-
func(l *velerov1api.BackupStorageLocationList) []string {
102-
names := make([]string, len(l.Items))
103-
for i, loc := range l.Items {
104-
names[i] = loc.Name
105-
}
106-
return names
107-
},
108-
)
80+
return completeNames(f, &velerov1api.BackupStorageLocationList{})
10981
}
11082

11183
func CompleteVolumeSnapshotLocationNames(f client.Factory) completionFunc {
112-
return completeNames(f,
113-
func() *velerov1api.VolumeSnapshotLocationList { return new(velerov1api.VolumeSnapshotLocationList) },
114-
func(l *velerov1api.VolumeSnapshotLocationList) []string {
115-
names := make([]string, len(l.Items))
116-
for i, loc := range l.Items {
117-
names[i] = loc.Name
118-
}
119-
return names
120-
},
121-
)
84+
return completeNames(f, &velerov1api.VolumeSnapshotLocationList{})
12285
}
12386

12487
func CompleteBackupRepositoryNames(f client.Factory) completionFunc {
125-
return completeNames(f,
126-
func() *velerov1api.BackupRepositoryList { return new(velerov1api.BackupRepositoryList) },
127-
func(l *velerov1api.BackupRepositoryList) []string {
128-
names := make([]string, len(l.Items))
129-
for i, r := range l.Items {
130-
names[i] = r.Name
131-
}
132-
return names
133-
},
134-
)
88+
return completeNames(f, &velerov1api.BackupRepositoryList{})
13589
}

‎pkg/cmd/cli/completion_functions_test.go‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,199 @@
1+
/*
2+
Copyright The Velero Contributors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package cli
18+
19+
import (
20+
"fmt"
21+
"testing"
22+
23+
"github.com/spf13/cobra"
24+
"github.com/stretchr/testify/assert"
25+
"github.com/stretchr/testify/require"
26+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27+
"k8s.io/apimachinery/pkg/runtime"
28+
kbclient "sigs.k8s.io/controller-runtime/pkg/client"
29+
30+
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
31+
factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks"
32+
velerotest "github.com/vmware-tanzu/velero/pkg/test"
33+
)
34+
35+
// TestCompleteNames exercises the core completeNames helper with various list
36+
// types, prefix filters, and edge cases (empty cluster, no match).
37+
func TestCompleteNames(t *testing.T) {
38+
tests := []struct {
39+
name string
40+
objects []runtime.Object
41+
list kbclient.ObjectList
42+
toComplete string
43+
want []string
44+
}{
45+
{
46+
name: "no resources returns nil",
47+
objects: nil,
48+
list: &velerov1api.BackupList{},
49+
toComplete: "",
50+
want: nil,
51+
},
52+
{
53+
name: "returns all matching names",
54+
objects: []runtime.Object{
55+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "daily", Namespace: "velero"}},
56+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "weekly", Namespace: "velero"}},
57+
},
58+
list: &velerov1api.BackupList{},
59+
toComplete: "",
60+
want: []string{"daily", "weekly"},
61+
},
62+
{
63+
name: "filters by prefix",
64+
objects: []runtime.Object{
65+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "daily", Namespace: "velero"}},
66+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "weekly", Namespace: "velero"}},
67+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "daily-full", Namespace: "velero"}},
68+
},
69+
list: &velerov1api.BackupList{},
70+
toComplete: "dai",
71+
want: []string{"daily", "daily-full"},
72+
},
73+
{
74+
name: "no prefix match returns nil",
75+
objects: []runtime.Object{
76+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "daily", Namespace: "velero"}},
77+
},
78+
list: &velerov1api.BackupList{},
79+
toComplete: "xyz",
80+
want: nil,
81+
},
82+
{
83+
name: "works with RestoreList",
84+
objects: []runtime.Object{
85+
&velerov1api.Restore{ObjectMeta: metav1.ObjectMeta{Name: "restore-1", Namespace: "velero"}},
86+
&velerov1api.Restore{ObjectMeta: metav1.ObjectMeta{Name: "restore-2", Namespace: "velero"}},
87+
},
88+
list: &velerov1api.RestoreList{},
89+
toComplete: "restore-",
90+
want: []string{"restore-1", "restore-2"},
91+
},
92+
{
93+
name: "works with ScheduleList",
94+
objects: []runtime.Object{
95+
&velerov1api.Schedule{ObjectMeta: metav1.ObjectMeta{Name: "nightly", Namespace: "velero"}},
96+
},
97+
list: &velerov1api.ScheduleList{},
98+
toComplete: "",
99+
want: []string{"nightly"},
100+
},
101+
{
102+
name: "works with BackupStorageLocationList",
103+
objects: []runtime.Object{
104+
&velerov1api.BackupStorageLocation{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "velero"}},
105+
&velerov1api.BackupStorageLocation{ObjectMeta: metav1.ObjectMeta{Name: "secondary", Namespace: "velero"}},
106+
},
107+
list: &velerov1api.BackupStorageLocationList{},
108+
toComplete: "s",
109+
want: []string{"secondary"},
110+
},
111+
{
112+
name: "works with VolumeSnapshotLocationList",
113+
objects: []runtime.Object{
114+
&velerov1api.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "aws-snap", Namespace: "velero"}},
115+
},
116+
list: &velerov1api.VolumeSnapshotLocationList{},
117+
toComplete: "",
118+
want: []string{"aws-snap"},
119+
},
120+
{
121+
name: "works with BackupRepositoryList",
122+
objects: []runtime.Object{
123+
&velerov1api.BackupRepository{ObjectMeta: metav1.ObjectMeta{Name: "repo-1", Namespace: "velero"}},
124+
},
125+
list: &velerov1api.BackupRepositoryList{},
126+
toComplete: "",
127+
want: []string{"repo-1"},
128+
},
129+
}
130+
131+
for _, tc := range tests {
132+
t.Run(tc.name, func(t *testing.T) {
133+
kbClient := velerotest.NewFakeControllerRuntimeClient(t, tc.objects...)
134+
135+
f := new(factorymocks.Factory)
136+
f.On("KubebuilderClient").Return(kbClient, nil)
137+
f.On("Namespace").Return("velero")
138+
139+
completionFn := completeNames(f, tc.list)
140+
got, directive := completionFn(&cobra.Command{}, nil, tc.toComplete)
141+
142+
assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive)
143+
assert.Equal(t, tc.want, got)
144+
})
145+
}
146+
}
147+
148+
// TestCompleteNames_KubebuilderClientError verifies that a factory error
149+
// (e.g. no kubeconfig) returns nil completions instead of panicking.
150+
func TestCompleteNames_KubebuilderClientError(t *testing.T) {
151+
f := new(factorymocks.Factory)
152+
f.On("KubebuilderClient").Return(nil, fmt.Errorf("connection refused"))
153+
154+
completionFn := completeNames(f, &velerov1api.BackupList{})
155+
got, directive := completionFn(&cobra.Command{}, nil, "")
156+
157+
assert.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive)
158+
assert.Nil(t, got)
159+
}
160+
161+
// TestCompleteWrappers verifies each exported Complete*Names wrapper returns
162+
// only its own resource type. A single fake client holds one object of every
163+
// type, so each wrapper must filter correctly and not leak other kinds.
164+
func TestCompleteWrappers(t *testing.T) {
165+
objects := []runtime.Object{
166+
&velerov1api.Backup{ObjectMeta: metav1.ObjectMeta{Name: "b1", Namespace: "velero"}},
167+
&velerov1api.Restore{ObjectMeta: metav1.ObjectMeta{Name: "r1", Namespace: "velero"}},
168+
&velerov1api.Schedule{ObjectMeta: metav1.ObjectMeta{Name: "s1", Namespace: "velero"}},
169+
&velerov1api.BackupStorageLocation{ObjectMeta: metav1.ObjectMeta{Name: "bsl1", Namespace: "velero"}},
170+
&velerov1api.VolumeSnapshotLocation{ObjectMeta: metav1.ObjectMeta{Name: "vsl1", Namespace: "velero"}},
171+
&velerov1api.BackupRepository{ObjectMeta: metav1.ObjectMeta{Name: "br1", Namespace: "velero"}},
172+
}
173+
kbClient := velerotest.NewFakeControllerRuntimeClient(t, objects...)
174+
175+
f := new(factorymocks.Factory)
176+
f.On("KubebuilderClient").Return(kbClient, nil)
177+
f.On("Namespace").Return("velero")
178+
179+
tests := []struct {
180+
name string
181+
fn completionFunc
182+
expected []string
183+
}{
184+
{"CompleteBackupNames", CompleteBackupNames(f), []string{"b1"}},
185+
{"CompleteRestoreNames", CompleteRestoreNames(f), []string{"r1"}},
186+
{"CompleteScheduleNames", CompleteScheduleNames(f), []string{"s1"}},
187+
{"CompleteBackupStorageLocationNames", CompleteBackupStorageLocationNames(f), []string{"bsl1"}},
188+
{"CompleteVolumeSnapshotLocationNames", CompleteVolumeSnapshotLocationNames(f), []string{"vsl1"}},
189+
{"CompleteBackupRepositoryNames", CompleteBackupRepositoryNames(f), []string{"br1"}},
190+
}
191+
192+
for _, tc := range tests {
193+
t.Run(tc.name, func(t *testing.T) {
194+
got, directive := tc.fn(&cobra.Command{}, nil, "")
195+
require.Equal(t, cobra.ShellCompDirectiveNoFileComp, directive)
196+
assert.Equal(t, tc.expected, got)
197+
})
198+
}
199+
}

0 commit comments

Comments
 (0)