Unify genclient tags and add more fine control on verbs generated #49192

mfojtik · 2017-07-19T09:03:06Z

This will change the syntax of the existing genclient tags be like this:

// +genclient
// +genclient:noStatus
// +genclient:noVerbs
// +genclient:nonNamespaced
// +genclient:readonly

The first one indicates the client will be generated from the struct below and the other tags are basically options to the genclient (which justify why they should be prefixed with genclient:)

This also changes the // +genclientstatus=false to // +genclient:noStatus to follow the pattern and also changes the // +noMethods=true to // +genclient:noVerbs as we call the REST operations verbs so it will make it consistent with terminology.

In addition to existing options this patch also add two more to allow more fine-grained control on which verbs are going to be generated. This is extra useful for third-party projects (like OpenShift) where some resources does not implement full CRUD, but for example just "create" verb or "create" and "delete"...
To support that, you can use this syntax:

// +genclient:onlyVerbs=create,delete
// +genclient:skipVerbs=patch

The first one will generate only create and delete functions and second one will generate full CRUD without "patch" actions. This somehow overlaps with the existing "readonly" tag, but I want to keep that tag in place as it reads better in some cases ;-)

mfojtik · 2017-07-19T09:04:10Z

@deads2k @sttts PTAL

This is based on discussion in openshift/origin#15236

sttts · 2017-07-19T09:05:53Z

Looks good.

sttts · 2017-07-19T09:06:16Z

/cc @kubernetes/sig-api-machinery-api-reviews

deads2k · 2017-07-19T11:48:35Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_type.go

 	}

-	if !noMethods {
+	if !tags.NoVerbs {
 		sw.Do(resource, m)
 		sw.Do(kind, m)
 	}



Can we actually return here if its tags.NoVerbs?

deads2k · 2017-07-19T11:49:26Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_type.go

 	}

-	if !noMethods && !readonly {
-		sw.Do(patchTemplate, m)
+	if !tags.NoVerbs && !tags.Readonly {


why is this if statement appearing again? It looks the same as above

yeah, it makes sure the Patch() will be the last method (i can move it upper but that will reorder the methods... not sure if we care about that)

deads2k · 2017-07-19T11:49:50Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_type.go

-			sw.Do(listUsingOptionsTemplate, m)
-		} else {
-			sw.Do(listTemplate, m)
+	if !tags.NoVerbs {


returning early makes this block unconditional and early.

deads2k · 2017-07-19T11:50:07Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_type.go

-		sw.Do(updateTemplate, m)
-		// Generate the UpdateStatus method if the type has a status
-		if genStatus(t) {
+	if !tags.NoVerbs && !tags.Readonly {


After you generate getters, you can check if its readonly and just return, right?

deads2k · 2017-07-19T11:52:10Z

cmd/libs/go2idl/client-gen/generators/generator_for_type.go


-	readonly := extractBoolTagOrDie("readonly", t.SecondClosestCommentLines) == true
+	if len(tags.OnlyVerbs) > 0 {


This block looks familiar. Why wouldn't this be done in the parse step?

deads2k · 2017-07-19T11:52:55Z

cmd/libs/go2idl/client-gen/generators/generator_for_type.go

 	}

 	return sw.Error()
 }

+func skipVerb(m string, skippedVerbs []string) bool {


this is starting to look like a helper on the tags. Second time I've seen it.

yeah I will move this to util

deads2k · 2017-07-19T11:55:11Z

cmd/libs/go2idl/lister-gen/generators/expansion.go

@@ -41,10 +43,14 @@ func (g *expansionGenerator) Filter(c *generator.Context, t *types.Type) bool {
 func (g *expansionGenerator) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error {
 	sw := generator.NewSnippetWriter(w, c, "$", "$")
 	for _, t := range g.types {
+		tags, err := util.ParseClientGenTags(t.SecondClosestCommentLines)
+		if err != nil {
+			fmt.Printf("Error parsing tags: %v", err)


if we can't parse, our return code should be non-zero. This seems to hide that.

deads2k · 2017-07-19T11:56:46Z

I'm definitely in favor. I have a few minor comments and one concern (spread throughout the pull) about error handling.

mfojtik · 2017-07-19T12:42:35Z

@deads2k addressed all comments which also caused all clients to regenerate and reorder the functions :-) somebody should probably double-check that the generated clients are correct as I can have bug in the logic but it is hard to tell :-)

deads2k · 2017-07-19T12:44:45Z

cmd/libs/go2idl/client-gen/generators/fake/generator_fake_for_type.go

 	if tags.NonNamespaced {
 		sw.Do(structNonNamespaced, m)
 	} else {
 		sw.Do(structNamespaced, m)
 	}

-	if !tags.NoVerbs {
+	if tags.NoVerbs {


This conditional switched. Need an if/else?

deads2k · 2017-07-19T12:47:19Z

One comment on an if/else block. otherwise lgtm.

mfojtik · 2017-07-19T12:50:20Z

@deads2k updated and regenerated

deads2k · 2017-07-24T16:54:10Z

@mfojtik doesn't build.

caesarxuchao · 2017-07-24T17:12:35Z

...regator/pkg/client/clientset_generated/clientset/typed/apiregistration/v1beta1/apiservice.go

@@ -57,6 +57,38 @@ func newAPIServices(c *ApiregistrationV1beta1Client) *aPIServices {
 	}
 }

+// Get takes name of the aPIService, and returns the corresponding aPIService object, and an error if there is any.


pkg/client/clientset_generated/clientset is removed in HEAD. Could you rebase?

done, but still failing to compile...

Sorry, i didn't notice this file is in k8s.io/aggregator. Github hid the path. Please keep it.

Please only remove k8s.io/kubernetes/pkg/client/clientset_generated/clientset, if there is any.

mfojtik · 2017-07-24T20:14:51Z

@deads2k @sttts

# k8s.io/kubernetes/vendor/k8s.io/client-go/informers/apps/v1beta2
vendor/k8s.io/client-go/informers/apps/v1beta2/scale.go:48: client.AppsV1beta2().Scales("k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".NamespaceAll).List undefined (type "k8s.io/kubernetes/vendor/k8s.io/client-go/kubernetes/typed/apps/v1beta2".ScaleInterface is interface with no methods)
vendor/k8s.io/client-go/informers/apps/v1beta2/scale.go:48: not enough arguments to return
	have (<T>)
	want (runtime.Object, error)
vendor/k8s.io/client-go/informers/apps/v1beta2/scale.go:51: client.AppsV1beta2().Scales("k8s.io/kubernetes/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".NamespaceAll).Watch undefined (type "k8s.io/kubernetes/vendor/k8s.io/client-go/kubernetes/typed/apps/v1beta2".ScaleInterface is interface with no methods)
vendor/k8s.io/client-go/informers/apps/v1beta2/scale.go:51: not enough arguments to return

i fixed the noMethods in types and regenerated stuff, but i'm getting this now...

caesarxuchao · 2017-07-24T20:22:31Z

@mfojtik could you take a look why scale.go is generated? HEAD doesn't have any scale.go in informers/

mfojtik · 2017-07-24T20:22:42Z

@caesarxuchao ^^ do you remember if scale.go was removed manually or something? i can't see the reason why that file is generated (the client is set to have no methods (iow. noVerbs)

mfojtik · 2017-07-24T20:22:50Z

lol :-)

mfojtik · 2017-07-24T20:37:17Z

@caesarxuchao ok I removed that file manually... seems like when I fixed the comment the file was not deleted automatically... lets see if the build pass now

mfojtik · 2017-07-25T07:42:02Z

@deads2k @sttts green again \o/

sttts · 2017-07-25T08:03:46Z

/lgtm

k8s-github-robot · 2017-07-25T08:04:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, mfojtik, sttts
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

Associated issue requirement bypassed by: deads2k

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-25T09:43:12Z

Automatic merge from submit-queue (batch tested with PRs 49498, 49192)

caesarxuchao · 2017-07-27T19:06:38Z

I opened issue #49737 to ask for help for updating client-gen doc.

cc @droot @pmorie @MHBauer FYI, I remember you guys used client-gen in service-catalog. You'll need to update the tags in your types.go.

Automatic merge from submit-queue (batch tested with PRs 15468, 15446) Pick kubernetes kube-gen changes This PR picks the refactorings that were done upstream to move the generators to kube-gen and updates the generators we use to accomodate the new import paths. This also removes the wrappers for lister-gen and informer-gen as we can just use the upstream generator directly (I hope). This will be a base where I need to land the kubernetes/kubernetes#49192

Automatic merge from submit-queue list supported genclient tags and fix the format of the genclient tag This depends on: kubernetes/kubernetes#51638 for the `// +genclient:method`. Fixes the syntax of the generator after kubernetes/kubernetes#49192

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017

mfojtik force-pushed the unify-clientgen-tags branch from 80056de to 10605a5 Compare July 19, 2017 09:03

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jul 19, 2017

k8s-github-robot assigned nikhiljindal and yujuhong Jul 19, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Jul 19, 2017

mfojtik force-pushed the unify-clientgen-tags branch 2 times, most recently from 1659459 to 5baf941 Compare July 19, 2017 10:15

mfojtik mentioned this pull request Jul 19, 2017

WIP: Allow to skip or whitelist methods client-gen generates openshift/origin#15236

Closed

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2017

deads2k reviewed Jul 19, 2017

View reviewed changes

mfojtik force-pushed the unify-clientgen-tags branch from 7920bbd to 2ef6da9 Compare July 19, 2017 12:41

deads2k reviewed Jul 19, 2017

View reviewed changes

mfojtik force-pushed the unify-clientgen-tags branch from 2ef6da9 to 692fd56 Compare July 19, 2017 12:50

mfojtik force-pushed the unify-clientgen-tags branch 2 times, most recently from 0b1ea05 to a3c8a57 Compare July 24, 2017 15:02

caesarxuchao reviewed Jul 24, 2017

View reviewed changes

mfojtik added 4 commits July 24, 2017 22:05

unify tag syntax for genclient tags and add onlyVerbs and skipVerbs

d7e3029

update tags in types for new genclient syntax

976488e

regenerate clients to pickup updated genclient:noStatus comment

dad4ee3

update staging clients

d8a4241

mfojtik force-pushed the unify-clientgen-tags branch from a3c8a57 to 06fdec3 Compare July 24, 2017 20:05

regenerate clients

e6be341

mfojtik force-pushed the unify-clientgen-tags branch from 06fdec3 to e6be341 Compare July 24, 2017 20:35

k8s-ci-robot assigned sttts Jul 25, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017

mfojtik mentioned this pull request Jul 25, 2017

Move the generators openshift/origin#15427

Closed

k8s-github-robot merged commit cce1c9b into kubernetes:master Jul 25, 2017

mfojtik mentioned this pull request Jul 25, 2017

Pick kubernetes kube-gen changes openshift/origin#15446

Merged

caesarxuchao mentioned this pull request Jul 27, 2017

Update client-gen doc #49737

Closed

mfojtik mentioned this pull request Aug 31, 2017

list supported genclient tags and fix the format of the genclient tag kubernetes/community#1008

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify genclient tags and add more fine control on verbs generated #49192

Unify genclient tags and add more fine control on verbs generated #49192

mfojtik commented Jul 19, 2017

mfojtik commented Jul 19, 2017

sttts commented Jul 19, 2017

sttts commented Jul 19, 2017

deads2k Jul 19, 2017

deads2k Jul 19, 2017

mfojtik Jul 19, 2017

deads2k Jul 19, 2017

deads2k Jul 19, 2017

deads2k Jul 19, 2017

deads2k Jul 19, 2017

mfojtik Jul 19, 2017

deads2k Jul 19, 2017

deads2k commented Jul 19, 2017

mfojtik commented Jul 19, 2017

deads2k Jul 19, 2017

mfojtik Jul 19, 2017

deads2k commented Jul 19, 2017

mfojtik commented Jul 19, 2017

deads2k commented Jul 24, 2017

caesarxuchao Jul 24, 2017

mfojtik Jul 24, 2017

caesarxuchao Jul 24, 2017

mfojtik commented Jul 24, 2017

caesarxuchao commented Jul 24, 2017

mfojtik commented Jul 24, 2017

mfojtik commented Jul 24, 2017

mfojtik commented Jul 24, 2017

mfojtik commented Jul 25, 2017

sttts commented Jul 25, 2017

k8s-github-robot commented Jul 25, 2017

k8s-github-robot commented Jul 25, 2017

caesarxuchao commented Jul 27, 2017


		readonly := extractBoolTagOrDie("readonly", t.SecondClosestCommentLines) == true
		if len(tags.OnlyVerbs) > 0 {

Unify genclient tags and add more fine control on verbs generated #49192

Unify genclient tags and add more fine control on verbs generated #49192

Conversation

mfojtik commented Jul 19, 2017

mfojtik commented Jul 19, 2017

sttts commented Jul 19, 2017

sttts commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 19, 2017

mfojtik commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 19, 2017

mfojtik commented Jul 19, 2017

deads2k commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Jul 24, 2017

caesarxuchao commented Jul 24, 2017

mfojtik commented Jul 24, 2017

mfojtik commented Jul 24, 2017

mfojtik commented Jul 24, 2017

mfojtik commented Jul 25, 2017

sttts commented Jul 25, 2017

k8s-github-robot commented Jul 25, 2017

k8s-github-robot commented Jul 25, 2017

caesarxuchao commented Jul 27, 2017