🐛 Use metav1.Object instead of Object

[boekkooi-fresh]

In order to allow for a broader adoption of ContainsFinalizer, RemoveFinalizer and AddFinalizer and remove a BC break introduced to 0.5.x I switched the method back to the metav1.Object.

After reading #959 and #962 I'm unable to understand the benefit of requiring runtime.Object so I propose to revert the BC break and possible panic on line 285.

[k8s-ci-robot]

Hi @boekkooi-fresh. 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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: boekkooi-fresh
To complete the pull request process, please assign droot
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.

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

[alvaroaleman]

@vincepri I think this would be a good candidate for 0.7
/ok-to-test

[k8s-ci-robot]

@boekkooi-fresh: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-controller-runtime-apidiff-master	ebfd135	link	/test pull-controller-runtime-apidiff-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[vincepri]

I'm not sure if we want to go down this path, we want to make sure objects are "Kubernetes Objects" going forward, or that was the original intent.

[alvaroaleman]

Yeah I apparantly didn't read the change properly. I think what we want for all finalizer-related funcs is to work on an Object instead of a metav1.Object or runtime.Object

[vincepri]

I think so, I also wouldn't be opposed to move the interface under the main package, maybe under the alias.go file?

[boekkooi-fresh]

I have to agree with @vincepri here that moving this under the main package maybe better for the Object.

Regarding my main issue is that between v0.6.0 and v0.6.1 I had to change my controller code from using metav1.Object to controllerutil.Object to keep using the controllerutil.RemoveFinalizer etc. This also caused a bunch of my code to break that was just passing along metav1.ObjectMeta and as far as I can see none of the now required runtime.Object methods are ever used by controllerutil.*Finalizer making me wonder why this change was made.
Since I was unable to determine why this change was made based on the related PR's I assume this was an oversight and opened this PR.

[vincepri]

@boekkooi-fresh Apologies this caused a breaking change for you, we're trying to move to this object interface internally in controller runtime to avoid casting at runtime, instead we want to require that objects are always runtime.Object and metav1.Object

[boekkooi-fresh]

Hey @vincepri,
It happens I'm already very happy that I can use controller-runtime for my project. I managed to refactor my code to use the new controllerutil.Object and the code actually looks better for it which is nice.
I'm fine with closing this PR but maybe next time if at all possible a BC break can be done in a minor bump?

Thank you all for the nice library!

[vincepri]

@boekkooi-fresh That's our goal! This was an oversight, apologies again for that.

/close

[k8s-ci-robot]

@vincepri: Closed this PR.

In response to this:

@boekkooi-fresh That's our goal! This was an oversight, apologies again for that.

/close

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.