🐛: Fake client - handle the subresource client calls too#3444
🐛: Fake client - handle the subresource client calls too#3444fedepaol wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
toMapString / fromMapString are used in copy. Returning the unstructured pointer is dangerous, as the calling site expects the returned object to be a new one, and to be free to modify it. With the old implementation changing a value on the copy had the side effect of changing the value in the original implementation, for example in https://github.com/kubernetes-sigs/controller-runtime/blob/83b689cd942b7b68a44eeb91f7a9cc114aa59aff/pkg/client/fake/versioned_tracker.go#L243 it had the effect to modify the resource version of the old object too. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
The stack when calling client.Status().Update contains "sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeSubResourceClient).Update" and not "sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Update". Here we add a new test (failing without the patch) and the fix. Signed-off-by: Federico Paolinelli <fpaoline@redhat.com> Assisted-by: Claude <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fedepaol The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @fedepaol! |
|
Hi @fedepaol. 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 Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
closing in favor of #3443 @alvaroaleman maybe makes sense to keep c30a9f8 ? |
Regressed when bumping controller runtime in metallb, getting:
Operation cannot be fulfilled on xxx: object was modifiedwhen validating
cl.Status().Patch()calls.Also, when trying to debug and to understand why the local tests weren't failing I found an odd behavior in versioned_tracker.go fixed by the first commit.
Hope this helps, thanks for the project!