Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrate controllerutil AddFinalizer and RemoveFinalizer controllerutil.Object and deprecate *WithError functions #959

Closed
dmvolod opened this issue May 22, 2020 · 1 comment · Fixed by #962
Closed

Migrate controllerutil AddFinalizer and RemoveFinalizer controllerutil.Object and deprecate *WithError functions #959

dmvolod opened this issue May 22, 2020 · 1 comment · Fixed by #962
Assignees
@dmvolod

Comments

@dmvolod
Copy link
Contributor

dmvolod commented May 22, 2020

As #898 introduces controllerutil.Object interface it would be nice to migrate controllerutil AddFinalizer and RemoveFinalizer to this interface and remove *WithError functions as they are not needed anymore as checks are performing on the compile time.
@dmvolod
Copy link
Contributor Author

dmvolod commented May 22, 2020

/assign

@dmvolod dmvolod mentioned this issue May 23, 2020
Merged
@dmvolod dmvolod changed the title Migrate controllerutil AddFinalizer and RemoveFinalizer controllerutil.Object and remove *WithError functions Migrate controllerutil AddFinalizer and RemoveFinalizer controllerutil.Object and deprecate *WithError functions May 24, 2020
boekkooi-fresh added a commit to boekkooi-fresh/controller-runtime that referenced this issue Jul 30, 2020
@boekkooi-fresh
Use metav1.Object instead of Object
ebfd135 
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 kubernetes-sigs#959 and kubernetes-sigs#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.
@boekkooi-fresh boekkooi-fresh mentioned this issue Jul 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dmvolod dmvolod

Labels
None yet
Projects
None yet
Milestone
No milestone
1 participant
@dmvolod