-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] ✨ Add support for implicit paging in un/structured clients #1358
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ package client | |
import ( | ||
"context" | ||
|
||
apimeta "k8s.io/apimachinery/pkg/api/meta" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
) | ||
|
||
|
@@ -143,6 +144,72 @@ func (c *typedClient) Get(ctx context.Context, key ObjectKey, obj Object) error | |
Name(key.Name).Do(ctx).Into(obj) | ||
} | ||
|
||
func (c *typedClient) ListPages(ctx context.Context, obj ObjectList, | ||
callback func(obj ObjectList) error, opts ...ListOption) error { | ||
r, err := c.cache.getResource(obj) | ||
if err != nil { | ||
return err | ||
} | ||
listOpts := ListOptions{} | ||
listOpts.ApplyOptions(opts) | ||
|
||
// Fetch items at chunks of one hundred if not specified differently. | ||
if listOpts.Limit == 0 { | ||
Limit(DefaultPageLimit).ApplyToList(&listOpts) | ||
} | ||
|
||
// Retrieve initial chunck of data. | ||
var allItems []runtime.Object | ||
var interimResult ObjectList | ||
err = r.Get(). | ||
NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()). | ||
Resource(r.resource()). | ||
VersionedParams(listOpts.AsListOptions(), c.paramCodec). | ||
Do(ctx).Into(interimResult) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if err := callback(interimResult); err != nil { | ||
return err | ||
} | ||
|
||
items, err := apimeta.ExtractList(interimResult) | ||
if err != nil { | ||
return err | ||
} | ||
allItems = append(allItems, items...) | ||
|
||
// Continue while there are more chunks. | ||
for { | ||
if interimResult.GetContinue() == "" { | ||
break | ||
} | ||
|
||
Continue(interimResult.GetContinue()).ApplyToList(&listOpts) | ||
err = r.Get(). | ||
nimrodshn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
NamespaceIfScoped(listOpts.Namespace, r.isNamespaced()). | ||
Resource(r.resource()). | ||
VersionedParams(listOpts.AsListOptions(), c.paramCodec). | ||
Do(ctx).Into(interimResult) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if err := callback(interimResult); err != nil { | ||
return err | ||
} | ||
|
||
items, err = apimeta.ExtractList(interimResult) | ||
if err != nil { | ||
return err | ||
} | ||
allItems = append(allItems, items...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want both a callback function and a huge list return, we should pick one pattern or the other (or move them to separate functions/options). The reason to use a visitor callback function is to avoid keeping the whole list in memory. I think it would be more Go-like to just do the callback style probably, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vincepri Do you have opinions on which API to support (or both)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Gotcha. 👍 @coderanger Actoually thinking about it - if the user would like to accumulate the result she can simply use the same There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is, she can pass something like this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, or more likely cast it to the correct type and skip needing ExtractList :) |
||
} | ||
|
||
return apimeta.SetList(obj, allItems) | ||
} | ||
|
||
// List implements client.Client | ||
func (c *typedClient) List(ctx context.Context, obj ObjectList, opts ...ListOption) error { | ||
r, err := c.cache.getResource(obj) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs a better name since it's not the default for List() too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vincepri @cben Any suggestions?