Add tooling to generate listers #35639
Conversation
ncdc
added
sig/api-machinery
| @@ -71,4 +73,21 @@ ${clientgen} --clientset-name=federation_internalclientset --clientset-path=k8s. | |||
| ${clientgen} --clientset-name=federation_release_1_5 --clientset-path=k8s.io/kubernetes/federation/client/clientset_generated --input="../../federation/apis/federation/v1beta1","api/v1","extensions/v1beta1" --included-types-overrides="api/v1/Service,api/v1/Namespace,extensions/v1beta1/ReplicaSet,api/v1/Secret,extensions/v1beta1/Ingress,extensions/v1beta1/Deployment,extensions/v1beta1/DaemonSet,api/v1/ConfigMap,api/v1/Event" "$@" | |||
| ${setgen} "$@" | |||
|
|
|||
| LISTERGEN_APIS=( | |||
There was a problem hiding this comment.
@deads2k I don't really like doing it this way, but I need to use the internal packages, not the versioned ones, and KUBE_AVAILABLE_GROUP_VERSIONS only contains the versioned packages. I'll gladly welcome any suggestions for how to improve this.
There was a problem hiding this comment.
@deads2k I don't really like doing it this way, but I need to use the internal packages, not the versioned ones, and KUBE_AVAILABLE_GROUP_VERSIONS only contains the versioned packages. I'll gladly welcome any suggestions for how to improve this.
based on the sig-api-machinery call, I suspect we want both internal an external.
There was a problem hiding this comment.
Especially if we have each GV for listers in its own package, this won't be an issue.
k8s-github-robot
added
the
size/XXL
| // PodDisruptionBudgetLister helps list PodDisruptionBudgets. | ||
| type PodDisruptionBudgetLister interface { | ||
| // List lists all PodDisruptionBudgets in the indexer. | ||
| List(selector labels.Selector) (ret []*policy.PodDisruptionBudget, err error) |
There was a problem hiding this comment.
This is cool. I think it will be easy modify lister-gen to generate List functions that return v1alpha1.PDB? I would like to include the versioned Listers in client-go.
There was a problem hiding this comment.
@caesarxuchao yeah, it should be. All the generated files for a single invocation of lister-gen go into the same package, and it's configurable, so we could run it multiple times for each set of apis we want to group together. Or we could decide that each api group gets its listers in an appropriately named package, per group. WDYT?
There was a problem hiding this comment.
From my perspective, I'd like to keep versioned and internalversioned listers in different packages, like how we organize clientset. That makes porting to client-go easy, because client-go should only expose versioned APIs.
There was a problem hiding this comment.
How do you handle batch/v1 and batch/v2alpha1? You can't put listers for both of those api groups into the same package.
There was a problem hiding this comment.
Both are generated (BatchV1 and BatchV2alpha1, there's also Batch which picks the preferred version but that's deprecated, already) , user needs to pick one explicitly, see #35471.
There was a problem hiding this comment.
@soltysh right, but both v1 and v2alpha1 have a Job, so I can't generate a JobLister for both GVs in the same package.
There was a problem hiding this comment.
I just realized that probably v2alpha1.Job is not being used anywhere and you can do anything with it :/
| var typeLister_NonNamespacedGet = ` | ||
| // Get retrieves the $.type|public$ from the index for a given name. | ||
| func (s *$.type|private$Lister) Get(name string) (*$.type|raw$, error) { | ||
| key := &$.type|raw${ObjectMeta: api.ObjectMeta{Name: name}} |
There was a problem hiding this comment.
I discovered that I can't blindly use api.ObjectMeta because the package won't always be api. I need to find a way to consistently pick the right package for ObjectMeta for each type in question. For right now, it's either api or v1, but at some point, if we have v2.ObjectMeta, I'd love to be able to support that w/o having to modify lister-gen after the fact.
There was a problem hiding this comment.
I have a local fix for this 😄
|
Can we separate the output to |
|
@deads2k yes, we can. Just waiting on agreement. |
|
I like the output. |
It sounds good to me. It will solve the |
|
Ok, sold. I will separate by group and version. |
|
Is the plan to put the internal one under |
|
For v1 pods, we'd find them in For internal pods, options could be:
|
|
I like |
|
I like |
|
I can update #35471 to use "internal", if that's what people agree on. |
|
internal sounds better to me |
|
I remembered golang reserves "internal" package, it means something different. See: https://docs.google.com/document/d/1e8kOo3r51b2BWtTs_1uADIA5djfXhPT36s6eHVRIvaU/edit |
|
I remember that now. I still think "internalversion" doesn't sound great, On Wed, Oct 26, 2016 at 6:53 PM, Chao Xu notifications@github.com wrote:
|
| if extractBoolTagOrDie("genclient", t.SecondClosestCommentLines) == false { | ||
| continue | ||
| } | ||
| if _, found := gvToTypes[gv]; !found { |
| var packageList generator.Packages | ||
|
|
||
| orderer := namer.Orderer{Namer: namer.NewPrivateNamer(0)} | ||
| for i := range groupVersions { |
| type PetSetLister interface { | ||
| // List lists all PetSets in the indexer. | ||
| List(selector labels.Selector) (ret []*apps.PetSet, err error) | ||
| // PetSets returns an object that can list and get PetSets. |
|
I'm refactoring what files this generates - hold off for reviewing for now |
|
Does this list of generated files look ok? |
|
The generated files lgtm. Regarding whether having |
We'll need the interfaces at compile/editing time or our searches won't work well. |
|
Any other comments? |
deads2k
added
the
lgtm
|
lgtm |
|
Jenkins unit/integration failed for commit d270474. Full PR test history. The magic incantation to run this job again is |
|
Jenkins verification failed for commit d270474. Full PR test history. The magic incantation to run this job again is |
|
still lgtm |
|
Thanks @ncdc. |
k8s-github-robot
removed
the
lgtm
ncdc
added
the
lgtm
|
Any file named zz_generated.* should be created "on demand" as part of the On Mon, Oct 31, 2016 at 7:39 PM, Kubernetes Submit Queue <
|
|
Jenkins GKE smoke e2e failed for commit 13abf36. Full PR test history. The magic incantation to run this job again is |
|
Jenkins GCE Node e2e failed for commit 13abf36. Full PR test history. The magic incantation to run this job again is |
Meaning a) we should remove zz_generated from these files and still commit them, or b) we should leave zz_generated and not check them in?
It's related to client-gen, which is also not plugged into Makefile. Do you want to ultimately get rid of |
|
Infra flakes @k8s-bot cvm gke e2e test this |
|
Infra flakes @k8s-bot node e2e test this |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
|
client-go needs to copy these files from the main repo, so we do need to commit these files, at least until client-go becomes independent of the main repo, so we shouldn't include |
|
On Tue, Nov 1, 2016 at 3:14 PM, Andy Goldstein notifications@github.com wrote:
Meaning you should use a different name until they are generated on
If this generator can not reasonably be tied into the build yet, |
Add lister-gen tool to auto-generate listers. So far this PR only demonstrates replacing the manually-written
StoreToLimitRangeListerwith the generatedLimitRangeLister, as it's a small and easy swap.cc @deads2k @liggitt @sttts @nikhiljindal @lavalamp @smarterclayton @derekwaynecarr @kubernetes/sig-api-machinery @kubernetes/rh-cluster-infra
This change is