kubeclient icon indicating copy to clipboard operation
kubeclient copied to clipboard

Harmonize positional vs keyword namespace: args

Open cben opened this issue 7 years ago • 3 comments

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 avatar Mar 04 '18 10:03 cben

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

:scream: :confounded: :interrobang: :face_with_head_bandage:

cben avatar Nov 22 '18 13:11 cben

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.

tsontario avatar Jan 27 '19 03:01 tsontario

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...

cben avatar Jan 27 '19 05:01 cben