Skip to content
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

⚠ Add GetOptions as optional argument of client.Reader and all its implementation #1917

Merged
merged 1 commit into from Jun 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/cache/informer_cache.go
Expand Up @@ -51,7 +51,7 @@ type informerCache struct {
}

// Get implements Reader.
func (ip *informerCache) Get(ctx context.Context, key client.ObjectKey, out client.Object) error {
func (ip *informerCache) Get(ctx context.Context, key client.ObjectKey, out client.Object, opts ...client.GetOption) error {
gvk, err := apiutil.GVKForObject(out, ip.Scheme)
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion pkg/cache/informertest/fake_cache.go
Expand Up @@ -131,7 +131,7 @@ func (c *FakeInformers) IndexField(ctx context.Context, obj client.Object, field
}

// Get implements Cache.
func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
func (c *FakeInformers) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/cache/internal/cache_reader.go
Expand Up @@ -54,7 +54,7 @@ type CacheReader struct {
}

// Get checks the indexer for the object and writes a copy of it if found.
func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Object) error {
func (c *CacheReader) Get(_ context.Context, key client.ObjectKey, out client.Object, opts ...client.GetOption) error {
if c.scopeName == apimeta.RESTScopeNameRoot {
key.Namespace = ""
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/cache/multi_namespace_cache.go
Expand Up @@ -200,7 +200,7 @@ func (c *multiNamespaceCache) IndexField(ctx context.Context, obj client.Object,
return nil
}

func (c *multiNamespaceCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
func (c *multiNamespaceCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
isNamespaced, err := objectutil.IsAPINamespaced(obj, c.Scheme, c.RESTMapper)
if err != nil {
return err
Expand Down
8 changes: 4 additions & 4 deletions pkg/client/client.go
Expand Up @@ -241,16 +241,16 @@ func (c *client) Patch(ctx context.Context, obj Object, patch Patch, opts ...Pat
}

// Get implements client.Client.
func (c *client) Get(ctx context.Context, key ObjectKey, obj Object) error {
func (c *client) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
switch obj.(type) {
case *unstructured.Unstructured:
return c.unstructuredClient.Get(ctx, key, obj)
return c.unstructuredClient.Get(ctx, key, obj, opts...)
case *metav1.PartialObjectMetadata:
// Metadata only object should always preserve the GVK coming in from the caller.
defer c.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
return c.metadataClient.Get(ctx, key, obj)
return c.metadataClient.Get(ctx, key, obj, opts...)
default:
return c.typedClient.Get(ctx, key, obj)
return c.typedClient.Get(ctx, key, obj, opts...)
}
}

Expand Down
20 changes: 19 additions & 1 deletion pkg/client/client_test.go
Expand Up @@ -2913,6 +2913,24 @@ var _ = Describe("Client", func() {
})
})

Describe("GetOptions", func() {
It("should be convertable to metav1.GetOptions", func() {
o := (&client.GetOptions{}).ApplyOptions([]client.GetOption{
&client.GetOptions{Raw: &metav1.GetOptions{ResourceVersion: "RV0"}},
})
mo := o.AsGetOptions()
Expect(mo).NotTo(BeNil())
Expect(mo.ResourceVersion).To(Equal("RV0"))
})

It("should produce empty metav1.GetOptions if nil", func() {
var o *client.GetOptions
Expect(o.AsGetOptions()).To(Equal(&metav1.GetOptions{}))
o = &client.GetOptions{}
Expect(o.AsGetOptions()).To(Equal(&metav1.GetOptions{}))
})
})

Describe("ListOptions", func() {
It("should be convertable to metav1.ListOptions", func() {
lo := (&client.ListOptions{}).ApplyOptions([]client.ListOption{
Expand Down Expand Up @@ -3389,7 +3407,7 @@ type fakeReader struct {
Called int
}

func (f *fakeReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
func (f *fakeReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
f.Called++
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/client/dryrun.go
Expand Up @@ -72,8 +72,8 @@ func (c *dryRunClient) Patch(ctx context.Context, obj Object, patch Patch, opts
}

// Get implements client.Client.
func (c *dryRunClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
return c.client.Get(ctx, key, obj)
func (c *dryRunClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
return c.client.Get(ctx, key, obj, opts...)
}

// List implements client.Client.
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/fake/client.go
Expand Up @@ -329,7 +329,7 @@ func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Ob
return t.ObjectTracker.Update(gvr, obj, ns)
}

func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
func (c *fakeClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
gvr, err := getGVRFromObject(obj, c.scheme)
if err != nil {
return err
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/interfaces.go
Expand Up @@ -50,7 +50,7 @@ type Reader interface {
// Get retrieves an obj for the given object key from the Kubernetes Cluster.
// obj must be a struct pointer so that obj can be updated with the response
// returned by the Server.
Get(ctx context.Context, key ObjectKey, obj Object) error
Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally the change looks good to me, but it needs to be marked as breaking since we change the interface here. It is very likely that ppl have fake implementations of the client that need changing to adhere to the new signature

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I have updated the title. Do you think it is necessary to add the breaking warning into code comments?


// 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
Expand Down
7 changes: 5 additions & 2 deletions pkg/client/metadata_client.go
Expand Up @@ -116,20 +116,23 @@ func (mc *metadataClient) Patch(ctx context.Context, obj Object, patch Patch, op
}

// Get implements client.Client.
func (mc *metadataClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
func (mc *metadataClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
metadata, ok := obj.(*metav1.PartialObjectMetadata)
if !ok {
return fmt.Errorf("metadata client did not understand object: %T", obj)
}

gvk := metadata.GroupVersionKind()

getOpts := GetOptions{}
getOpts.ApplyOptions(opts)

resInt, err := mc.getResourceInterface(gvk, key.Namespace)
if err != nil {
return err
}

res, err := resInt.Get(ctx, key.Name, metav1.GetOptions{})
res, err := resInt.Get(ctx, key.Name, *getOpts.AsGetOptions())
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/client/namespaced_client.go
Expand Up @@ -138,7 +138,7 @@ func (n *namespacedClient) Patch(ctx context.Context, obj Object, patch Patch, o
}

// Get implements client.Client.
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
isNamespaceScoped, err := objectutil.IsAPINamespaced(obj, n.Scheme(), n.RESTMapper())
if err != nil {
return fmt.Errorf("error finding the scope of the object: %w", err)
Expand All @@ -149,7 +149,7 @@ func (n *namespacedClient) Get(ctx context.Context, key ObjectKey, obj Object) e
}
key.Namespace = n.namespace
}
return n.client.Get(ctx, key, obj)
return n.client.Get(ctx, key, obj, opts...)
}

// List implements client.Client.
Expand Down
45 changes: 45 additions & 0 deletions pkg/client/options.go
Expand Up @@ -37,6 +37,12 @@ type DeleteOption interface {
ApplyToDelete(*DeleteOptions)
}

// GetOption is some configuration that modifies options for a get request.
type GetOption interface {
// ApplyToGet applies this configuration to the given get options.
ApplyToGet(*GetOptions)
}

// ListOption is some configuration that modifies options for a list request.
type ListOption interface {
// ApplyToList applies this configuration to the given list options.
Expand Down Expand Up @@ -311,6 +317,45 @@ func (p PropagationPolicy) ApplyToDeleteAllOf(opts *DeleteAllOfOptions) {

// }}}

// {{{ Get Options

// GetOptions contains options for get operation.
// Now it only has a Raw field, with support for specific resourceVersion.
type GetOptions struct {
// Raw represents raw GetOptions, as passed to the API server. Note
// that these may not be respected by all implementations of interface.
Raw *metav1.GetOptions
}

var _ GetOption = &GetOptions{}

// ApplyToGet implements GetOption for GetOptions.
func (o *GetOptions) ApplyToGet(lo *GetOptions) {
if o.Raw != nil {
lo.Raw = o.Raw
}
}

// AsGetOptions returns these options as a flattened metav1.GetOptions.
// This may mutate the Raw field.
func (o *GetOptions) AsGetOptions() *metav1.GetOptions {
if o == nil || o.Raw == nil {
return &metav1.GetOptions{}
}
return o.Raw
}

// ApplyOptions applies the given get options on these options,
// and then returns itself (for convenient chaining).
func (o *GetOptions) ApplyOptions(opts []GetOption) *GetOptions {
for _, opt := range opts {
opt.ApplyToGet(o)
}
return o
}

// }}}

// {{{ List Options

// ListOptions contains options for limiting or filtering results.
Expand Down
9 changes: 9 additions & 0 deletions pkg/client/options_test.go
Expand Up @@ -74,6 +74,15 @@ var _ = Describe("ListOptions", func() {
})
})

var _ = Describe("GetOptions", func() {
It("Should set Raw", func() {
o := &client.GetOptions{Raw: &metav1.GetOptions{ResourceVersion: "RV0"}}
newGetOpts := &client.GetOptions{}
o.ApplyToGet(newGetOpts)
Expect(newGetOpts).To(Equal(o))
})
})

var _ = Describe("CreateOptions", func() {
It("Should set DryRun", func() {
o := &client.CreateOptions{DryRun: []string{"Hello", "Theodore"}}
Expand Down
6 changes: 3 additions & 3 deletions pkg/client/split.go
Expand Up @@ -121,13 +121,13 @@ func (d *delegatingReader) shouldBypassCache(obj runtime.Object) (bool, error) {
}

// Get retrieves an obj for a given object key from the Kubernetes Cluster.
func (d *delegatingReader) Get(ctx context.Context, key ObjectKey, obj Object) error {
func (d *delegatingReader) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
if isUncached, err := d.shouldBypassCache(obj); err != nil {
return err
} else if isUncached {
return d.ClientReader.Get(ctx, key, obj)
return d.ClientReader.Get(ctx, key, obj, opts...)
}
return d.CacheReader.Get(ctx, key, obj)
return d.CacheReader.Get(ctx, key, obj, opts...)
}

// List retrieves list of objects for a given namespace and list options.
Expand Down
5 changes: 4 additions & 1 deletion pkg/client/typed_client.go
Expand Up @@ -132,14 +132,17 @@ func (c *typedClient) Patch(ctx context.Context, obj Object, patch Patch, opts .
}

// Get implements client.Client.
func (c *typedClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
func (c *typedClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
r, err := c.cache.getResource(obj)
if err != nil {
return err
}
getOpts := GetOptions{}
getOpts.ApplyOptions(opts)
return r.Get().
NamespaceIfScoped(key.Namespace, r.isNamespaced()).
Resource(r.resource()).
VersionedParams(getOpts.AsGetOptions(), c.paramCodec).
Name(key.Name).Do(ctx).Into(obj)
}

Expand Down
6 changes: 5 additions & 1 deletion pkg/client/unstructured_client.go
Expand Up @@ -165,14 +165,17 @@ func (uc *unstructuredClient) Patch(ctx context.Context, obj Object, patch Patch
}

// Get implements client.Client.
func (uc *unstructuredClient) Get(ctx context.Context, key ObjectKey, obj Object) error {
func (uc *unstructuredClient) Get(ctx context.Context, key ObjectKey, obj Object, opts ...GetOption) error {
u, ok := obj.(*unstructured.Unstructured)
if !ok {
return fmt.Errorf("unstructured client did not understand object: %T", obj)
}

gvk := u.GroupVersionKind()

getOpts := GetOptions{}
getOpts.ApplyOptions(opts)

r, err := uc.cache.getResource(obj)
if err != nil {
return err
Expand All @@ -181,6 +184,7 @@ func (uc *unstructuredClient) Get(ctx context.Context, key ObjectKey, obj Object
result := r.Get().
NamespaceIfScoped(key.Namespace, r.isNamespaced()).
Resource(r.resource()).
VersionedParams(getOpts.AsGetOptions(), uc.paramCodec).
Name(key.Name).
Do(ctx).
Into(obj)
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/controllerutil/controllerutil_test.go
Expand Up @@ -832,6 +832,6 @@ type errorReader struct {
client.Client
}

func (e errorReader) Get(ctx context.Context, key client.ObjectKey, into client.Object) error {
func (e errorReader) Get(ctx context.Context, key client.ObjectKey, into client.Object, opts ...client.GetOption) error {
return fmt.Errorf("unexpected error")
}