-
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
🐛 Carry over metadata.uid at ServerSidePatchHelper #6742
🐛 Carry over metadata.uid at ServerSidePatchHelper #6742
Conversation
/test all |
internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go
Outdated
Show resolved
Hide resolved
/test all |
/test pull-cluster-api-e2e-full-main |
internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go
Outdated
Show resolved
Hide resolved
/lgtm |
internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go
Outdated
Show resolved
Hide resolved
4775d1e
to
b54df37
Compare
/test all |
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 one nit
except if we want to move it to desired_state, but for me it's fine where it is
internal/controllers/topology/cluster/structuredmerge/serversidepathhelper.go
Outdated
Show resolved
Hide resolved
The issue with desired_state is: we don't know if we want to update (in-place -> keep uid) or recreate (template rotation -> no uid) the object. |
Ah good point / reason to keep it as is |
/lgtm |
/lgtm /cherry-pick release-1.2 |
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you. In response to this:
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. |
/hold Let's see if it is a new flake |
/test pull-cluster-api-test-main |
b54df37
to
b93b125
Compare
We'll have to wait for #6773 to merge first. Afterwards this one should turn green. |
b93b125
to
f73ea3e
Compare
Rebased as #6773 has been merged |
This prevents the race condition of running server side apply on an object which got: * deleted in the meantime * recreated in the meantime and because of that the reconciler may use outdated information.
f73ea3e
to
524d990
Compare
/lgtm But let's keep the hold to run the tests a few times to make sure we're not introducing flakes |
/test pull-cluster-api-test-main |
4 similar comments
/test pull-cluster-api-test-main |
/test pull-cluster-api-test-main |
/test pull-cluster-api-test-main |
/test pull-cluster-api-test-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
/test pull-cluster-api-test-main |
Looks pretty stable /approve |
[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 |
/hold cancel |
@sbueringer: new pull request created: #6784 In response to this:
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. |
What this PR does / why we need it:
This prevents the race condition of running server side apply on an object which got:
and the topology controller won't apply using outdated information anymore.
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 #6741