Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign theishshah after the PR has been reviewed.
You can assign the PR to them by writing /assign @theishshah in a comment when ready.

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

Pull Request Test Coverage Report for Build 2265466586

• 132 of 137 (96.35%) changed or added relevant lines in 4 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+3.8%) to 84.703%

Totals
Change from base Build 2064088101:	3.8%
Covered Lines:	598
Relevant Lines:	706

💛 - Coveralls

Mentioning @fgiloux due to involvement in the review of the EP for this feature.

As of now 1 test is expected to fail due to an issue with the controller-runtime fakeClient not respecting dry run options when running delete operations. I opened an upstream issue: kubernetes-sigs/controller-runtime#1871

I think we need to discuss the Pruner struct. Using functional arguments AND exposing fields kind of strikes me as an anti-pattern.

One option would be to completely unexpose the Pruner struct and to expose an interface with the Prune method instead, similar to what is done here and here

I think we need to discuss the Pruner struct. Using functional arguments AND exposing fields kind of strikes me as an anti-pattern

I made some changes in commit 6768874

Let me know what you think @ryantking @fgiloux

@everettraven it looks good to me. One question: would it make sense to provide a couple of most common strategies? What I have in mind:

• keep at least the latest X resources
• keep at least the resources newer than Y

@everettraven it looks good to me. One question: would it make sense to provide a couple of most common strategies? What I have in mind:

* keep at least the latest X resources

* keep at least the resources newer than Y

I think if we wanted to do something like this it would require more discussion as to what some sensible defaults are.

My thoughts on it:
I'm a little hesitant to create some provided strategies that follow the StrategyFunc type and aren't very configurable and more so leave it to the library users to define their strategy.

As an example - if we took the idea of keeping at least the latest X resources and created a common strategy for it, we would have to explicitly define what X is within the logic of that common strategy. If a user wanted to use that common strategy but wanted to change X they wouldn't be able to.

func KeepLatestXStrategy(ctx context.Context, objs []client.Object) ([]client.Object, error) {
	// in this case we would want to keep the 10 latest resources
	// there isn't currently a way to pass something like the number of resources you would like to keep in.
	x := 10

	// Just for an example return the first 10
	if len(objs) < x {
		return objs, nil
	}

	return objs[:x], nil
}

One thing I think we could try is to create some kind of helper function that returns a common strategy with a configurable value kind of like this:

func GetKeepLatestXStrategy(x int) StrategyFunc {
	return func(ctx context.Context, objs []client.Object) ([]client.Object, error) {
		// Just for an example return the first 10
		if len(objs) < x {
			return objs, nil
		}
		return objs[:x], nil
	}
}

What do you think @fgiloux @ryantking ?

@everettraven I like:

func GetKeepLatestXStrategy(x int) StrategyFunc {
	return func(ctx context.Context, objs []client.Object) ([]client.Object, error) {
...

It allows the users for simple use cases to have pruning implemented with ~4 lines of code.

The alternative would be to provide examples but that is not the same UX even when it is just a copy/paste away

@fgiloux @ryantking Some common strategy functions should be added with commit 3bf051a

Let me know what you think!

It looks good to me. Let see what @ryantking wants to add

@ryantking how does it look? Anything holding this PR?

@fgiloux Can you approve the EP? operator-framework/enhancements#109 We need that merged to move forward with this. I have updated it to match the implementation here.

done (as much as I can)

@fgiloux @jmrodri @ryantking if you could re-review the most up to date changes we can try to have this ready to go right after the EP merges.

/approved

@fgiloux When you have some time would you mind reviewing the latest changes?

@everettraven sorry I am very busy these days. I am fine with the approach and what I have seen but I would like to trial the library to make sure I am not missing something in terms of UX. This requires unfortunately a bit of time.

@fgiloux Then let's move forward with merging this and we can open followup tickets for issues that you find while working with it.

/unhold

CC: @everettraven