Generify lister-gen

[skitt]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This adds a generic implementation of a lister, and uses it to replace the template code in generated listers. The corresponding templates are no longer used and are removed.

Listers are reduced to their interfaces (non-namespaced and namespaced if appropriate), their specific structs, and their constructors. All method implementations are provided by the generic implementation. The dedicated interface is preserved so that each lister can have its own set of methods (e.g. the method returning the namespaced lister if appropriate), and the dedicated struct is preserved to allow expansions to be defined where necessary.

The external interface is unchanged and doesn't expose generics.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The core of this change is https://github.com/kubernetes/kubernetes/pull/121574/files#diff-8b2fc7ac28a79479e5ffb0b072f0b5d1b308ff4cb29daa3ae0657e23d200ac46 and https://github.com/kubernetes/kubernetes/pull/121574/files#diff-08b8fe190a6fe29b3cb7708a75aa9e7256459c4f7d941854d76eae79862ff56f — the rest of the change is generated code.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[k8s-ci-robot]

Hi @skitt. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[skitt]

@thockin if you have the time I’m interested in your thoughts on this and #121439 — I understand you like generics but are also wary of their overuse.

[thockin]

I don't find this abusive AT ALL. I think this is exactly what generics are good for, and anything that reduces the LOC count of generated code is a win.

I'd love to see us explore a "v2" client which really leans into generics (even at the user-facing API) and uses less code generation. These codegen tools are crusty and hard to maintain and even harder to change (hence why my go-workspaces work has stalled).

[thockin]

comment and code don't match (type name)

[thockin]

@liggitt @jpbetz @deads2k

[skitt]

I'd love to see us explore a "v2" client which really leans into generics (even at the user-facing API) and uses less code generation. These codegen tools are crusty and hard to maintain and even harder to change (hence why my go-workspaces work has stalled).

Yes, I’ve been looking at two approaches, generifying the static client (without changing the external interface), and building a new client more along the lines of the dynamic client but using generics to produce concrete types instead of unstructured.Unstructured. The latter quickly runs into limitations inherent to (current) Go generics (e.g. not being able create an instance of the type parameter), so some codegen would still be required, but the resulting API would be useful in many different scenarios. One feature I would like, which I haven’t dug into much yet, would be to make lists generic (instead of StatefulSetList, have List[StatefulSet]), which would reduce the need for functions such as meta.ExtractList.

Go’s duck-typing means that clients can layer their own generic abstractions on top of K8s, e.g. submariner-io/admiral#756, but it would be nice to have a common layer everyone can use — and in a place where current limitations can be addressed (see the List handling in that PR; we still need to hard-code singleton-to-list pairs).

[skitt]

/ok-to-test

[seans3]

/triage accepted

[skitt]

I’ve updated the PR to address the linting issues, and also to leave client-go/generic free for a possible future generic client (alongside dynamic etc.). The helpers are moved into listers, and renamed to reflect their nature (for structs) and reduce stuttering (for constructors).

[thockin]

If this were broken into commits with "real work" and "regenerated code" it would help review.

Who is the right approver for this? I love the idea, but I am not deeply familiar with this area

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: dbbc5d6c9985e5c91887118dd54e6499fcff5267

[pacoxu]

/hold
for https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/batch/pull-kubernetes-verify/1780819179771793408

https://prow.k8s.io/?repo=kubernetes%2Fkubernetes&type=batch
/test pull-kubernetes-verify

[skitt]

Thanks @pacoxu, this does indeed need a last-minute codegen re-run before merging to make sure all the generated code is up-to-date.

@thockin this means the PR needs a new lgtm 😉

[pacoxu]

/unhold

[skitt]

/retest-required

[thockin]

I'm picking nits now.

[thockin]

Idea:

Why not just have 1 type:

ResourceIndexer[T runtime.Object] struct {
	indexer  cache.Indexer
	resource schema.GroupResource
	namespace string // empty for non-namespaced types
}

and then just test l.namespace for empty. One code path, one type. Two
constructors functions (New and NewNamespaced).

[skitt]

I thought I had tried that and run into issues, but apparently not; I’ve updated the PR to use a single type.

[skitt]

/retest-required

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b6224f7179f65c79182ad89dcb2b461e7c371df6

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skitt, thockin

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:

• staging/src/k8s.io/apiextensions-apiserver/OWNERS [thockin]
• staging/src/k8s.io/client-go/OWNERS [thockin]
• staging/src/k8s.io/code-generator/OWNERS [thockin]
• staging/src/k8s.io/kube-aggregator/OWNERS [thockin]
• staging/src/k8s.io/sample-apiserver/OWNERS [thockin]
• staging/src/k8s.io/sample-controller/OWNERS [thockin]

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

[k8s-ci-robot]

@skitt: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-gce	54e8993	link	unknown	/test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.