Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should DeleteOptions be also parsed from url Params? #84976

Closed
wojtek-t opened this issue Nov 8, 2019 · 7 comments

Comments

@wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Nov 8, 2019

Currently, DeleteOptions can be parsed in two ways:

Is the second option really want we want to support?
@smarterclayton @deads2k @kubernetes/sig-api-machinery-bugs

We need to add that the second option is actually exercised by one of the conformance tests:
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/node/pods.go#L123

Assuming we want to support it, we should create a dedicated conversion method, because currently it's not really possible to set things like Preconditions via the url params:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L474

@liggitt

This comment has been minimized.

Copy link
Member

@liggitt liggitt commented Nov 8, 2019

Is the second option really want we want to support?

Passing a body in delete requests is not uniformly supported by http libraries, so I would not expect us to drop support for options in the URL

Assuming we want to support it, we should create a dedicated conversion method, because currently it's not really possible to set things like Preconditions via the url params

Agree. xref #21476

@wojtek-t

This comment has been minimized.

Copy link
Member Author

@wojtek-t wojtek-t commented Nov 8, 2019

Thanks Jordan.

Adding "help-wanted" label, so to quickly summarize, we need to create a manual conversion function here:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/conversion.go
We need to double check how much we can auto-convert, I think the only one is "[]string -> Preconditions" and auto-generate the url.Values->DeleteOptions (there are examples of that there)

@wojtek-t wojtek-t added the help wanted label Nov 8, 2019
@gongguan

This comment has been minimized.

Copy link
Contributor

@gongguan gongguan commented Nov 8, 2019

/assign

@yue9944882

This comment has been minimized.

Copy link
Member

@yue9944882 yue9944882 commented Nov 11, 2019

I think the only one is "[]string -> Preconditions" and auto-generate the url.Values->DeleteOptions (there are examples of that there)

this should also be properly reflected in the openapi v2 spec. currently there's no such query param "precondition" published in all delete apis. (ref #34856)

@gongguan

This comment has been minimized.

Copy link
Contributor

@gongguan gongguan commented Nov 14, 2019

/remove-help
Should this issue be closed?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Nov 14, 2019

@gongguan: Those labels are not set on the issue: kind/help

In response to this:

/remove-kind help
Should this issue be closed?

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.

@wojtek-t

This comment has been minimized.

Copy link
Member Author

@wojtek-t wojtek-t commented Nov 14, 2019

Yes - thank you!

@wojtek-t wojtek-t closed this Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.