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.

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

Already on GitHub? Sign in to your account

馃尡 Add function to controllerutil RemoveControllerReference to remove controller references from objects #2455

Conversation

troy0820
Copy link
Member

@troy0820 troy0820 commented Aug 24, 2023

When using the function SetControllerReference it will error when trying to overwrite the controller reference. In issue #2381 the method used to circumvent the errors is to remove the controller reference and then reapply the new controller reference. There is no exposed method to do this. This mimics the behavior of removing a finalizer

  • Add function with description
  • Add test with coverage

Resolves #2381

Release note:

Add function RemoveControllerReference to pkg/controller/controllerutil 
to allow the removal of the owner reference of the controlled object.

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 24, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @troy0820!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 馃帀. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 馃槂

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 24, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @troy0820. 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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: troy0820
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 24, 2023
@troy0820 troy0820 force-pushed the troy0820/add-remove-controller-reference-function branch from a8c1417 to 770c4ab Compare August 25, 2023 13:59
@troy0820 troy0820 changed the title 馃悰 Add function to controllerutil RemoveControllerReference to remove controller references from objects 鉁ˋdd function to controllerutil RemoveControllerReference to remove controller references from objects Aug 25, 2023
@troy0820 troy0820 changed the title 鉁ˋdd function to controllerutil RemoveControllerReference to remove controller references from objects 馃悰 Add function to controllerutil RemoveControllerReference to remove controller references from objects Aug 26, 2023
@vincepri
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 28, 2023
Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
@troy0820 troy0820 force-pushed the troy0820/add-remove-controller-reference-function branch from 770c4ab to 7bdfa40 Compare August 28, 2023 18:19
Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 28, 2023
@troy0820 troy0820 requested a review from vincepri August 30, 2023 18:52
Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
@troy0820
Copy link
Member Author

troy0820 commented Sep 6, 2023

/assign @vincepri

Could you take another look at this @vincepri, not sure what else we need with this.

@troy0820 troy0820 changed the title 馃悰 Add function to controllerutil RemoveControllerReference to remove controller references from objects 馃尡 Add function to controllerutil RemoveControllerReference to remove controller references from objects Sep 9, 2023
@troy0820
Copy link
Member Author

Closing and deferring all feedback to #2462

Same method but allows us to utilize this function to remove owner references and/or controller references.

@troy0820 troy0820 closed this Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SetControllerReference should allow overwriting the previous controller
3 participants