-
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
🐛Check errors for IsNotFound after patching spec and status #10787
🐛Check errors for IsNotFound after patching spec and status #10787
Conversation
2d389a3
to
d6ac4de
Compare
d6ac4de
to
13dba33
Compare
0b48a19
to
42fa16e
Compare
/hold going to comment on the issue on the reasoning for the hold |
42fa16e
to
1a51016
Compare
/assign @sbueringer |
1a51016
to
120874b
Compare
/test pull-cluster-api-e2e-main |
util/patch/patch.go
Outdated
if err := h.patchStatus(ctx, obj); err != nil { | ||
errs = append(errs, err) | ||
if !apierrors.IsNotFound(err) && !obj.GetDeletionTimestamp().IsZero() && len(obj.GetFinalizers()) == 0 { |
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.
I think the if is not entirely correct
See
if err := h.patchStatus(ctx, obj); err != nil {
if !(apierrors.IsNotFound(err) && !obj.GetDeletionTimestamp().IsZero() && len(obj.GetFinalizers()) == 0) {
errs = append(errs, err)
}
}
```
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.
How did I forget the (
and the )
🤦🏽♂️
This should be good to go now.
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.
No worries, that's why we do reviews :)
Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
120874b
to
c8ae301
Compare
Thank you very much!! /lgtm /hold cancel |
LGTM label has been added. Git tree hash: af7e5f0dfb22f0b35f80444857a1ed1c786d67b5
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
if err := h.patchStatus(ctx, obj); err != nil { | ||
errs = append(errs, err) | ||
if !(apierrors.IsNotFound(err) && !obj.GetDeletionTimestamp().IsZero() && len(obj.GetFinalizers()) == 0) { |
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.
Should we add a test?
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.
I can add a test for this and submit a new PR.
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.
PR with the test #10866
What this PR does / why we need it:
This allows the patch helper to ignore patching the status when creating a patch.The solution for the issue below was to check the object if the finalizers are removed and if the DeletionTimeStamp was not nil and the error after patching the status/spec was
IsNotFound
. This will cut down on noise as well as achieve with not exposing the internals of the patch helper.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #10786
/area util