Make label and field selector query strings versionable. #5233
Conversation
|
cc @lavalamp |
|
Thanks. API change LGTM. I'm in favor of renaming the query parameters to "label-selector" and "field-selector". I suppose reverse lookup would be "selector-target-labels" or somesuch. Delegating detailed review to others. We should also clarify kubectl flags at some point. |
|
BTW, bonus points for making these consistent across list operations and/or documented using go-restful. |
| @@ -124,19 +124,19 @@ type FakeRESTClient struct { | |||
| } | |||
|
|
|||
| func (c *FakeRESTClient) Get() *Request { | |||
| return NewRequest(c, "GET", &url.URL{Host: "localhost"}, c.Codec, c.Legacy, c.Legacy) | |||
| return NewRequest(c, "GET", &url.URL{Host: "localhost"}, "v1beta1", c.Codec, c.Legacy, c.Legacy) | |||
There was a problem hiding this comment.
Will recommend using testapi.Version() or latest.Version rather than hardcoding v1beta1
|
Comments addressed. Please take a look. |
| @@ -110,6 +110,8 @@ type Request struct { | |||
| selector labels.Selector | |||
| timeout time.Duration | |||
|
|
|||
| apiVersion string | |||
There was a problem hiding this comment.
This does not seem to be used anywhere.
Is this change required? Or am I missing something.
|
LGTM. |
|
LGTM for now. |
Make label and field selector query strings versionable.
|
I'm currently fixing and refactoring some of this, but I don't think we should be using "field-selector". In brief, we already use "resourceVersion", I'd like this to be consistent with our field naming ("fieldSelector" would be the JSON name), and we're using camelCase in field labels. If there's no disagreement, I'm going to change this in #5763 where I'm standardizing query parsing to follow our JSON pattern (so you can call api.Scheme.Convert(URL.Query.Values(), &api.Object) and version them). That will reduce our number of conversion patterns down to 1, and then we don't need special field naming for these sorts of cases (instead, we are deserializing query parameters into a versional object, then converting it to the internal form). |
|
SGTM. Presumably |
@bgrant0607
See #2131 for some context.
I know the unit tests are broken. If this LGTM to folks, I will fix them.