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

Harmonize positional vs keyword namespace: args #312

Open
cben opened this issue Mar 4, 2018 · 3 comments
Open

Harmonize positional vs keyword namespace: args #312

cben opened this issue Mar 4, 2018 · 3 comments
Labels
enhancement help wanted watches About watching k8s objects

Comments

@cben
Copy link
Collaborator

cben commented Mar 4, 2018

In some method we take namespace as optional positional arg, in some as optional keyword namespace: arg. This is error-prone, and doesn't scale as we want to add more optional args. (It'd be especially awkward if you'd need to pass dummy nil namespace when dealing with a non-namespaced entity such as nodes...)

Long-term I'd like to support keyword namespace: everywhere, initially still supporting positional where needed for compatibility, eventually deprecating positional.

@cben
Copy link
Collaborator Author

cben commented Nov 22, 2018

Summary of current signatures wrt. name & namespace:

get_foos(namespace: 'namespace', **opts)  # namespaced collection
get_foos(**opts)                          # all namespaces or global collection

get_foo('name', 'namespace', opts)  # namespaced
get_foo('name', nil, opts)          # global
  # just get_foo('name', opts) currently doesn't work.

# same plural method name, double duty collection/single!
watch_foos(namespace: ns, **opts)   # namespaced collection
watch_foos(**opts)                  # all namespaces or global collection
watch_foos(namespace: ns, name: 'name', **opts)   # namespaced single object
watch_foos(name: 'name', **opts)                  # global single object

delete_foo('name', 'namespace', opts)    # namespaced
delete_foo('name', nil, opts)            # global
  # just delete_foo('name', opts) currently doesn't work.

create_foo(Kubeclient::Resource.new({metadata: {name: 'name', namespace: 'namespace', ...}, ...}))
create_foo(Kubeclient::Resource.new({metadata: {name: 'name', ...}, ...}))  # global

update_foo(Kubeclient::Resource.new({metadata: {name: 'name', namespace: 'namespace', ...}, ...}))
update_foo(Kubeclient::Resource.new({metadata: {name: 'name', ...}, ...}))  # global

patch_foo('name', patch, 'namespace')    # namespaced
patch_foo('name', patch)                 # global

😱 😖 ⁉️ 🤕

@tsontario
Copy link
Contributor

Long-term I'd like to support keyword namespace: everywhere

What do you think about requiring keywords for all identifying parameters? E.g. instead of

get_foo(name, namespace)

Instead we do

get_foo(name: name, namespace: namespace)

For myself, I think it would be ideal if we required keywords for all parameters for the get/update/patch/etc.

@cben
Copy link
Collaborator Author

cben commented Jan 27, 2019

I don't want to break comaptibility, especially as this would affect almost every call.
I'll consider allowing name too as keyword. Though it's not optional.

See also #332 that proposes a new hash-only interface.
There, I'm thinking of omitting name to .get & .watch distinguishing plural list from singular get (?).

EDIT: looking at #391...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement help wanted watches About watching k8s objects
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants