Skip to content
This repository has been archived by the owner on Nov 19, 2020. It is now read-only.

Add CustomOption for API calls #43

Merged
merged 1 commit into from
Jun 29, 2017
Merged

Conversation

exekias
Copy link
Contributor

@exekias exekias commented Apr 8, 2017

This should allow anyone to add custom query params to API calls

Copy link
Owner

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity what query params do you need?

client.go Outdated
}

// CustomOption lets you define any filter for watch operations.
func CustomOption(name, value string) Option {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just QueryParam?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, let me update it

@exekias
Copy link
Contributor Author

exekias commented Apr 10, 2017

I'm using it with spec.nodeName atm, not sure if I will need more in the future

Copy link
Owner

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so you're using this to define a FieldSelector. You may want to look at #37. FieldSelectors aren't really supported by upstream, and doesn't work for a bunch of resources, and so I've been hesitant to add them to this client.

client.go Outdated
return o.paramName, o.paramValue
}

// QueryParam lets you define any filter for watch operations.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this could be used to set any URL query parameter.

// QueryParam can be used to manually set a URL query parameter by name.

@vjsamuel
Copy link

@ericchiang, field selectors have first class support upstream.

https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/types.go#L283

@ericchiang
Copy link
Owner

@vjsamuel the client supports adding it to the query but the API server doesn't have full support on every resource object: kubernetes/kubernetes#1362

It generally breaks for different resources so isn't something that people should be encouraged to use. You should prefer label selectors, so that why I haven't added field selector support.

@vjsamuel
Copy link

@ericchiang, the fieldSelector params working or not working depends on if the resource being queried defines valid selectable fields in its strategy.go

https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/core/pod/strategy.go#L163

FieldSelectors typically work for all such use-cases. It would be great if k8s-client could support the same. :)

@ericchiang
Copy link
Owner

ericchiang commented Apr 17, 2017

Sure, we can add them. I just want to make sure that there's adequate warning that they don't work with a bunch of resources. It'll probably look like a bug in the client otherwise.

@vjsamuel
Copy link

Agreed that it looks like a bug. But its only lack of documentation IMHO.

@exekias
Copy link
Contributor Author

exekias commented Jun 29, 2017

@ericchiang what's the status of this? do you want me to add a warning comment?

Copy link
Owner

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, lgtm

@ericchiang ericchiang merged commit 5803ed7 into ericchiang:master Jun 29, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants