kubernetes-sigs / kubebuilder-declarative-pattern

A toolkit for building declarative operators with kubebuilder
Apache License 2.0
254 stars 84 forks source link

enhance handling prune for CR #214

Closed xuzhenglun closed 2 years ago

xuzhenglun commented 2 years ago

What this PR does / why we need it:

  1. allow CR to decide whether to enable pruning resources;
  2. allow CR to decide what kinds of resources should be pruned.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

introduce 2 interfaces to let CR can decide whether and how to prune resources. When none of them are implemented, the behavior should be the same as before. So I think this is not a breaking change.

Additional documentation:

k8s-ci-robot commented 2 years ago

Hi @xuzhenglun. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
xuzhenglun commented 2 years ago

ping @atoato88

This PR enhances the ability of prune, and it doesn't relate with DirectApplier. I would be very grateful if you could take a look at this PR.

atoato88 commented 2 years ago

I agree with this PR's enhancement about prune-whitelist.

I think kubebuilder-declarative-pattern has similar functions by WithApplyPrune already. I think this framework chosed implement With*** functions for manipurating, so it's prefer to update WithApplyPrune or add new With*** functions if you would like to enhance prune functionality. e.g. implement new WithPruneWhitelist? WDYT?

xuzhenglun commented 2 years ago

agree

With*** function works just fine when we want to build a specific addon operator. However, I'm trying to implement a generic AddonManager for Cluster. My user could choose to enable pruning by setting a flag in CR. Furthermore, consisting of Addon could have CRD inside, so they should be able to configure the white list to determine what kind of resource could be pruned.

To achieve the above by using the WithPrune function, It's necessary to pass CR to the user's function, and it will change the function signature of WithPrune which can be a breaking change. Secondly, framework users still need to implement methods like Prune or PruneWhiteList anyway, because those configurations are defined in CR.

WDYT?

atoato88 commented 2 years ago

Thank you to explain the context detailed. I agree with your thinking, I think it's preferable for achieve your requirements.

BTW, I left review comments above, please check those.

xuzhenglun commented 2 years ago

Thank you to explain the context detailed. I agree with your thinking, I think it's preferable for achieve your requirements.

BTW, I left review comments above, please check those.

Thanks for your review. I append some comments in the source code and just updated this PR.

atoato88 commented 2 years ago

/approve Thank you to update quickly!

atoato88 commented 2 years ago

/approve cancel oh, we should check test before approving.

atoato88 commented 2 years ago

/ok-to-test

atoato88 commented 2 years ago

/approve

k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atoato88, xuzhenglun

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/blob/master/OWNERS)~~ [atoato88] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
xuzhenglun commented 2 years ago

@atoato88 still needs an LGTM label to merge. Is something I need to do yet?

atoato88 commented 2 years ago

/lgtm