Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter #35806

bdbauer · 2016-10-28T18:05:15Z

Which issue this PR fixes (optional, in fixes #<issue number>(, #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #34856

DELETE requests can now pass in their DeleteOptions as a query parameter or a body parameter, rather than just as a body parameter.

This change is

smarterclayton · 2016-10-28T20:36:36Z

pkg/apiserver/resthandler.go

@@ -759,7 +759,7 @@ func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope,
 		ctx = api.WithNamespace(ctx, namespace)

 		options := &api.DeleteOptions{}
-		if checkBody {
+		if isGracefulDeletion {


Why change this?

The only place this function is called, the passed in value is "isGracefulDeleter". It just seemed like with the change, what we're checking is no longer whether or not to check the body, but whether or not to look for graceful deletion.

Totally fine with leaving it as checkBody.

Probably better for it to be allowsOptions (now that you support query parameters). We may have sub resources that support delete that have different api objects instead of DeleteOptions in the future and this would be one step towards that.

smarterclayton · 2016-10-28T20:37:27Z

pkg/apiserver/resthandler.go

@@ -781,6 +781,13 @@ func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope,
 					scope.err(fmt.Errorf("decoded object cannot be converted to DeleteOptions"), res.ResponseWriter, req.Request)
 					return
 				}
+			} else {
+				if values := req.Request.URL.Query(); len(values) > 0 {


please add a test to apiserver_test.go that verifies the 3 permutations (body+query, body, query)

Added, please take a look

bdbauer · 2016-10-29T00:08:44Z

@k8s-bot verify test this

smarterclayton · 2016-10-31T18:56:06Z

Changes look good, please squash once you get green and I'll label.

smarterclayton · 2016-10-31T18:56:25Z

Also please add a release note to the PR description.

bdbauer · 2016-11-01T17:05:24Z

@k8s-bot unit test this

maisem · 2016-11-01T17:25:40Z

@k8s-bot cvm gce e2e test this

smarterclayton · 2016-11-01T18:03:39Z

/lgtm

nikhiljindal · 2016-11-01T19:20:12Z

I am surprised that there is no swagger spec change in this PR.
Dont we document the allowed query params in our swagger spec?

cc @mbohlool

smarterclayton · 2016-11-01T19:35:13Z

We need to change the generation code in api_installer and potentially openapi.

smarterclayton · 2016-11-01T19:35:24Z

Good catch @nikhiljindal

mbohlool · 2016-11-01T22:45:17Z

I've looked into this. Look like some (most?) operations accept SomeOption also as query parameters. Because query parameters cannot be a type (should be simple type like string), it reads the Option type members and decode/encode them as a single query param. I don't see we add those to the spec anywhere, so I suggest this change is not doing worse than what we have by not adding query parameters. We should fix that for all operations.

smarterclayton · 2016-11-02T00:04:39Z

Don't we already do this in swagger for ListOptions? What's the difference?

On Nov 1, 2016, at 6:45 PM, mbohlool notifications@github.com wrote:

I've looked into this. Look like some (most?) operations accept SomeOption
also as query parameters. Because query parameters cannot be a type (should
be simple type like string), it reads the Option type members and
decode/encode them as a single query param. I don't see we add those to the
spec anywhere, so I suggest this change is not doing worse than what we
have by not adding query parameters. We should fix that for all operations.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#35806 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p53XvOlXsy6xq_ACbKvrYyHdDDeHks5q58EMgaJpZM4KjtBN
.

mbohlool · 2016-11-02T00:29:55Z

You are right. First I didn't see any ListOptions in swagger, but that is the point. addObjectParams in api_installer will do the trick. I suggest we do the same for DeleteOption.

k8s-ci-robot · 2016-11-02T21:21:03Z

Jenkins GCE Node e2e failed for commit 5e6c034633aa858b33fe9df9bfe2ae742bb1616c. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-02T21:26:16Z

Jenkins unit/integration failed for commit 5e6c034633aa858b33fe9df9bfe2ae742bb1616c. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-02T21:34:52Z

Jenkins Kubemark GCE e2e failed for commit 5e6c034633aa858b33fe9df9bfe2ae742bb1616c. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-02T21:36:15Z

Jenkins GCI GCE e2e failed for commit 5e6c034633aa858b33fe9df9bfe2ae742bb1616c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-02T21:41:06Z

Jenkins GCE etcd3 e2e failed for commit 5e6c034633aa858b33fe9df9bfe2ae742bb1616c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

smarterclayton · 2016-11-03T21:17:14Z

Code as is and we'll figure out how to fix struct types in 1.6

On Nov 3, 2016, at 5:12 PM, bdbauer notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton Do you mean leave the
code as is, and create an issue to fix the Time/serialized type problem in
the future? Or, get rid of my swagger spec changes, and make a bug to fix
them later?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35806 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pzvpGJzV2z2ePKfpbokf6JQAnyCzks5q6k4pgaJpZM4KjtBN
.

mbohlool · 2016-11-03T21:55:58Z

We are removing a field from swagger 1.2, is it not a backward compatibility issue?

I can see from typeToJSON function that we should only allow unversioned.Time to pass through. We can fix this in a more abstract way later (file the bug please) but I think we should fix it for unversioned.Time to not have a backward compatibility issue with clients that already using swagger 1.2. It should be a simple fix.

mbohlool · 2016-11-03T21:59:26Z

pkg/apiserver/api_installer.go

@@ -944,6 +948,11 @@ func addObjectParams(ws *restful.WebService, route *restful.RouteBuilder, obj in
 			}
 			switch sf.Type.Kind() {
 			case reflect.Interface, reflect.Struct:
+			case reflect.Ptr:
+				if sf.Type.Elem().Kind() == reflect.Interface || sf.Type.Elem().Kind() == reflect.Struct {


you can add something like

// TODO... (add appropriate message here to say this is a hack and need to be fixed in a generic way later, hopefully with a bug number) if (...) && strings.TrimPrefix(sf.Type.String(),"*") != "unversioned.Time"

to let unversioned.Time pass through and have the removed time field back to the spec.

…QueryParameter

k8s-ci-robot · 2016-11-04T00:24:32Z

Jenkins GCE e2e failed for commit cb188d9067addb1a88f9b04161c401a67c3ad98a. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-04T00:37:58Z

Jenkins verification failed for commit 297c8dbfc30c31c66ec08f3602f8cfcb706ba404. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-11-04T00:49:18Z

Jenkins GKE smoke e2e failed for commit 297c8dbfc30c31c66ec08f3602f8cfcb706ba404. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

bdbauer · 2016-11-04T21:15:25Z

@smarterclayton look good?

smarterclayton · 2016-11-04T21:28:00Z

/lgtm

k8s-github-robot · 2016-11-05T15:11:18Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-11-05T15:49:34Z

Automatic merge from submit-queue

…6-upstream-release-1.4 Automated cherry pick of #35806

k8s-cherrypick-bot · 2016-11-10T01:26:17Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

…k-of-#35806-upstream-release-1.4 Automated cherry pick of kubernetes#35806

googlebot added the cla: yes label Oct 28, 2016

k8s-github-robot assigned smarterclayton Oct 28, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 28, 2016

smarterclayton reviewed Oct 28, 2016

View reviewed changes

smarterclayton added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 31, 2016

smarterclayton changed the title ~~WIP: Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter~~ Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter Nov 1, 2016

k8s-github-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2016

smarterclayton removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2016

k8s-github-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 Nov 3, 2016

mbohlool reviewed Nov 3, 2016

View reviewed changes

bdbauer mentioned this pull request Nov 3, 2016

Determine correct behavior for addObjectParams in api_installer.go #36191

Closed

Made changes to DELETE API to let v1.DeleteOptions be passed in as a …

76c3804

…QueryParameter

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 3, 2016

Updated openapi spec, swagger spec, and swagger doc

2e7195f

smarterclayton added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label Nov 4, 2016

smarterclayton added this to the v1.5 milestone Nov 4, 2016

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2016

k8s-github-robot merged commit 17fda0a into kubernetes:master Nov 5, 2016

jessfraz added cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Nov 8, 2016

jessfraz modified the milestones: v1.4, v1.5 Nov 8, 2016

bdbauer mentioned this pull request Nov 9, 2016

Automated cherry pick of #35806 #36534

Merged

jessfraz added a commit that referenced this pull request Nov 10, 2016

Merge pull request #36534 from bdbauer/automated-cherry-pick-of-#3580…

bc643df

…6-upstream-release-1.4 Automated cherry pick of #35806

k8s-cherrypick-bot removed the cherrypick-candidate label Nov 10, 2016

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016

Merge pull request kubernetes#36534 from bdbauer/automated-cherry-pic…

2580a08

…k-of-#35806-upstream-release-1.4 Automated cherry pick of kubernetes#35806

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

caesarxuchao mentioned this pull request Mar 17, 2017

API server cannot parse query parameter propagationPolicy sent with a delete request #43329

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter #35806

Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter #35806

bdbauer commented Oct 28, 2016 •

edited

Loading

smarterclayton Oct 28, 2016

bdbauer Oct 28, 2016

smarterclayton Oct 28, 2016

smarterclayton Oct 28, 2016 •

edited

Loading

bdbauer Oct 28, 2016

bdbauer commented Oct 29, 2016

smarterclayton commented Oct 31, 2016

smarterclayton commented Oct 31, 2016

bdbauer commented Nov 1, 2016

maisem commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

nikhiljindal commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

mbohlool commented Nov 1, 2016

smarterclayton commented Nov 2, 2016

mbohlool commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

smarterclayton commented Nov 3, 2016

mbohlool commented Nov 3, 2016

mbohlool Nov 3, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

bdbauer commented Nov 4, 2016

smarterclayton commented Nov 4, 2016

k8s-github-robot commented Nov 5, 2016

k8s-github-robot commented Nov 5, 2016

k8s-cherrypick-bot commented Nov 10, 2016

Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter #35806

Made changes to DELETE API to let v1.DeleteOptions be passed in as a queryParameter #35806

Conversation

bdbauer commented Oct 28, 2016 • edited Loading

smarterclayton Oct 28, 2016

Choose a reason for hiding this comment

bdbauer Oct 28, 2016

Choose a reason for hiding this comment

smarterclayton Oct 28, 2016

Choose a reason for hiding this comment

smarterclayton Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

bdbauer Oct 28, 2016

Choose a reason for hiding this comment

bdbauer commented Oct 29, 2016

smarterclayton commented Oct 31, 2016

smarterclayton commented Oct 31, 2016

bdbauer commented Nov 1, 2016

maisem commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

nikhiljindal commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

mbohlool commented Nov 1, 2016

smarterclayton commented Nov 2, 2016

mbohlool commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

k8s-ci-robot commented Nov 2, 2016

smarterclayton commented Nov 3, 2016

mbohlool commented Nov 3, 2016

mbohlool Nov 3, 2016

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

bdbauer commented Nov 4, 2016

smarterclayton commented Nov 4, 2016

k8s-github-robot commented Nov 5, 2016

k8s-github-robot commented Nov 5, 2016

k8s-cherrypick-bot commented Nov 10, 2016

bdbauer commented Oct 28, 2016 •

edited

Loading

smarterclayton Oct 28, 2016 •

edited

Loading