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

add delete_collection api #541

Merged
merged 1 commit into from
Jun 9, 2022
Merged

Conversation

russell
Copy link
Contributor

@russell russell commented Feb 22, 2022

Delete collection is like getting a list, except it triggers a delete of the
resources. It returns a list of all resources that match the call.

I based this of the get entities like get_pods because it's roughly
the same API except as a side effect it deletes all the resources.

# :orphan_dependents (bool) - should the dependent objects be orphaned
# :propagation_policy (string) - one of Foreground|Background|Orphan
# :resource_version (string) - sets a limit on the resource versions that can be served
# :resource_version_match (string) - determines how the resource_version constraint will be applied
Copy link
Collaborator

Choose a reason for hiding this comment

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

lib/kubeclient.rb:481:101: C: Layout/LineLength: Line is too long. [105/100]

Opened #547 to add support resource_version_match in other actions.

@cben
Copy link
Collaborator

cben commented Mar 5, 2022

Sorry for delay. Generally looks good 👍

Please document this in README. Including what it returns (the content of the deleted resources?) because it's not entirely obvious.

Let's see, how this compares to delete-single interface:

delete_foo('name', 'namespace', opts)    # namespaced
delete_foo('name', nil, opts)            # global
delete_foos(namespace: 'ns', **opts)     # namespaced
delete_foos(**opts)                      # global

Not consistent but it mirrors the existing inconsistency between get_foo and get_foos.
And I do like keyword arguments in new APIs. So no objection 👍

  • Is this dangerous that this is only 1 letter off from deleting a single resource?
    If I meant delete_pod but accidentally mistype delete_pods('mypod', 'ns') will it error, or delete stuff?

Another point worth documenting: What happens if you omit namespace: with a namespaced resource, e.g. pods — will it error, or delete all pods in all namespaces?
I see the unit test covers exactly this scenario — it sends DELETE request to http://localhost:8080/api/v1/pods. (makes sense, build_namespace_prefix has no idea whether it's used with a resource supposed to be namespaced)
But will that work? In k8s API I only see documentation for DELETE /api/v1/namespaces/{namespace}/pods.

  • can you please add a test case using a namespace: param?

Delete collection is like get list, except it triggers a delete of the
resources. It returns a list of all resources that match the call.

I based this of the get entities like get_pods because it's roughly
the same API except as a side effect it deletes all the resources.
@russell
Copy link
Contributor Author

russell commented Apr 26, 2022

Sorry for delay. Generally looks good 👍

I have rebased the PR since you last looked. DW, i'm even slower ;)

Please document this in README. Including what it returns (the content of the deleted resources?) because it's not entirely obvious.

It returns the same thing as doing a get_foos and the doc suggests looking at get to understand it's potential return values.

  • Is this dangerous that this is only 1 letter off from deleting a single resource?
    If I meant delete_pod but accidentally mistype delete_pods('mypod', 'ns') will it error, or delete stuff?

maybe, but it's no more dangerous than any other api IMHO.

if you mistype it you get

[5] pry(main)> client.delete_pods('foo', namespace: 'default')
ArgumentError: wrong number of arguments (given 2, expected 0..1)
from /Users/rsim/Code/zendesk/kubeclient/lib/kubeclient.rb:313:in `block (2 levels) in define_entity_methods'
[6] pry(main)> client.delete_pods('foo', 'default')
ArgumentError: wrong number of arguments (given 2, expected 0..1)
from /Users/rsim/Code/zendesk/kubeclient/lib/kubeclient.rb:313:in `block (2 levels) in define_entity_methods'

so it won't accept any positional argument for a name.

Another point worth documenting: What happens if you omit namespace: with a namespaced resource, e.g. pods — will it error, or delete all pods in all namespaces? I see the unit test covers exactly this scenario — it sends DELETE request to http://localhost:8080/api/v1/pods. (makes sense, build_namespace_prefix has no idea whether it's used with a resource supposed to be namespaced) But will that work? In k8s API I only see documentation for DELETE /api/v1/namespaces/{namespace}/pods.

Nothing, you get an error

Kubeclient::HttpError: the server does not allow this method on the requested resource
  • can you please add a test case using a namespace: param?

I have updated all test cases to handle namespace: because it's the only working form of the method

@cben
Copy link
Collaborator

cben commented Jun 2, 2022

CI failures are just flaky timeouts. Close-cycling to re-run.

@cben cben closed this Jun 2, 2022
@cben cben reopened this Jun 2, 2022
@cben cben merged commit 024f5df into ManageIQ:master Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants