Fixes fake client generation for non-namespaced subresources #60445

jhorwit2 · 2018-02-26T19:12:15Z

What this PR does / why we need it:

Fixes code generation for non-namespaced subresources fake clients.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #60444

Special notes for your reviewer:

Release note:

Fixes fake client generation for non-namespaced subresources

/cc @mfojtik @liggitt

I'm not sure the best way to add tests for this. Any pointers?

liggitt · 2018-02-27T05:32:49Z

staging/src/k8s.io/client-go/testing/actions.go

@@ -95,6 +105,19 @@ func NewListSubresourceAction(resource schema.GroupVersionResource, name, subres
 	return action
 }

+func NewRootListSubresourceAction(resource schema.GroupVersionResource, name, subresource string, kind schema.GroupVersionKind, opts interface{}) ListActionImpl {


how do you list a subresource? subresources are always under a single resource

Good question 🤔. The only reason I added this was because the template was wrong. I can remove it as well. Should I do that in this PR or a separate one?

here please.

liggitt · 2018-02-27T05:34:10Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/fake/generator_fake_for_type.go

@@ -327,7 +330,7 @@ var listSubresourceTemplate = `
 func (c *Fake$.type|publicPlural$) List($.type|private$Name string, opts $.ListOptions|raw$) (result *$.resultType|raw$List, err error) {


listSubresourceTemplate seems questionable, since subresource is always on a get

mbohlool · 2018-03-01T21:25:30Z

/cc @roycaihw @jennybuckley

roycaihw · 2018-03-01T22:03:38Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/fake/generator_fake_for_type.go

@@ -423,7 +426,7 @@ var createSubresourceTemplate = `
 func (c *Fake$.type|publicPlural$) Create($.type|private$Name string, $.inputType|private$ *$.inputType|raw$) (result *$.resultType|raw$, err error) {
 	obj, err := c.Fake.
 		$if .namespaced$Invokes($.NewCreateSubresourceAction|raw$($.type|allLowercasePlural$Resource, $.type|private$Name, "$.subresourcePath$", c.ns, $.inputType|private$), &$.resultType|raw${})
-		$else$Invokes($.NewRootCreateAction|raw$($.inputType|allLowercasePlural$Resource, $.inputType|private$), &$.resultType|raw${})$end$
+		$else$Invokes($.NewRootCreateSubresourceAction|raw$($.type|allLowercasePlural$Resource, "$.subresourcePath$", $.inputType|private$), &$.inputType|raw${})$end$


s/&$.inputType|raw${})$end$/&$.resultType|raw${})$end$ ?

roycaihw · 2018-03-01T22:07:58Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/fake/generator_fake_for_type.go

@@ -327,7 +330,7 @@ var listSubresourceTemplate = `
 func (c *Fake$.type|publicPlural$) List($.type|private$Name string, opts $.ListOptions|raw$) (result *$.resultType|raw$List, err error) {
 	obj, err := c.Fake.
 		$if .namespaced$Invokes($.NewListSubresourceAction|raw$($.type|allLowercasePlural$Resource, $.type|private$Name, "$.subresourcePath$", $.type|allLowercasePlural$Kind, c.ns, opts), &$.resultType|raw$List{})
-		$else$Invokes($.NewRootListAction|raw$($.type|allLowercasePlural$Resource, $.type|allLowercasePlural$Kind, opts), &$.resultType|raw$List{})$end$
+		$else$Invokes($.NewRootListSubresourceAction|raw$($.type|allLowercasePlural$Resource, $.type|private$Name), &$.resultType|raw${})$end$


missed $List{} in the end. but agree with @liggitt, subresource doesn't support list

sttts · 2018-04-06T09:30:38Z

/cc @nikhita

sttts · 2018-04-06T09:34:58Z

@jhorwit2 can you update the PR so we can get it in?

jhorwit2 · 2018-04-06T11:26:11Z

I'll get to it today @sttts! Sorry about the delay.

roycaihw · 2018-04-06T17:39:40Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/fake/generator_fake_for_type.go

@@ -449,7 +434,7 @@ var updateSubresourceTemplate = `
 func (c *Fake$.type|publicPlural$) Update($.type|private$Name string, $.inputType|private$ *$.inputType|raw$) (result *$.resultType|raw$, err error) {
 	obj, err := c.Fake.
 		$if .namespaced$Invokes($.NewUpdateSubresourceAction|raw$($.type|allLowercasePlural$Resource, "$.subresourcePath$", c.ns, $.inputType|private$), &$.inputType|raw${})
-		$else$Invokes($.NewRootUpdateAction|raw$($.type|allLowercasePlural$Resource, $.type|private$), &$.type|raw${})$end$
+		$else$Invokes($.NewRootUpdateSubresourceAction|raw$($.type|allLowercasePlural$Resource, "$.subresourcePath$", $.inputType|private$), &$.resultType|raw${})$end$


could you explain why the namespaced one (the line above) and the non-namespaced one (this line) differ at the end (&$.inputType|raw${}) vs &$.resultType|raw${}))?

it's a bug :)

It should be fixed in another PR

Because the result type is always the same as the input type currently, so although it's not technically correct it won't break until you have a scenario where the update call returns a different object.

Ex:
//+genclient:method=UpdateScale,verb=update,subresource=scale,input=k8s.io/kubernetes/pkg/apis/autoscaling.Scale,result=k8s.io/kubernetes/pkg/apis/autoscaling.Scale

jhorwit2 · 2018-04-06T17:45:29Z

Is anyone aware of a way I can add some tests for this? This is my first time messing w/ the code gen and don't see anything obvious.

sttts · 2018-04-09T10:30:48Z

@jhorwit2 usually the are changes in the generated code. Why not here?

jhorwit2 · 2018-04-09T17:48:55Z

@sttts Nothing in core uses a cluster scoped object with subresources besides /status which has special generation code (still not sure why but that's another convo). The reason I encountered this is because I have a cluster scoped custom object that I wanted to have /scale subresource.

sttts · 2018-04-10T07:59:18Z

@jhorwit2 can you add an example to https://github.com/kubernetes/code-generator/tree/master/_examples ?

jhorwit2 · 2018-04-19T15:05:21Z

@sttts Done! I added a cluster scoped resource with a scale sub-resource.

sttts · 2018-04-26T12:21:57Z

Can you squash?

jhorwit2 · 2018-04-26T18:59:22Z

Done.

sttts · 2018-05-07T12:08:50Z

staging/src/k8s.io/code-generator/cmd/client-gen/generators/fake/generator_fake_for_type.go

@@ -146,9 +146,10 @@ func (g *genFakeForType) GenerateType(c *generator.Context, t *types.Type, w io.
 		"NewRootWatchAction":             c.Universe.Function(types.Name{Package: pkgClientGoTesting, Name: "NewRootWatchAction"}),
 		"NewWatchAction":                 c.Universe.Function(types.Name{Package: pkgClientGoTesting, Name: "NewWatchAction"}),
 		"NewCreateSubresourceAction":     c.Universe.Function(types.Name{Package: pkgClientGoTesting, Name: "NewCreateSubresourceAction"}),
+		"NewRootCreateSubresourceAction": c.Universe.Function(types.Name{Package: pkgClientGoTesting, Name: "NewRootCreateSubresourceAction"}),


Do we use the term "root" elsewhere for cluster wide resources? I think it's either "cluster" or "NoneNamespaced".

@sttts I was following the convention that already existed in the generation code. I'd prefer handling a rename like that in another PR.

Right, "Root" is already used. So it's fine.

To answer your question though, i've never seen it used else where in the code. I was a little confused at first :)

sttts · 2018-05-08T09:13:17Z

/approve
/lgtm

k8s-ci-robot · 2018-05-08T09:13:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/client-go/OWNERS~~ [sttts]
~~staging/src/k8s.io/code-generator/OWNERS~~ [sttts]

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

fejta-bot · 2018-05-08T12:25:07Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot · 2018-05-08T14:33:07Z

@jhorwit2: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	57259db5efe7c909697d313bad6ef4a76a167cf5	link	`/test pull-kubernetes-unit`

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.

fejta-bot · 2018-05-08T16:37:09Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-github-robot · 2018-05-08T20:16:16Z

Automatic merge from submit-queue (batch tested with PRs 63291, 63490, 60445, 63507, 63524). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested a review from liggitt February 26, 2018 19:12

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 26, 2018

k8s-ci-robot requested a review from mfojtik February 26, 2018 19:12

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 26, 2018

jhorwit2 force-pushed the for/upstream/master/60444 branch 2 times, most recently from 02f30dc to 57259db Compare February 26, 2018 19:17

liggitt reviewed Feb 27, 2018

View reviewed changes

k8s-ci-robot requested review from jennybuckley and roycaihw March 1, 2018 21:25

roycaihw reviewed Mar 1, 2018

View reviewed changes

k8s-ci-robot requested a review from nikhita April 6, 2018 09:30

jhorwit2 force-pushed the for/upstream/master/60444 branch from 57259db to b284718 Compare April 6, 2018 13:52

roycaihw reviewed Apr 6, 2018

View reviewed changes

jhorwit2 force-pushed the for/upstream/master/60444 branch from b284718 to b60c32a Compare April 19, 2018 14:57

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 19, 2018

Fixes fake client generation for non-namespaced subresources

0f62a8c

jhorwit2 force-pushed the for/upstream/master/60444 branch from b60c32a to 0f62a8c Compare April 26, 2018 18:59

sttts reviewed May 7, 2018

View reviewed changes

k8s-ci-robot assigned sttts May 8, 2018

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 8, 2018

k8s-github-robot merged commit 8203d35 into kubernetes:master May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes fake client generation for non-namespaced subresources #60445

Fixes fake client generation for non-namespaced subresources #60445

jhorwit2 commented Feb 26, 2018 •

edited by sttts

Loading

liggitt Feb 27, 2018

jhorwit2 Feb 27, 2018 •

edited

Loading

sttts Apr 6, 2018

liggitt Feb 27, 2018

mbohlool commented Mar 1, 2018

roycaihw Mar 1, 2018

roycaihw Mar 1, 2018

sttts commented Apr 6, 2018

sttts commented Apr 6, 2018

jhorwit2 commented Apr 6, 2018

roycaihw Apr 6, 2018

jhorwit2 Apr 6, 2018 •

edited

Loading

jhorwit2 commented Apr 6, 2018

sttts commented Apr 9, 2018

jhorwit2 commented Apr 9, 2018

sttts commented Apr 10, 2018

jhorwit2 commented Apr 19, 2018

sttts commented Apr 26, 2018

jhorwit2 commented Apr 26, 2018

sttts May 7, 2018

jhorwit2 May 7, 2018

sttts May 8, 2018

jhorwit2 May 8, 2018

sttts commented May 8, 2018

k8s-ci-robot commented May 8, 2018

fejta-bot commented May 8, 2018

k8s-ci-robot commented May 8, 2018 •

edited

Loading

fejta-bot commented May 8, 2018

k8s-github-robot commented May 8, 2018

		@@ -327,7 +330,7 @@ var listSubresourceTemplate = `
		func (c Fake$.type\|publicPlural$) List($.type\|private$Name string, opts $.ListOptions\|raw$) (result $.resultType\|raw$List, err error) {

Fixes fake client generation for non-namespaced subresources #60445

Fixes fake client generation for non-namespaced subresources #60445

Conversation

jhorwit2 commented Feb 26, 2018 • edited by sttts Loading

Choose a reason for hiding this comment

jhorwit2 Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbohlool commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Apr 6, 2018

sttts commented Apr 6, 2018

jhorwit2 commented Apr 6, 2018

Choose a reason for hiding this comment

jhorwit2 Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

jhorwit2 commented Apr 6, 2018

sttts commented Apr 9, 2018

jhorwit2 commented Apr 9, 2018

sttts commented Apr 10, 2018

jhorwit2 commented Apr 19, 2018

sttts commented Apr 26, 2018

jhorwit2 commented Apr 26, 2018

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

sttts commented May 8, 2018

k8s-ci-robot commented May 8, 2018

fejta-bot commented May 8, 2018

k8s-ci-robot commented May 8, 2018 • edited Loading

fejta-bot commented May 8, 2018

k8s-github-robot commented May 8, 2018

jhorwit2 commented Feb 26, 2018 •

edited by sttts

Loading

jhorwit2 Feb 27, 2018 •

edited

Loading

jhorwit2 Apr 6, 2018 •

edited

Loading

k8s-ci-robot commented May 8, 2018 •

edited

Loading