[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

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 cben mentioned this issue Mar 4, 2018
Closed
@cben cben mentioned this issue May 27, 2018
Open
4 tasks
@cben cben mentioned this issue Nov 22, 2018
Open
@cben
Copy link
Collaborator Author
cben commented Nov 22, 2018 •
edited
Loading

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 tsontario mentioned this issue Jan 26, 2019
Closed
@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 •
edited
Loading

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 cben mentioned this issue Apr 4, 2019
Closed
@rhysm rhysm mentioned this issue May 7, 2019
Open
@cben cben mentioned this issue Dec 15, 2019
Closed
@cben cben added the watches About watching k8s objects label Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement help wanted watches About watching k8s objects
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Positional and params support tsontario/kubeclient
2 participants
@cben @tsontario