Skip to content

Commit

Permalink
Merge pull request #294 from droot/list-options
Browse files Browse the repository at this point in the history 
⚠️ Convert client.List to use functional options
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jan 16, 2019
2 parents 0dd6edb + 3285ad1 commit 5734523
Show file tree
Hide file tree
Showing 15 changed files with 159 additions and 164 deletions.
3 changes: 2 additions & 1 deletion pkg/builder/example_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
53 changes: 21 additions & 32 deletions pkg/cache/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -154,10 +154,9 @@ var _ = Describe("Informer Cache", func() {
By("listing pods with a particular label")
// NB: each pod has a "test-label": <pod-name>
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())
Expand All @@ -174,9 +173,7 @@ var _ = Describe("Informer Cache", func() {
// NB: each pod has a "test-label": <pod-name>
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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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")
Expand Down
4 changes: 2 additions & 2 deletions pkg/cache/informer_cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/cache/informertest/fake_cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
19 changes: 11 additions & 8 deletions pkg/cache/internal/cache_reader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,32 +87,35 @@ 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()
}
if err != nil {
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)
Expand Down
6 changes: 3 additions & 3 deletions pkg/client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading

0 comments on commit 5734523

Please sign in to comment.