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

[WIP] ⚠️ Add DeleteCollection to the client Interface. #324

Closed

Conversation

MadVikingGod
Copy link
Contributor

This Adds the DeleteCollection to the client Writer Interface. This would be used to delete objects based on label selection, instead of using Delete on each object individually.

Things I would appreciate feedback on:

  • DeleteCollectionOptions - This is currently a superset of ListOptions and DeleteOptions. Is there a different way to build the "functional options pattern" for two sets of options?
  • The Obj passed to DeleteCollection() is just used to determine the GVK/GVR, and will not delete objects in the list. Is there a way to signal that?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2019
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MadVikingGod
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: droot

If they are not already assigned, you can assign the PR to them by writing /assign @droot in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2019
@MadVikingGod
Copy link
Contributor Author

I signed it.

@DirectXMan12
Copy link
Contributor

  • DeleteCollectionOptions - This is currently a superset of ListOptions and DeleteOptions. Is there a different way to build the "functional options pattern" for two sets of options?

Other than just reproducing the different functions? Not really :-/. We could maybe make things more complicated elsewhere to avoid this, but I'm not sure its worth it.

  • The Obj passed to DeleteCollection() is just used to determine the GVK/GVR, and will not delete objects in the list. Is there a way to signal that?

No, we don't really have a good way to signal that.

There is one other design to consider -- something like

Delete(pod, someDeleteOption, CollectionMatching(listOptions...)) and just having that be the interface to delete-collection (i.e. it would be in the options), similarly to how namespaced vs non-namespaced is determined by an option.

Added CollectionOptions to the DeleteOptions for capturing the List
options needed for a DeleteCollection.
@DirectXMan12
Copy link
Contributor

Ping, what's the state on this? Just want to make sure you're not waiting on anything from me :-)

@MadVikingGod
Copy link
Contributor Author

Sorry for the large break, I had other schedule priorities.

I have updated this so that there is no longer a DeleteCollectionOptions, and instead there is a ListOptions as part of the DeleteOptions. This is only used by the DeleteCollection method, and it feels more natural.

@DirectXMan12 I would appreciate it if you could look over, and let me know what is needed to move this forward.

@DirectXMan12
Copy link
Contributor

I'll take a look. I'm still wondering if there's a good reason to separate the DeleteCollection from Delete methods. Could we just have DeleteOptions choose one or the other based on the types and/or the presence of ListOptions in the delete options? That feels slightly more natural to me. WDYT?

@adracus
Copy link
Contributor

adracus commented May 14, 2019

@MadVikingGod / @DirectXMan12 still up to getting this in? Pretty nice feature I'd also like to have :) If you need any help, let me know!

@DirectXMan12 DirectXMan12 added this to the 0.2.0 milestone May 17, 2019
@DirectXMan12
Copy link
Contributor

yeah, I'd still like to get it in. Let's aim for before the 0.2.0 final release

@adracus
Copy link
Contributor

adracus commented May 21, 2019

So then @MadVikingGod will you finish or should I take over ?

@MadVikingGod
Copy link
Contributor Author

Sorry, I haven't had the time to dedicate to following up on this.

You are more than welcome to take this over. I have signed the CLA, I don't know why it is not showing.

@adracus
Copy link
Contributor

adracus commented May 26, 2019

Since I'm not a maintainer I couldn't modify your PR, @MadVikingGod. I now went ahead with a different PR 😐 : #447 . Also mentioned you as a co-author, although the main work really has been done by you, hope this is ok for you. Otherwise, I guess the PR contains the suggested changes by @DirectXMan12 (not having a DeleteCollection method but extending the DeleteOptions with CollectionOptions).

@MadVikingGod
Copy link
Contributor Author

MadVikingGod commented May 26, 2019 via email

@MadVikingGod
Copy link
Contributor Author

Lost track of this, I'm very sorry. Thank you again for the time, and@adracus for taking it up in the other pr.

DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants