Allow to extend generated clients with custom verbs #16019

mfojtik · 2017-08-28T20:40:35Z

We need this in order to generate client calls like .Instantiate() or .Secrets() (for image stream secrets).
In upstream, this will allow to generate UpdateStatus which is currently "guessed" by checking for the presence of ".Status" field in the resource. It also allows to generate custom .Scale(...) in autoscaler.

Additionally this allows to send requests to sub-resources but also allows to override the input and result types for existing client calls or for extended client calls. The extended client calls are generated to the main client interface.

The final syntax is:

//+genclient:method=Instantiate,verb=create,subresource=instantiate,input=DeploymentRequest,output=DeploymentConfig

(the input/result types will allow to provide full import path, see the checklist)

TODO

Finalize the tag syntax
Allow cross-group type references for input/result type overrides
Fix the fake generator
Move this PR upstream and demonstrate this on .Scale() and .UpdateStatus()

mfojtik · 2017-08-28T20:45:15Z

@sttts @deads2k not ideal but somewhat working...

I tried to avoid copying the templates because I hate having the same function bodies templates copied into two places (although for sub-resources the body is different). I might end up add some conditional to original template to add sub resource
The string.Replace() is clunky but I dunno if there is a better way without rewriting the the original templates (maybe I should do that)

This need more work, but want to share WIP to get some early feedback...
(also this should probably be opened upstream, but I would like to have this downstream first to unblock our effort to remove legacy client).

mfojtik · 2017-08-28T20:46:19Z

pkg/deploy/apis/apps/types.go

@@ -123,6 +123,7 @@ const (
 )

 // +genclient
+// +genclient:createInstantiateVerb=subresource=instantiate,input=DeploymentRequest


This only works if DeploymentRequest is in the same package (does not work for external types or kapi / etc.) ...

This only works if DeploymentRequest is in the same package (does not work for external types or kapi / etc.)

One our our use-cases is Scale (in autoscaling) and another is ImageSecrets or some such (in the core API group). Could we provide a full package name or something?

bparees · 2017-08-28T20:46:30Z

/unassign

sttts · 2017-08-29T07:51:01Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/util/tags.go

+func parseClientExpansions(tags map[string][]string) ([]expansion, error) {
+	ret := []expansion{}
+	for name, values := range tags {
+		if !strings.HasPrefix(name, "genclient:") || !strings.HasSuffix(name, "Verb") {


"genclient:" is worth a constant

sttts · 2017-08-29T07:52:31Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/util/tags.go

+		name = strings.TrimPrefix(name, "genclient:")
+		exp := expansion{}
+		for _, supportedVerb := range SupportedVerbs {
+			if strings.HasPrefix(name, supportedVerb) {


this looks fuzzy. Can't we be more precise with matching?

@sttts at this point we only matched everything that starts with the supported verb and has *Verb suffix. Everything between is considered as expansion verb name. Dunno if there is a better way other than regexp.

sttts · 2017-08-29T07:53:56Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/util/tags.go

+			continue
+		}
+		exp.VerbName = strings.TrimSuffix(strings.TrimPrefix(name, exp.VerbType), "Verb")
+		if len(values[0]) > 0 {


are we sure there are values?

do I remember right that multiple tag line give you multiple values here?

yes, if you repeat the same tag (same verb) with different configuration it will give you multiple values... i'm not sure if we want to support it... something like this looks like something we don't want to recommend?

// +genclient:createFooVerb=subresource=foo // +genclient:createFooVerb=output=Foo

nvmd. this actually works well if I move this into a for _, v := range values {} loop... so people can use this in multi-line if they really want

sttts · 2017-08-29T07:58:30Z

vendor/k8s.io/kubernetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/util/tags.go

 	return ret, validateClientGenTags(values)
 }

+func parseClientExpansions(tags map[string][]string) ([]expansion, error) {
+	ret := []expansion{}


var ret []expansion. Then you don't need the if at the bottom.

mfojtik · 2017-08-29T11:17:11Z

@sttts @deads2k PTAL

This generates what we need and we can use this to produce updateStatus as well (which is a sub-resource) and also the Scale() for autoscaling...

This is missing the fake client (it won't conform the interface right now, but this is also broken with normal expansions, I will try to fix it).

mfojtik · 2017-08-29T13:29:13Z

@sttts syntax updated :)

mfojtik · 2017-08-29T19:37:31Z

@deads2k @sttts finalized this (fake generate fake methods, cross-imports works, etc). This should now have everything OpenShift need to add missing methods, I will start moving this upstream tomorrow.

PTAL generated methods and the generator changes.

mfojtik · 2017-08-29T19:38:08Z

...rnetes/staging/src/k8s.io/kube-gen/cmd/client-gen/generators/fake/generator_fake_for_type.go

 	obj, err := c.Fake.
-		$if .namespaced$Invokes($.NewListAction|raw$($.type|allLowercasePlural$Resource, $.type|allLowercasePlural$Kind, c.ns, opts), &$.type|raw$List{})
-		$else$Invokes($.NewRootListAction|raw$($.type|allLowercasePlural$Resource, $.type|allLowercasePlural$Kind, opts), &$.type|raw$List{})$end$
+		$if .namespaced$Invokes($if .subresource$$.NewListSubresourceAction|raw$$else$$.NewListAction|raw$$end$($.type|allLowercasePlural$Resource, $if .subresource$"$.subresourcePath$",$end$$.type|allLowercasePlural$Kind, c.ns, opts), &$.resultType|raw$List{})


@sttts this is hell.

mfojtik · 2017-08-30T17:14:15Z

(no need to api review, this is just adding genclient tags, not modifying API)

dobbymoodge · 2017-08-30T18:29:48Z

/retest

mfojtik · 2017-08-30T18:34:51Z

@Kargakis seems like the @openshift-merge-robot doing crazy things :)

0xmichalis · 2017-08-30T18:54:18Z

@Kargakis seems like the @openshift-merge-robot doing crazy things :)

It actually is doing the right thing: adding the approved label because you removed it and the PR is approved, and adding the needs-api-review label because again you removed it. :) The SQ will not block on needs-api-review. We can improve the path-label munger in the future to add labels only on PR creation which will allow us to remove them afterwards.

mfojtik · 2017-08-31T09:22:40Z

/test extended_conformance_gce

…loyment config

mfojtik · 2017-08-31T13:16:10Z

/test cmd

seems like server failed to start?

mfojtik · 2017-08-31T14:01:37Z

/test extended_conformance_gce

mfojtik · 2017-08-31T20:22:20Z

/retest

sttts · 2017-09-01T09:21:47Z

/lgtm

If there are changes in the upstream PR, it shouldn't be hard to backport or wait for the next rebase.

openshift-merge-robot · 2017-09-01T09:22:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, sttts

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

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

~~OWNERS~~ [mfojtik]

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

openshift-merge-robot · 2017-09-01T20:03:12Z

Automatic merge from submit-queue

mfojtik added the do-not-merge label Aug 28, 2017

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2017

openshift-merge-robot assigned bparees and 0xmichalis Aug 28, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Aug 28, 2017

mfojtik commented Aug 28, 2017

View reviewed changes

openshift-ci-robot unassigned bparees Aug 28, 2017

mfojtik mentioned this pull request Aug 29, 2017

WIP: use generated clientset for deployment controllers and add expansion for instantiate #15981

Closed

sttts reviewed Aug 29, 2017

View reviewed changes

0xmichalis assigned sttts and unassigned 0xmichalis Aug 29, 2017

mfojtik force-pushed the codegen-expansion-tags branch from edf8d47 to 1f01937 Compare August 29, 2017 11:15

mfojtik force-pushed the codegen-expansion-tags branch from 1f01937 to e0a027a Compare August 29, 2017 12:14

mfojtik changed the title ~~WIP: Add expansion generator to client-gen~~ WIP: Allow to extended generate clients with custom verbs Aug 29, 2017

mfojtik force-pushed the codegen-expansion-tags branch from e0a027a to a2d4d48 Compare August 29, 2017 13:28

mfojtik force-pushed the codegen-expansion-tags branch from a2d4d48 to 89245a7 Compare August 29, 2017 14:14

openshift-merge-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 29, 2017

mfojtik force-pushed the codegen-expansion-tags branch from 89245a7 to f982838 Compare August 29, 2017 19:35

mfojtik commented Aug 29, 2017

View reviewed changes

mfojtik force-pushed the codegen-expansion-tags branch from f982838 to a95673f Compare August 30, 2017 11:32

mfojtik removed the needs-api-review label Aug 30, 2017

openshift-merge-robot added the needs-api-review label Aug 30, 2017

mfojtik added api-approved and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Aug 30, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2017

mfojtik added 3 commits August 31, 2017 14:35

UPSTREAM: 51638: allow to generate extended methods in client-go

b0e0d3f

apps: add Instantiate, GetScale and UpdateScale client method for dep…

d3fb931

…loyment config

image: add image stream secrets client

c087b7d

mfojtik force-pushed the codegen-expansion-tags branch from 40a369a to c087b7d Compare August 31, 2017 12:35

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2017

mfojtik mentioned this pull request Sep 1, 2017

registry: use generated imagestream clientset to retrieve secrets #16098

Merged

openshift-merge-robot merged commit e765e9c into openshift:master Sep 1, 2017

mfojtik deleted the codegen-expansion-tags branch September 5, 2018 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to extend generated clients with custom verbs #16019

Allow to extend generated clients with custom verbs #16019

mfojtik commented Aug 28, 2017 •

edited

Loading

mfojtik commented Aug 28, 2017

mfojtik Aug 28, 2017

deads2k Aug 29, 2017

bparees commented Aug 28, 2017

sttts Aug 29, 2017

sttts Aug 29, 2017

mfojtik Aug 29, 2017

sttts Aug 29, 2017

sttts Aug 29, 2017

mfojtik Aug 29, 2017

mfojtik Aug 29, 2017

sttts Aug 29, 2017

mfojtik commented Aug 29, 2017

mfojtik commented Aug 29, 2017

mfojtik commented Aug 29, 2017

mfojtik Aug 29, 2017

mfojtik commented Aug 30, 2017

dobbymoodge commented Aug 30, 2017

mfojtik commented Aug 30, 2017

0xmichalis commented Aug 30, 2017

mfojtik commented Aug 31, 2017

mfojtik commented Aug 31, 2017

mfojtik commented Aug 31, 2017

mfojtik commented Aug 31, 2017

sttts commented Sep 1, 2017

openshift-merge-robot commented Sep 1, 2017

openshift-merge-robot commented Sep 1, 2017

Allow to extend generated clients with custom verbs #16019

Allow to extend generated clients with custom verbs #16019

Conversation

mfojtik commented Aug 28, 2017 • edited Loading

TODO

mfojtik commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Aug 28, 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

mfojtik commented Aug 29, 2017

mfojtik commented Aug 29, 2017

mfojtik commented Aug 29, 2017

Choose a reason for hiding this comment

mfojtik commented Aug 30, 2017

dobbymoodge commented Aug 30, 2017

mfojtik commented Aug 30, 2017

0xmichalis commented Aug 30, 2017

mfojtik commented Aug 31, 2017

mfojtik commented Aug 31, 2017

mfojtik commented Aug 31, 2017

mfojtik commented Aug 31, 2017

sttts commented Sep 1, 2017

openshift-merge-robot commented Sep 1, 2017

openshift-merge-robot commented Sep 1, 2017

mfojtik commented Aug 28, 2017 •

edited

Loading