✨Finalizer helper library

[rashmigottipati]

Closes #1453
PR for adding a finalizer helper library to controller-runtime

Signed-off-by: rashmigottipati chowdary.grashmi@gmail.com

[k8s-ci-robot]

Hi @rashmigottipati. 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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[joelanford]

/ok-to-test

[rashmigottipati]

/cc @estroz @jmrodri

[k8s-ci-robot]

@rashmigottipati: GitHub didn't allow me to request PR reviews from the following users: jmrodri.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @estroz @jmrodri

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.

[rashmigottipati]

@joelanford thanks for the review! Addressed the latest set of comments. Could you please take another look?

[joelanford]

I think the last question I have is whether we want to split the needsUpdate value in two: needsUpdate and needsStatusUpdate.

Without that, callers using CRDs with the status subresource will likely need to call both client.Update() and client.Status().Update()

On the otherhand, an API with three return values starts to get into "is this the right API?" territory.

[estroz]

@joelanford I agree; something like OperationResult would be useful here.

[joelanford]

+1 @estroz. Are you thinking of using that directly or adding a separate type in the finalizer package that conveys just what a finalizer would need? I vote the latter.

If you agree, I think there's two options for how to do this:

type Result struct {
   Updated bool
   StatusUpdated bool
}

or

type Result string

const (
	ResultUnchanged     Result = "unchanged"
	ResultUpdated       Result = "updated"
	ResultUpdatedStatus Result = "updatedStatus"
    ResultUpdatedAll    Result = "updatedAll"
)

I think I prefer the result struct rather than the result string, since it seems easier to use in conditional expressions that drive the client update calls.

[estroz]

A new type for sure, and I'm fine with the struct approach (pretty similar to Option<T> return types in functional languages that get pattern-matched).

[rashmigottipati]

/test pull-controller-runtime-test-master

[estroz]

/lgtm

[joelanford]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: estroz, joelanford, rashmigottipati

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

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

[alvaroaleman]

@estroz @joelanford Can we in the future please either tell our contributors to squash their commits or use the label to have them squashed on merge? Something like this clutters the git history bigtimes for no good reason.

[estroz]

@alvaroaleman yes, a worthwhile callout.

[rashmigottipati]

@alvaroaleman, my bad I didn't realize that the PR would get automatically merged after it's approved. Will be mindful of that going forward. Thanks for mentioning it.

[joelanford]

My bad! Perhaps we should configure the merge bot to squash by default? I feel like I'm guilty of approving several PRs without using the label or asking for a squash because I'm used to other projects where that happens automatically.