🐛 fakeclient: Allow updating managedFields through Update#3485
🐛 fakeclient: Allow updating managedFields through Update#3485metlos wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
or merge Patch in the fake client. This is explicitly allowed and sometimes even necessary (e.g. upgrade from CSA to SSA) and so the fake client should support this.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: metlos 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 |
|
|
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @metlos! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @metlos. 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 Regular contributors should join the org to skip this step. 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. |
k8s-ci-robot
added
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| }) | ||
|
|
||
| // this is not working properly and can't without a larger change to the codebase | ||
| PIt("should not be able to manually update the managed fields through a subresource create,update", func(ctx SpecContext) { |
There was a problem hiding this comment.
No, not a regression. I think this never really worked and I'm not 100% sure about the full extent of changes that would make this work (e.g. the special handling of the scale subresource in update, ...). That's why I asked in the issue description if you even want this test in this PR.
|
/retest |
|
@metlos: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
| }) | ||
|
|
||
| // GH-3484 | ||
| It("respects the ManagedFields during create, update, merge patch", func(ctx SpecContext) { |
There was a problem hiding this comment.
Please write a testcase per operation
| }) | ||
|
|
||
| // this is not working properly and can't without a larger change to the codebase | ||
| PIt("should not be able to manually update the managed fields through a subresource create,update", func(ctx SpecContext) { |
There was a problem hiding this comment.
This test is extremely confusing and tests two distinct things. It doesn't actually test that scale requests can not manipulate the objects managed fields, it tests that objects managed fields account correctly for scale, which is a different thing.
Please write a test instead that validates that setting managed fields on a scale request does not result in a change of the objects managed fields.
| @@ -59,6 +59,7 @@ import ( | |||
| const ( | |||
There was a problem hiding this comment.
There is a compile failure in here
|
/retitle 🐛 fakeclient: Allow updating managedFields through Update |
k8s-ci-robot
changed the title
4 participants
The managed fields could not be explicitly modified in Update or merge Patch in the fake client.
This is explicitly allowed and sometimes even necessary (e.g. upgrade from CSA to SSA) and so the fake client should support this.
Also, I think the handling of managed fields on subresources in general (apart from status) is not well supported yet, so I took the liberty of adding a pending test for that (and am happy to remove it if you don't want it as part of this PR).
This fixes #3484.