Feature/auto pruning

[everettraven]

Description of the change:
Update the pre-existing pruning functionality

Motivation for the change:

• Issue Improve prune feature implementation #101
• @ryantking EP: Automatic resource pruning EP enhancements#109

resolves #101

[openshift-ci]

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

[openshift-ci]

[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.

Needs approval from an approver in each of these files:

• OWNERS

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

[jmrodri]

first round. A few nits, a couple of typos, and a question about ErrUnprunable.

[ryantking]

Good start, I like the file organization. Would love to see some tests also, even if they're not passing yet.

[coveralls]

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%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
prune/prune.go	71	76	93.42%

Files with Coverage Reduction	New Missed Lines	%
prune/prune.go	2	91.01%

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

---

💛 - Coveralls

[everettraven]

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

[ryantking]

I think we need to discuss the Pruner struct. Using functional arguments AND exposing fields kind of strikes me as an anti-pattern because you could have something like this:

func main() {
    pruner := NewPruner(client.New())
    doEvilThings(p)
    p.Prune(ctx) // this will blow up calling a nil client
}

func doEvilThings(p *Pruner) {
    p.Client = nil
}

When a struct has publicly modifiable fields, the methods cannot trust the fields to be valid so much validate them before using them. Look at [this line][https://cs.opensource.google/go/go/+/refs/tags/go1.18.1:src/net/http/request.go;l=573] in net/http. Since request.URL is publicly modifiable, it must nil-check it. Typically, the point of the constructor is to force an instance to be in a good state when its created by having two things:

1. Sane defaults
2. Validation for user overrides.

That way we know that the fields are all either defaults the library chose or overrides the user chose that are valid. I think we should un-export most if not all fields on the Pruner and funnel all configuration into the pruner. A rule of thumb I use is if the point of the struct is to hold structured data like a Kubernetes type then export the fields, but if the point of the struct is to do work, then it should have a constructor that enforces a good state.

Generally speaking, I'm happy with how this is shaping up. I will review the tests once we get the main library to a good state. Great work.

[everettraven]

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

[fgiloux]

lots of good things

[fgiloux]

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

[everettraven]

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

[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]

@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 ?

[fgiloux]

@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

[everettraven]

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

Let me know what you think!

[fgiloux]

This looks good to me. I am just wondering whether we could save a few CPU cycles by not doing stable sorting.

[fgiloux]

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

[fgiloux]

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

[ryantking]

Fantastic work, @everettraven!

Few changes, the big one to call out is removing all the logging logic that's currently built-in. Users should use custom strategy and is-prunable functions with their own loggers as desired. We should not couple to a specific logging implementation.

@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.

[fgiloux]

@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)

[everettraven]

@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.

[ryantking]

/lgtm
/hold
Want signoff from @fgiloux then I'll unhold (or you can if you have the permissions)

[jmrodri]

/approved

[everettraven]

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

[fgiloux]

@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.

[ryantking]

@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