From 3285ad1e768092947e30e66155a44d3599502811 Mon Sep 17 00:00:00 2001 From: Sunil Arora Date: Tue, 15 Jan 2019 15:30:18 -0800 Subject: [PATCH] :warning: Convert client.List to use functional options Replaces: List(ctx, ListOptions, list) with: List(ctx, list, ...option) This matches the upcoming functional options signature of client.Delete. To use the previous builder options, use the UseListOptions functional option: lo := &client.ListOptions{} client.List(ctx, list, client.UseListOptions( lo.InNamespace(ns).MatchingLabels(labels) )) Ported https://github.com/kubernetes-sigs/controller-runtime/pull/106 PR --- pkg/builder/example_test.go | 3 +- pkg/cache/cache_test.go | 53 ++++++-------- pkg/cache/informer_cache.go | 4 +- pkg/cache/informertest/fake_cache.go | 2 +- pkg/cache/internal/cache_reader.go | 19 ++--- pkg/client/client.go | 6 +- pkg/client/client_test.go | 69 ++++++++++--------- pkg/client/example_test.go | 6 +- pkg/client/fake/client.go | 42 ++++------- pkg/client/fake/client_test.go | 8 +-- pkg/client/interfaces.go | 61 ++++++++++++---- pkg/client/split.go | 6 +- pkg/client/typed_client.go | 12 ++-- pkg/client/unstructured_client.go | 12 ++-- .../internal/cert/writer/secret_test.go | 20 ++---- 15 files changed, 159 insertions(+), 164 deletions(-) diff --git a/pkg/builder/example_test.go b/pkg/builder/example_test.go index 00eafb0baf..7b3c8e308e 100644 --- a/pkg/builder/example_test.go +++ b/pkg/builder/example_test.go @@ -86,7 +86,8 @@ func (a *ReplicaSetReconciler) Reconcile(req reconcile.Request) (reconcile.Resul // List the Pods matching the PodTemplate Labels pods := &corev1.PodList{} - err = a.List(context.TODO(), client.InNamespace(req.Namespace).MatchingLabels(rs.Spec.Template.Labels), pods) + err = a.List(context.TODO(), pods, client.InNamespace(req.Namespace), + client.MatchingLabels(rs.Spec.Template.Labels)) if err != nil { return reconcile.Result{}, err } diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 9eeab54f7a..08d98860cb 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -124,7 +124,7 @@ var _ = Describe("Informer Cache", func() { It("should be able to list objects that haven't been watched previously", func() { By("listing all services in the cluster") listObj := &kcorev1.ServiceList{} - Expect(informerCache.List(context.Background(), nil, listObj)).To(Succeed()) + Expect(informerCache.List(context.Background(), listObj)).To(Succeed()) By("verifying that the returned list contains the Kubernetes service") // NB: kubernetes default service is automatically created in testenv. @@ -154,10 +154,9 @@ var _ = Describe("Informer Cache", func() { By("listing pods with a particular label") // NB: each pod has a "test-label": out := kcorev1.PodList{} - lo := &client.ListOptions{} - lo.InNamespace(testNamespaceTwo) - lo.MatchingLabels(map[string]string{"test-label": "test-pod-2"}) - Expect(informerCache.List(context.Background(), lo, &out)).To(Succeed()) + Expect(informerCache.List(context.Background(), &out, + client.InNamespace(testNamespaceTwo), + client.MatchingLabels(map[string]string{"test-label": "test-pod-2"}))).To(Succeed()) By("verifying the returned pods have the correct label") Expect(out.Items).NotTo(BeEmpty()) @@ -174,9 +173,7 @@ var _ = Describe("Informer Cache", func() { // NB: each pod has a "test-label": out := kcorev1.PodList{} labels := map[string]string{"test-label": "test-pod-2"} - lo := &client.ListOptions{} - lo.MatchingLabels(labels) - Expect(informerCache.List(context.Background(), lo, &out)).To(Succeed()) + Expect(informerCache.List(context.Background(), &out, client.MatchingLabels(labels))).To(Succeed()) By("verifying multiple pods with the same label in different namespaces are returned") Expect(out.Items).NotTo(BeEmpty()) @@ -191,9 +188,8 @@ var _ = Describe("Informer Cache", func() { It("should be able to list objects by namespace", func() { By("listing pods in test-namespace-1") listObj := &kcorev1.PodList{} - lo := &client.ListOptions{} - lo.InNamespace(testNamespaceOne) - Expect(informerCache.List(context.Background(), lo, listObj)).To(Succeed()) + Expect(informerCache.List(context.Background(), listObj, + client.InNamespace(testNamespaceOne))).To(Succeed()) By("verifying that the returned pods are in test-namespace-1") Expect(listObj.Items).NotTo(BeEmpty()) @@ -216,7 +212,7 @@ var _ = Describe("Informer Cache", func() { By("listing pods in all namespaces") out := &kcorev1.PodList{} - Expect(namespacedCache.List(context.Background(), nil, out)).To(Succeed()) + Expect(namespacedCache.List(context.Background(), out)).To(Succeed()) By("verifying the returned pod is from the watched namespace") Expect(out.Items).NotTo(BeEmpty()) @@ -225,7 +221,7 @@ var _ = Describe("Informer Cache", func() { By("listing all namespaces - should still be able to get a cluster-scoped resource") namespaceList := &kcorev1.NamespaceList{} - Expect(namespacedCache.List(context.Background(), nil, namespaceList)).To(Succeed()) + Expect(namespacedCache.List(context.Background(), namespaceList)).To(Succeed()) By("verifying the namespace list is not empty") Expect(namespaceList.Items).NotTo(BeEmpty()) @@ -267,7 +263,7 @@ var _ = Describe("Informer Cache", func() { Version: "v1", Kind: "ServiceList", }) - err := informerCache.List(context.Background(), nil, listObj) + err := informerCache.List(context.Background(), listObj) Expect(err).To(Succeed()) By("verifying that the returned list contains the Kubernetes service") @@ -308,10 +304,9 @@ var _ = Describe("Informer Cache", func() { Version: "v1", Kind: "PodList", }) - lo := &client.ListOptions{} - lo.InNamespace(testNamespaceTwo) - lo.MatchingLabels(map[string]string{"test-label": "test-pod-2"}) - err := informerCache.List(context.Background(), lo, &out) + err := informerCache.List(context.Background(), &out, + client.InNamespace(testNamespaceTwo), + client.MatchingLabels(map[string]string{"test-label": "test-pod-2"})) Expect(err).To(Succeed()) By("verifying the returned pods have the correct label") @@ -334,9 +329,7 @@ var _ = Describe("Informer Cache", func() { Kind: "PodList", }) labels := map[string]string{"test-label": "test-pod-2"} - lo := &client.ListOptions{} - lo.MatchingLabels(labels) - err := informerCache.List(context.Background(), lo, &out) + err := informerCache.List(context.Background(), &out, client.MatchingLabels(labels)) Expect(err).To(Succeed()) By("verifying multiple pods with the same label in different namespaces are returned") @@ -357,9 +350,7 @@ var _ = Describe("Informer Cache", func() { Version: "v1", Kind: "PodList", }) - lo := &client.ListOptions{} - lo.InNamespace(testNamespaceOne) - err := informerCache.List(context.Background(), lo, listObj) + err := informerCache.List(context.Background(), listObj, client.InNamespace(testNamespaceOne)) Expect(err).To(Succeed()) By("verifying that the returned pods are in test-namespace-1") @@ -388,7 +379,7 @@ var _ = Describe("Informer Cache", func() { Version: "v1", Kind: "PodList", }) - Expect(namespacedCache.List(context.Background(), nil, out)).To(Succeed()) + Expect(namespacedCache.List(context.Background(), out)).To(Succeed()) By("verifying the returned pod is from the watched namespace") Expect(out.Items).NotTo(BeEmpty()) @@ -402,7 +393,7 @@ var _ = Describe("Informer Cache", func() { Version: "v1", Kind: "NamespaceList", }) - Expect(namespacedCache.List(context.Background(), nil, namespaceList)).To(Succeed()) + Expect(namespacedCache.List(context.Background(), namespaceList)).To(Succeed()) By("verifying the namespace list is not empty") Expect(namespaceList.Items).NotTo(BeEmpty()) @@ -552,9 +543,8 @@ var _ = Describe("Informer Cache", func() { By("listing Pods with restartPolicyOnFailure") listObj := &kcorev1.PodList{} - lo := &client.ListOptions{} - lo.MatchingField("spec.restartPolicy", "OnFailure") - Expect(informer.List(context.Background(), lo, listObj)).To(Succeed()) + Expect(informer.List(context.Background(), listObj, + client.MatchingField("spec.restartPolicy", "OnFailure"))).To(Succeed()) By("verifying that the returned pods have correct restart policy") Expect(listObj.Items).NotTo(BeEmpty()) @@ -647,9 +637,8 @@ var _ = Describe("Informer Cache", func() { Version: "v1", Kind: "PodList", }) - lo := &client.ListOptions{} - lo.MatchingField("spec.restartPolicy", "OnFailure") - err = informer.List(context.Background(), lo, listObj) + err = informer.List(context.Background(), listObj, + client.MatchingField("spec.restartPolicy", "OnFailure")) Expect(err).To(Succeed()) By("verifying that the returned pods have correct restart policy") diff --git a/pkg/cache/informer_cache.go b/pkg/cache/informer_cache.go index eedfcc6cf0..c33914ee19 100644 --- a/pkg/cache/informer_cache.go +++ b/pkg/cache/informer_cache.go @@ -58,7 +58,7 @@ func (ip *informerCache) Get(ctx context.Context, key client.ObjectKey, out runt } // List implements Reader -func (ip *informerCache) List(ctx context.Context, opts *client.ListOptions, out runtime.Object) error { +func (ip *informerCache) List(ctx context.Context, out runtime.Object, opts ...client.ListOptionFunc) error { gvk, err := apiutil.GVKForObject(out, ip.Scheme) if err != nil { return err @@ -95,7 +95,7 @@ func (ip *informerCache) List(ctx context.Context, opts *client.ListOptions, out return err } - return cache.Reader.List(ctx, opts, out) + return cache.Reader.List(ctx, out, opts...) } // GetInformerForKind returns the informer for the GroupVersionKind diff --git a/pkg/cache/informertest/fake_cache.go b/pkg/cache/informertest/fake_cache.go index 87e6aeedcb..2f311cf729 100644 --- a/pkg/cache/informertest/fake_cache.go +++ b/pkg/cache/informertest/fake_cache.go @@ -136,6 +136,6 @@ func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj runti } // List implements Cache -func (c *FakeInformers) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { +func (c *FakeInformers) List(ctx context.Context, list runtime.Object, opts ...client.ListOptionFunc) error { return nil } diff --git a/pkg/cache/internal/cache_reader.go b/pkg/cache/internal/cache_reader.go index 62a16eb1ae..fe6cfc02b5 100644 --- a/pkg/cache/internal/cache_reader.go +++ b/pkg/cache/internal/cache_reader.go @@ -87,23 +87,26 @@ func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out runtime.O } // List lists items out of the indexer and writes them to out -func (c *CacheReader) List(_ context.Context, opts *client.ListOptions, out runtime.Object) error { +func (c *CacheReader) List(_ context.Context, out runtime.Object, opts ...client.ListOptionFunc) error { var objs []interface{} var err error - if opts != nil && opts.FieldSelector != nil { + listOpts := client.ListOptions{} + listOpts.ApplyOptions(opts) + + if listOpts.FieldSelector != nil { // TODO(directxman12): support more complicated field selectors by // combining multiple indicies, GetIndexers, etc - field, val, requiresExact := requiresExactMatch(opts.FieldSelector) + field, val, requiresExact := requiresExactMatch(listOpts.FieldSelector) if !requiresExact { return fmt.Errorf("non-exact field matches are not supported by the cache") } // list all objects by the field selector. If this is namespaced and we have one, ask for the // namespaced index key. Otherwise, ask for the non-namespaced variant by using the fake "all namespaces" // namespace. - objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToNamespacedKey(opts.Namespace, val)) - } else if opts != nil && opts.Namespace != "" { - objs, err = c.indexer.ByIndex(cache.NamespaceIndex, opts.Namespace) + objs, err = c.indexer.ByIndex(FieldIndexName(field), KeyToNamespacedKey(listOpts.Namespace, val)) + } else if listOpts.Namespace != "" { + objs, err = c.indexer.ByIndex(cache.NamespaceIndex, listOpts.Namespace) } else { objs = c.indexer.List() } @@ -111,8 +114,8 @@ func (c *CacheReader) List(_ context.Context, opts *client.ListOptions, out runt return err } var labelSel labels.Selector - if opts != nil && opts.LabelSelector != nil { - labelSel = opts.LabelSelector + if listOpts.LabelSelector != nil { + labelSel = listOpts.LabelSelector } outItems, err := c.getListItems(objs, labelSel) diff --git a/pkg/client/client.go b/pkg/client/client.go index 05b9eba2b4..0650da0f35 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -131,12 +131,12 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj runtime.Object) err } // List implements client.Client -func (c *client) List(ctx context.Context, opts *ListOptions, obj runtime.Object) error { +func (c *client) List(ctx context.Context, obj runtime.Object, opts ...ListOptionFunc) error { _, ok := obj.(*unstructured.UnstructuredList) if ok { - return c.unstructuredClient.List(ctx, opts, obj) + return c.unstructuredClient.List(ctx, obj, opts...) } - return c.typedClient.List(ctx, opts, obj) + return c.typedClient.List(ctx, obj, opts...) } // Status implements client.StatusClient diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 3ab5c12de7..c8d23e1915 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -1096,7 +1096,7 @@ var _ = Describe("Client", func() { By("listing all objects of that type in the cluster") deps := &appsv1.DeploymentList{} - Expect(cl.List(context.Background(), nil, deps)).NotTo(HaveOccurred()) + Expect(cl.List(context.Background(), deps)).NotTo(HaveOccurred()) Expect(deps.Items).NotTo(BeEmpty()) hasDep := false @@ -1126,7 +1126,7 @@ var _ = Describe("Client", func() { Kind: "DeploymentList", Version: "v1", }) - err = cl.List(context.Background(), nil, deps) + err = cl.List(context.Background(), deps) Expect(err).NotTo(HaveOccurred()) Expect(deps.Items).NotTo(BeEmpty()) @@ -1149,7 +1149,7 @@ var _ = Describe("Client", func() { By("listing all Deployments in the cluster") deps := &appsv1.DeploymentList{} - Expect(cl.List(context.Background(), nil, deps)).NotTo(HaveOccurred()) + Expect(cl.List(context.Background(), deps)).NotTo(HaveOccurred()) By("validating no Deployments are returned") Expect(deps.Items).To(BeEmpty()) @@ -1205,9 +1205,7 @@ var _ = Describe("Client", func() { By("listing all Deployments with label app=backend") deps := &appsv1.DeploymentList{} labels := map[string]string{"app": "backend"} - lo := &client.ListOptions{} - lo.MatchingLabels(labels) - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, client.MatchingLabels(labels)) Expect(err).NotTo(HaveOccurred()) By("only the Deployment with the backend label is returned") @@ -1266,9 +1264,7 @@ var _ = Describe("Client", func() { By("listing all Deployments in test-namespace-1") deps := &appsv1.DeploymentList{} - lo := &client.ListOptions{} - lo.InNamespace("test-namespace-1") - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, client.InNamespace("test-namespace-1")) Expect(err).NotTo(HaveOccurred()) By("only the Deployment in test-namespace-1 is returned") @@ -1323,9 +1319,8 @@ var _ = Describe("Client", func() { By("listing all Deployments with field metadata.name=deployment-backend") deps := &appsv1.DeploymentList{} - lo := &client.ListOptions{} - lo.MatchingField("metadata.name", "deployment-backend") - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, + client.MatchingField("metadata.name", "deployment-backend")) Expect(err).NotTo(HaveOccurred()) By("only the Deployment with the backend field is returned") @@ -1413,10 +1408,10 @@ var _ = Describe("Client", func() { By("listing all Deployments in test-namespace-3 with label app=frontend") deps := &appsv1.DeploymentList{} labels := map[string]string{"app": "frontend"} - lo := &client.ListOptions{} - lo.InNamespace("test-namespace-3") - lo.MatchingLabels(labels) - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, + client.InNamespace("test-namespace-3"), + client.MatchingLabels(labels), + ) Expect(err).NotTo(HaveOccurred()) By("only the Deployment in test-namespace-3 with label app=frontend is returned") @@ -1464,7 +1459,7 @@ var _ = Describe("Client", func() { Kind: "DeploymentList", Version: "v1", }) - err = cl.List(context.Background(), nil, deps) + err = cl.List(context.Background(), deps) Expect(err).NotTo(HaveOccurred()) Expect(deps.Items).NotTo(BeEmpty()) @@ -1490,7 +1485,7 @@ var _ = Describe("Client", func() { Kind: "DeploymentList", Version: "v1", }) - Expect(cl.List(context.Background(), nil, deps)).NotTo(HaveOccurred()) + Expect(cl.List(context.Background(), deps)).NotTo(HaveOccurred()) By("validating no Deployments are returned") Expect(deps.Items).To(BeEmpty()) @@ -1547,9 +1542,7 @@ var _ = Describe("Client", func() { Kind: "DeploymentList", Version: "v1", }) - lo := &client.ListOptions{} - lo.InNamespace("test-namespace-5") - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, client.InNamespace("test-namespace-5")) Expect(err).NotTo(HaveOccurred()) By("only the Deployment in test-namespace-5 is returned") @@ -1609,9 +1602,8 @@ var _ = Describe("Client", func() { Kind: "DeploymentList", Version: "v1", }) - lo := &client.ListOptions{} - lo.MatchingField("metadata.name", "deployment-backend") - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, + client.MatchingField("metadata.name", "deployment-backend")) Expect(err).NotTo(HaveOccurred()) By("only the Deployment with the backend field is returned") @@ -1704,10 +1696,8 @@ var _ = Describe("Client", func() { Version: "v1", }) labels := map[string]string{"app": "frontend"} - lo := &client.ListOptions{} - lo.InNamespace("test-namespace-7") - lo.MatchingLabels(labels) - err = cl.List(context.Background(), lo, deps) + err = cl.List(context.Background(), deps, + client.InNamespace("test-namespace-7"), client.MatchingLabels(labels)) Expect(err).NotTo(HaveOccurred()) By("only the Deployment in test-namespace-7 with label app=frontend is returned") @@ -1828,19 +1818,30 @@ var _ = Describe("Client", func() { It("should be created from MatchingLabels", func() { labels := map[string]string{"foo": "bar"} - lo := client.MatchingLabels(labels) + lo := &client.ListOptions{} + client.MatchingLabels(labels)(lo) Expect(lo).NotTo(BeNil()) Expect(lo.LabelSelector.String()).To(Equal("foo=bar")) }) It("should be created from MatchingField", func() { - lo := client.MatchingField("field1", "bar") + lo := &client.ListOptions{} + client.MatchingField("field1", "bar")(lo) Expect(lo).NotTo(BeNil()) Expect(lo.FieldSelector.String()).To(Equal("field1=bar")) }) It("should be created from InNamespace", func() { - lo := client.InNamespace("test") + lo := &client.ListOptions{} + client.InNamespace("test")(lo) + Expect(lo).NotTo(BeNil()) + Expect(lo.Namespace).To(Equal("test")) + }) + + It("should allow pre-built ListOptions", func() { + lo := &client.ListOptions{} + newLo := &client.ListOptions{} + client.UseListOptions(newLo.InNamespace("test"))(lo) Expect(lo).NotTo(BeNil()) Expect(lo.Namespace).To(Equal("test")) }) @@ -1885,7 +1886,7 @@ var _ = Describe("DelegatingReader", func() { ClientReader: clientReader, } var actual appsv1.DeploymentList - dReader.List(context.Background(), nil, &actual) + dReader.List(context.Background(), &actual) Expect(1).To(Equal(cachedReader.Called)) Expect(0).To(Equal(clientReader.Called)) @@ -1899,7 +1900,7 @@ var _ = Describe("DelegatingReader", func() { } var actual unstructured.UnstructuredList - dReader.List(context.Background(), nil, &actual) + dReader.List(context.Background(), &actual) Expect(0).To(Equal(cachedReader.Called)) Expect(1).To(Equal(clientReader.Called)) @@ -1916,7 +1917,7 @@ func (f *fakeReader) Get(ctx context.Context, key client.ObjectKey, obj runtime. return nil } -func (f *fakeReader) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { +func (f *fakeReader) List(ctx context.Context, list runtime.Object, opts ...client.ListOptionFunc) error { f.Called = f.Called + 1 return nil } diff --git a/pkg/client/example_test.go b/pkg/client/example_test.go index 46b5971ace..8059812bbf 100644 --- a/pkg/client/example_test.go +++ b/pkg/client/example_test.go @@ -42,7 +42,7 @@ func ExampleNew() { podList := &corev1.PodList{} - err = cl.List(context.Background(), client.InNamespace("default"), podList) + err = cl.List(context.Background(), podList, client.InNamespace("default")) if err != nil { fmt.Printf("failed to list pods in namespace default: %v\n", err) os.Exit(1) @@ -132,7 +132,7 @@ func ExampleClient_list() { // Using a typed object. pod := &corev1.PodList{} // c is a created client. - _ = c.List(context.Background(), nil, pod) + _ = c.List(context.Background(), pod) // Using a unstructured object. u := &unstructured.UnstructuredList{} @@ -141,7 +141,7 @@ func ExampleClient_list() { Kind: "DeploymentList", Version: "v1", }) - _ = c.List(context.Background(), nil, u) + _ = c.List(context.Background(), u) } // This example shows how to use the client with typed and unstrucurted objects to update objects. diff --git a/pkg/client/fake/client.go b/pkg/client/fake/client.go index ee5e537b93..3239f6f3ea 100644 --- a/pkg/client/fake/client.go +++ b/pkg/client/fake/client.go @@ -88,19 +88,23 @@ func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj runtime. return err } -func (c *fakeClient) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error { - gvk, err := getGVKFromList(list, c.scheme) +func (c *fakeClient) List(ctx context.Context, obj runtime.Object, opts ...client.ListOptionFunc) error { + gvk, err := apiutil.GVKForObject(obj, scheme.Scheme) if err != nil { - // The old fake client required GVK info in Raw.TypeMeta, so check there - // before giving up - if opts.Raw == nil || opts.Raw.TypeMeta.APIVersion == "" || opts.Raw.TypeMeta.Kind == "" { - return err - } - gvk = opts.Raw.TypeMeta.GroupVersionKind() + return err + } + + if !strings.HasSuffix(gvk.Kind, "List") { + return fmt.Errorf("non-list type %T (kind %q) passed as output", obj, gvk) } + // we need the non-list GVK, so chop off the "List" from the end of the kind + gvk.Kind = gvk.Kind[:len(gvk.Kind)-4] + + listOpts := client.ListOptions{} + listOpts.ApplyOptions(opts) gvr, _ := meta.UnsafeGuessKindToResource(gvk) - o, err := c.tracker.List(gvr, gvk, opts.Namespace) + o, err := c.tracker.List(gvr, gvk, listOpts.Namespace) if err != nil { return err } @@ -109,7 +113,7 @@ func (c *fakeClient) List(ctx context.Context, opts *client.ListOptions, list ru return err } decoder := scheme.Codecs.UniversalDecoder() - _, _, err = decoder.Decode(j, nil, list) + _, _, err = decoder.Decode(j, nil, obj) return err } @@ -163,24 +167,6 @@ func getGVRFromObject(obj runtime.Object, scheme *runtime.Scheme) (schema.GroupV return gvr, nil } -func getGVKFromList(list runtime.Object, scheme *runtime.Scheme) (schema.GroupVersionKind, error) { - gvk, err := apiutil.GVKForObject(list, scheme) - if err != nil { - return schema.GroupVersionKind{}, err - } - - if gvk.Kind == "List" { - return schema.GroupVersionKind{}, fmt.Errorf("cannot derive GVK for generic List type %T (kind %q)", list, gvk) - } - - if !strings.HasSuffix(gvk.Kind, "List") { - return schema.GroupVersionKind{}, fmt.Errorf("non-list type %T (kind %q) passed as output", list, gvk) - } - // we need the non-list GVK, so chop off the "List" from the end of the kind - gvk.Kind = gvk.Kind[:len(gvk.Kind)-4] - return gvk, nil -} - type fakeStatusWriter struct { client *fakeClient } diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index ac05f9302a..d193ebe772 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -67,9 +67,7 @@ var _ = Describe("Fake client", func() { It("should be able to List", func() { By("Listing all deployments in a namespace") list := &appsv1.DeploymentList{} - err := cl.List(nil, &client.ListOptions{ - Namespace: "ns1", - }, list) + err := cl.List(nil, list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(1)) Expect(list.Items).To(ConsistOf(*dep)) @@ -129,9 +127,7 @@ var _ = Describe("Fake client", func() { By("Listing all deployments in the namespace") list := &appsv1.DeploymentList{} - err = cl.List(nil, &client.ListOptions{ - Namespace: "ns1", - }, list) + err = cl.List(nil, list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(0)) }) diff --git a/pkg/client/interfaces.go b/pkg/client/interfaces.go index 36d0fce620..087b5eb43b 100644 --- a/pkg/client/interfaces.go +++ b/pkg/client/interfaces.go @@ -51,7 +51,7 @@ type Reader interface { // List retrieves list of objects for a given namespace and list options. On a // successful call, Items field in the list will be populated with the // result returned from the server. - List(ctx context.Context, opts *ListOptions, list runtime.Object) error + List(ctx context.Context, list runtime.Object, opts ...ListOptionFunc) error } // Writer knows how to create, delete, and update Kubernetes objects. @@ -185,7 +185,7 @@ func PropagationPolicy(p metav1.DeletionPropagation) DeleteOptionFunc { } } -// ListOptions contains options for limitting or filtering results. +// ListOptions contains options for limiting or filtering results. // It's generally a subset of metav1.ListOptions, with support for // pre-parsed selectors (since generally, selectors will be executed // against the cache). @@ -248,6 +248,20 @@ func (o *ListOptions) AsListOptions() *metav1.ListOptions { return o.Raw } +// ApplyOptions executes the given ListOptionFuncs and returns the mutated +// ListOptions. +func (o *ListOptions) ApplyOptions(optFuncs []ListOptionFunc) *ListOptions { + for _, optFunc := range optFuncs { + optFunc(o) + } + return o +} + +// ListOptionFunc is a function that mutates a ListOptions struct. It implements +// the functional options pattern. See +// https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md. +type ListOptionFunc func(*ListOptions) + // MatchingLabels is a convenience function that sets the label selector // to match the given labels, and then returns the options. // It mutates the list options. @@ -273,20 +287,39 @@ func (o *ListOptions) InNamespace(ns string) *ListOptions { return o } -// MatchingLabels is a convenience function that constructs list options -// to match the given labels. -func MatchingLabels(lbls map[string]string) *ListOptions { - return (&ListOptions{}).MatchingLabels(lbls) +// MatchingLabels is a functional option that sets the LabelSelector field of +// a ListOptions struct. +func MatchingLabels(lbls map[string]string) ListOptionFunc { + sel := labels.SelectorFromSet(lbls) + return func(opts *ListOptions) { + opts.LabelSelector = sel + } +} + +// MatchingField is a functional option that sets the FieldSelector field of +// a ListOptions struct. +func MatchingField(name, val string) ListOptionFunc { + sel := fields.SelectorFromSet(fields.Set{name: val}) + return func(opts *ListOptions) { + opts.FieldSelector = sel + } } -// MatchingField is a convenience function that constructs list options -// to match the given field. -func MatchingField(name, val string) *ListOptions { - return (&ListOptions{}).MatchingField(name, val) +// InNamespace is a functional option that sets the Namespace field of +// a ListOptions struct. +func InNamespace(ns string) ListOptionFunc { + return func(opts *ListOptions) { + opts.Namespace = ns + } } -// InNamespace is a convenience function that constructs list -// options to list in the given namespace. -func InNamespace(ns string) *ListOptions { - return (&ListOptions{}).InNamespace(ns) +// UseListOptions is a functional option that replaces the fields of a +// ListOptions struct with those of a different ListOptions struct. +// +// Example: +// cl.List(ctx, list, client.UseListOptions(lo.InNamespace(ns).MatchingLabels(labels))) +func UseListOptions(newOpts *ListOptions) ListOptionFunc { + return func(opts *ListOptions) { + *opts = *newOpts + } } diff --git a/pkg/client/split.go b/pkg/client/split.go index efcf6d4c31..91519196a8 100644 --- a/pkg/client/split.go +++ b/pkg/client/split.go @@ -50,10 +50,10 @@ func (d *DelegatingReader) Get(ctx context.Context, key ObjectKey, obj runtime.O } // List retrieves list of objects for a given namespace and list options. -func (d *DelegatingReader) List(ctx context.Context, opts *ListOptions, list runtime.Object) error { +func (d *DelegatingReader) List(ctx context.Context, list runtime.Object, opts ...ListOptionFunc) error { _, isUnstructured := list.(*unstructured.UnstructuredList) if isUnstructured { - return d.ClientReader.List(ctx, opts, list) + return d.ClientReader.List(ctx, list, opts...) } - return d.CacheReader.List(ctx, opts, list) + return d.CacheReader.List(ctx, list, opts...) } diff --git a/pkg/client/typed_client.go b/pkg/client/typed_client.go index 26cb81d8ba..47a90e37d9 100644 --- a/pkg/client/typed_client.go +++ b/pkg/client/typed_client.go @@ -92,20 +92,18 @@ func (c *typedClient) Get(ctx context.Context, key ObjectKey, obj runtime.Object } // List implements client.Client -func (c *typedClient) List(ctx context.Context, opts *ListOptions, obj runtime.Object) error { +func (c *typedClient) List(ctx context.Context, obj runtime.Object, opts ...ListOptionFunc) error { r, err := c.cache.getResource(obj) if err != nil { return err } - namespace := "" - if opts != nil { - namespace = opts.Namespace - } + listOpts := ListOptions{} + listOpts.ApplyOptions(opts) return r.Get(). - NamespaceIfScoped(namespace, r.isNamespaced()). + NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()). Resource(r.resource()). Body(obj). - VersionedParams(opts.AsListOptions(), c.paramCodec). + VersionedParams(listOpts.AsListOptions(), c.paramCodec). Context(ctx). Do(). Into(obj) diff --git a/pkg/client/unstructured_client.go b/pkg/client/unstructured_client.go index 2ce0b19eb8..4e616b8946 100644 --- a/pkg/client/unstructured_client.go +++ b/pkg/client/unstructured_client.go @@ -106,7 +106,7 @@ func (uc *unstructuredClient) Get(_ context.Context, key ObjectKey, obj runtime. } // List implements client.Client -func (uc *unstructuredClient) List(_ context.Context, opts *ListOptions, obj runtime.Object) error { +func (uc *unstructuredClient) List(_ context.Context, obj runtime.Object, opts ...ListOptionFunc) error { u, ok := obj.(*unstructured.UnstructuredList) if !ok { return fmt.Errorf("unstructured client did not understand object: %T", obj) @@ -115,16 +115,14 @@ func (uc *unstructuredClient) List(_ context.Context, opts *ListOptions, obj run if strings.HasSuffix(gvk.Kind, "List") { gvk.Kind = gvk.Kind[:len(gvk.Kind)-4] } - namespace := "" - if opts != nil { - namespace = opts.Namespace - } - r, err := uc.getResourceInterface(gvk, namespace) + listOpts := ListOptions{} + listOpts.ApplyOptions(opts) + r, err := uc.getResourceInterface(gvk, listOpts.Namespace) if err != nil { return err } - i, err := r.List(*opts.AsListOptions()) + i, err := r.List(*listOpts.AsListOptions()) if err != nil { return err } diff --git a/pkg/webhook/internal/cert/writer/secret_test.go b/pkg/webhook/internal/cert/writer/secret_test.go index 03b73d12fd..e8f413d74b 100644 --- a/pkg/webhook/internal/cert/writer/secret_test.go +++ b/pkg/webhook/internal/cert/writer/secret_test.go @@ -108,9 +108,7 @@ var _ = Describe("secretCertWriter", func() { _, _, err := certWriter.EnsureCert(dnsName) Expect(err).NotTo(HaveOccurred()) list := &corev1.SecretList{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - }, list) + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(HaveLen(1)) }) @@ -121,9 +119,7 @@ var _ = Describe("secretCertWriter", func() { _, changed, err := certWriter.EnsureCert(dnsName) Expect(err).NotTo(HaveOccurred()) list := &corev1.SecretList{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - }, list) + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(ConsistOf(*secret)) Expect(list.Items).To(HaveLen(1)) @@ -147,9 +143,7 @@ var _ = Describe("secretCertWriter", func() { _, changed, err := certWriter.EnsureCert(dnsName) Expect(err).NotTo(HaveOccurred()) list := &corev1.SecretList{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - }, list) + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(ConsistOf(*secret)) Expect(list.Items).To(HaveLen(1)) @@ -174,9 +168,7 @@ var _ = Describe("secretCertWriter", func() { _, changed, err := certWriter.EnsureCert(dnsName) Expect(err).NotTo(HaveOccurred()) list := &corev1.SecretList{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - }, list) + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(ConsistOf(*secret)) Expect(list.Items).To(HaveLen(1)) @@ -214,9 +206,7 @@ var _ = Describe("secretCertWriter", func() { _, changed, err := certWriter.EnsureCert(dnsName) Expect(err).NotTo(HaveOccurred()) list := &corev1.SecretList{} - err = sCertWriter.Client.List(nil, &client.ListOptions{ - Namespace: "namespace-bar", - }, list) + err = sCertWriter.Client.List(nil, list, client.InNamespace("namespace-bar")) Expect(err).NotTo(HaveOccurred()) Expect(list.Items).To(HaveLen(1)) Expect(list.Items[0]).To(Equal(*oldSecret))