-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 Handle finalizers early in Reconciles #11286
🌱 Handle finalizers early in Reconciles #11286
Conversation
/test pull-cluster-api-e2e-main /hold |
b7ee8b6
to
1f9ec2e
Compare
/test pull-cluster-api-e2e-main |
Signed-off-by: Stefan Büringer [email protected]
1f9ec2e
to
7183826
Compare
Rebased /hold cancel |
/test pull-cluster-api-e2e-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
return false, err | ||
} | ||
|
||
controllerutil.AddFinalizer(o, finalizer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, not for this PR.
I think we should stop calling the patch helper when it is easy to figure out that no changes have been applied, like in this case (this will avoid a few Mashal operations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand what you mean. In general I agree we shouldn't call the patch helper if we know that there is no change. But in this case there will always be a change?
(or do you mean just in general when using a func like AddFinalizer that easily tells us if there was a change?)
LGTM label has been added. Git tree hash: 52881545fdbf66982ddd56af5d8b4abbae0beec3
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #11105