Convert client.List to use functional options #106

grantr · 2018-08-08T00:20:00Z

Replaces:

List(ctx, ListOptions, list)

with:

List(ctx, list, ...Option)

This matches the functional options signature of client.Delete in #94.

A few examples (I tried formatting these as a table but it wasn't wide enough):

Select all:

// current
List(ctx, nil, list)
// proposed
List(ctx, list)

Filter by namespace:

// current
List(ctx, client.InNamespace(ns), list)
// proposed
List(ctx, list, client.InNamespace(ns))

Filter by namespace and labels:

// current
List(ctx, client.InNamespace(ns).MatchingLabels(labels), list)
// proposed
List(ctx, list, client.InNamespace(ns), client.MatchingLabels(labels))

To assemble a ListOptions with the builder methods or some other strategy, use the UseListOptions functional option:

lo := &client.ListOptions{}
client.List(ctx, list, client.UseListOptions(
  lo.InNamespace(ns).MatchingLabels(labels)
))

DirectXMan12 · 2018-08-09T16:19:36Z

@pwittrock what's our policy on breaking backwards compat (or incrementing major versions)? This ends up with a much more ergonomic interface, and while I'm generally loathe to break the compatibility, I think it ends up being much less verbose, and follows the "you don't have to care about what you're not using" principle better.

droot · 2018-08-15T19:08:41Z

@grantr new interface is nice UX wise, but can't get break the backward compatibility.

@DirectXMan12 We haven't defined the policy yet and have not been versioning the lib yet (so far, the unsaid rule is not to break the API :)). Are there any good library projects we can take inspiration from wrt versioning/backward-compatibility aspect ?

grantr · 2018-08-15T22:45:04Z

I think the backward compatibility policy dovetails with the dependency versioning story from #111. How much is controller-runtime a standalone library versus a private dependency of kubebuilder?

From my perspective (of course 😸) I'd like kubebuilder to be the semantically versioned umbrella, with each release pinning projects that use it to a specific revision of controller-runtime. Then controller-runtime can remain unversioned or have a different versioning scheme, with the freedom to move quickly knowing that kubebuilder users won't accidentally upgrade. When users install a new version of kubebuilder, an upgrade procedure can point out or automatically fix incompatibilities.

Anyone using controller-runtime directly is on their own fixing breaking changes, which is probably what they expect from an unversioned library. Maybe a release notes file in the repo could have a list of breaking changes to guide them.

In lieu of the above, what about this until the next major "release" (in the semantic versioning sense):

Keep the existing List, but mark it deprecated, noting that it will be removed at next major release.
Rename this List to ListVariadic (or some other, better name). Encourage users to start using it, and note that it will be renamed to List at next major release.

That gives users a way to proactively migrate, and a clean upgrade story for those who did (s/ListVariadic/List/).

DirectXMan12 · 2018-08-21T20:16:04Z

@grantr I we're aiming to set up a version policy (semver) and have a story mostly ironed out for kubebuilder versioning, so we should be able to tag first semver release, and then hypothetically make a breaking change, and then do a new major version.

@droot a lot of them that I know do one of a few things:

a) add in alternate names, like suggested by @grantr. This tends to result in a confusing mess if those are left to languish for too long (or sometimes even if they're not), from what I've seen.
b) do a number of rapid "major" version revs pre-1.0, and then tag 1.0.0 and move to step c
c) "save up" major changes until you hit critical mass for a major version change, then do a large major revision

Of course, we'll want to avoid breaking changes in general, but I think we'll have a short period where we'll want to make one or two and do a few version revs, simply due to the nature of actually getting user feedback (i.e. option b above).

As for ListVariadic, since we're still early on, we may just want to do a new major version quickly, after releasing an initial stability version, and avoid confusion from that. WDYT @droot ?

droot · 2018-08-21T21:26:44Z

How much is controller-runtime a standalone library versus a private dependency of kubebuilder?

Goal is to have controller-runtime a standalone library following proper semver style versioning scheme so that Kubebuilder generated projects are reliably depend on it. As @DirectXMan12 pointed out, we will soon move to the new world to achieve that goal because we were figuring out the dependency story for kubebuilder generated projects. We sort of have a proposal which will work.

From my perspective (of course 😸) I'd like kubebuilder to be the semantically versioned umbrella, with each release pinning projects that use it to a specific revision of controller-runtime. Then controller-runtime can remain unversioned or have a different versioning scheme, with the freedom to move quickly knowing that kubebuilder users won't accidentally upgrade. When users install a new version of kubebuilder, an upgrade procedure can point out or automatically fix incompatibilities.

Instead of pinning to specific revision (I am assuming you referring to SHA), we can pin the controller-runtime to a minor version (0.1.x say) in Kubebuilder projects so that we don't have to do Kubebuilder release for every patch version on controller-runtime.

In lieu of the above, what about this until the next major "release" (in the semantic versioning sense):
Keep the existing List, but mark it deprecated, noting that it will be removed at next major release.
Rename this List to ListVariadic (or some other, better name). Encourage users to start using it, and note that it will be renamed to List at next major release.
That gives users a way to proactively migrate, and a clean upgrade story for those who did (s/ListVariadic/List/).

Sounds reasonable.

@DirectXMan12 will respond to your post soon.

droot · 2018-09-04T21:01:17Z

@grantr we have sorted out the versioning story for controller-runtime/tools and Kubebuilder now uses versioned pkgs of controller-*, so we can move forward with these changes now.

DirectXMan12 · 2018-09-05T17:06:14Z

looks like it just needs a rebase to fix merge conflicts

grantr · 2018-09-05T20:00:46Z

👍 Hoping to get back to this tomorrow!

grantr · 2018-09-06T17:48:08Z

Rebased.

DirectXMan12 · 2018-09-06T19:26:26Z

/lgtm
/approve

grantr · 2018-09-06T19:38:37Z

There's an issue with the fake client and this new style.

Currently the fake client's List implementation relies on the provided ListOptions having a TypeMeta in its Raw field. See https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/fake/client.go#L81 and here for an example of the required workaround in user code.

This PR removes the ability of the user to provide a Raw field, and thus breaks the fake client's List implementation.

Is there a way to infer the GVK of the resource wrapped by a List object?

DirectXMan12 · 2018-09-06T20:13:00Z

/hold
while we figure that out (although the unit test failure should prevent it anyway)

DirectXMan12 · 2018-09-06T20:15:52Z

Is there a way to infer the GVK of the resource wrapped by a List object?

Yeah, you should be able to do similar logic to what we actually do in the client -- the Scheme has logic for converting a runtime.Object to a GVK, and then the Kind can be truncated (remove the List) to find the underlying Kind.

DirectXMan12 · 2018-09-06T20:17:49Z

This PR removes the ability of the user to provide a Raw field

Huh? Can you just call UseListOptions(somethingWithRawSet)?

grantr · 2018-09-06T20:27:27Z

Oh true, that is possible. In practice that would be required for every call to List from (well-tested) user code, which kind of defeats the purpose of this change. I'd still like to fix the fake client before merging, or if someone else wants to try that's fine too.

grantr · 2018-09-06T21:59:43Z

I updated the fake client to no longer need Raw.TypeMeta, updated all the implementations of List with the new signature, and updated tests to use the new signature. There's one failing test in certwriter that I haven't figured out yet, but I thought I'd push the new work for review anyway.

grantr · 2018-09-07T00:01:14Z

Rebased to squash the fixup commits.

DirectXMan12 · 2018-09-10T17:25:48Z

pkg/client/fake/client.go

-func (c *fakeClient) List(ctx context.Context, opts *client.ListOptions, list runtime.Object) error {
-	gvk := opts.Raw.TypeMeta.GroupVersionKind()
+func (c *fakeClient) List(ctx context.Context, obj runtime.Object, opts ...client.ListOptionFunc) error {
+	gvk, err := apiutil.GVKForObject(obj, scheme.Scheme)


it should be possible to set the scheme for the fake client, otherwise the fake client might not work for some user resources. We should use this scheme if none is explicitly set.

Agreed, though the fake client already restricts itself to scheme.Scheme in its constructor

controller-runtime/pkg/client/fake/client.go

Line 50 in 18a8fbc

tracker := testing.NewObjectTracker(scheme.Scheme, scheme.Codecs.UniversalDecoder())

ah, ok. Let's do that in a follow-up PR then. Can you file an issue?

#137. The workaround for now is to add custom types to scheme.Scheme in an init function in the test file. See https://github.com/knative/eventing/blob/e6fb7df8f1b83b6e78f6639f7f7461ee75a9b196/pkg/controller/feed/reconcile_test.go.

grantr · 2018-09-14T18:34:59Z

Oh right, Prow isn't used here so /retest doesn't work. I don't appear to have permission to try another build - can someone else do that?

Locally I'm consistently getting a different error related to the event recorder:

Timeout [1.003 seconds]
recorder
/go/src/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder_integration_test.go:36
  recorder
  /go/src/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder_integration_test.go:48
    should publish events [It]
    /go/src/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder_integration_test.go:49

    Timed out

    /go/src/sigs.k8s.io/controller-runtime/pkg/internal/recorder/recorder_integration_test.go:49

DirectXMan12 · 2018-09-14T19:31:37Z

@droot @pwittrock we should switch to prow when we get a chance.

DirectXMan12 · 2018-09-14T19:32:34Z

I restarted the test. Let's see if it works, and file a bug about the flakes. Don't want to have any of those.

DirectXMan12 · 2018-09-14T19:41:21Z

/lgtm
/approve

k8s-ci-robot · 2018-09-14T19:41:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, grantr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

grantr · 2018-09-14T20:50:45Z

/hold cancel
(hold was added to block on versioning strategy which is done now)

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) ))

The fake client now uses similar GVK detection code as the other implementations and no longer relies on the Raw object having a TypeMeta.

Now that the fake client no longer needs Raw, these tests can use typed instead of untyped List objects.

grantr · 2018-09-14T21:18:26Z

Hopefully the final rebase!

grantr · 2018-09-14T21:29:33Z

@DirectXMan12 need another lgtm after rebase, please :)

DirectXMan12 · 2018-09-17T09:11:19Z

/lgtm

mengqiy · 2018-09-17T23:22:23Z

Had a offline discussion with @droot, we probably should have something like this to not break existing users.

type Reader interface {
	Get(ctx context.Context, key ObjectKey, obj runtime.Object) error

	// Deprecated
	// This is the old signature.
	List(ctx context.Context, list runtime.Object, opts ...ListOptionFunc) error

	// This is the new signature. We may want to have a better name.
	ListV2(ctx context.Context, obj runtime.Object, opts ...ListOptionFunc) error 
}

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 kubernetes-sigs#106 PR

Category support

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 8, 2018

grantr mentioned this pull request Aug 8, 2018

Add options to client.Delete() #94

Merged

grantr force-pushed the list-options branch from f969025 to f4bf564 Compare September 6, 2018 17:47

k8s-ci-robot assigned DirectXMan12 Sep 6, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 6, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2018

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 6, 2018

grantr force-pushed the list-options branch from d2718ca to 18a8fbc Compare September 7, 2018 00:00

DirectXMan12 reviewed Sep 10, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2018

grantr added 3 commits September 14, 2018 14:14

Update List implementations to use new signature

8ca92a0

The fake client now uses similar GVK detection code as the other implementations and no longer relies on the Raw object having a TypeMeta.

Update tests to use new List signature

bf0795d

Now that the fake client no longer needs Raw, these tests can use typed instead of untyped List objects.

grantr force-pushed the list-options branch from 57c4ead to bf0795d Compare September 14, 2018 21:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 14, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2018

k8s-ci-robot merged commit 28ba1f2 into kubernetes-sigs:master Sep 17, 2018

grantr deleted the list-options branch September 17, 2018 17:01

mengqiy mentioned this pull request Sep 17, 2018

Revert "Convert client.List to use functional options" #146

Merged

mengqiy mentioned this pull request Sep 20, 2018

Redo client.List() in a backward compatible way #155

Closed

grantr mentioned this pull request Nov 8, 2018

Remove the hack around fake client list bugs. knative/eventing#590

Closed

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#106 from grantr/list-options

98c108d

Convert client.List to use functional options

droot mentioned this pull request Jan 15, 2019

⚠️ Convert client.List to use functional options #294

Merged

grantr mentioned this pull request Apr 1, 2019

🐛Fix used scheme for fake client in List method #380

Merged

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020

Merge pull request #106 from pmorie/category-support

2d1d976

Category support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert client.List to use functional options #106

Convert client.List to use functional options #106

grantr commented Aug 8, 2018 •

edited

Loading

DirectXMan12 commented Aug 9, 2018 •

edited

Loading

droot commented Aug 15, 2018 •

edited

Loading

grantr commented Aug 15, 2018 •

edited

Loading

DirectXMan12 commented Aug 21, 2018

droot commented Aug 21, 2018

droot commented Sep 4, 2018

DirectXMan12 commented Sep 5, 2018

grantr commented Sep 5, 2018

grantr commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

grantr commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

grantr commented Sep 6, 2018

grantr commented Sep 6, 2018

grantr commented Sep 7, 2018

DirectXMan12 Sep 10, 2018

grantr Sep 10, 2018

DirectXMan12 Sep 10, 2018

grantr Sep 10, 2018

DirectXMan12 Sep 11, 2018

grantr commented Sep 14, 2018

DirectXMan12 commented Sep 14, 2018

DirectXMan12 commented Sep 14, 2018

DirectXMan12 commented Sep 14, 2018

k8s-ci-robot commented Sep 14, 2018

grantr commented Sep 14, 2018

grantr commented Sep 14, 2018

grantr commented Sep 14, 2018

DirectXMan12 commented Sep 17, 2018

mengqiy commented Sep 17, 2018 •

edited

Loading

Convert client.List to use functional options #106

Convert client.List to use functional options #106

Conversation

grantr commented Aug 8, 2018 • edited Loading

DirectXMan12 commented Aug 9, 2018 • edited Loading

droot commented Aug 15, 2018 • edited Loading

grantr commented Aug 15, 2018 • edited Loading

DirectXMan12 commented Aug 21, 2018

droot commented Aug 21, 2018

droot commented Sep 4, 2018

DirectXMan12 commented Sep 5, 2018

grantr commented Sep 5, 2018

grantr commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

grantr commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

DirectXMan12 commented Sep 6, 2018

grantr commented Sep 6, 2018

grantr commented Sep 6, 2018

grantr commented Sep 7, 2018

DirectXMan12 Sep 10, 2018

Choose a reason for hiding this comment

grantr Sep 10, 2018

Choose a reason for hiding this comment

DirectXMan12 Sep 10, 2018

Choose a reason for hiding this comment

grantr Sep 10, 2018

Choose a reason for hiding this comment

DirectXMan12 Sep 11, 2018

Choose a reason for hiding this comment

grantr commented Sep 14, 2018

DirectXMan12 commented Sep 14, 2018

DirectXMan12 commented Sep 14, 2018

DirectXMan12 commented Sep 14, 2018

k8s-ci-robot commented Sep 14, 2018

grantr commented Sep 14, 2018

grantr commented Sep 14, 2018

grantr commented Sep 14, 2018

DirectXMan12 commented Sep 17, 2018

mengqiy commented Sep 17, 2018 • edited Loading

grantr commented Aug 8, 2018 •

edited

Loading

DirectXMan12 commented Aug 9, 2018 •

edited

Loading

droot commented Aug 15, 2018 •

edited

Loading

grantr commented Aug 15, 2018 •

edited

Loading

mengqiy commented Sep 17, 2018 •

edited

Loading