Hi @glebiller. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

/assign @apelisse

Thanks for working on this!

Following the feedback, I simplified the code and added the following unit tests for both apply & update:

• A change to a managed field should update the time
• No changes to managed fields should not update the time
• A change to a non-managed field should not update the time

For Update

It looks like managed.Fields()[self] is defined only if there is a change to a managed field.
I, therefore, removed the second check to always update the time.

For Apply

The server-side-apply seems to be functional during my test in a live cluster. With the unit tests, I noticed that the object is either nil when the time does not need to be updated or not-nil when the update is required. I removed the extra condition.

Note for the reviewer

I notice that in the unit test TestNonManagedFieldsUpdateDoesNotModifyTime, after the third Update, the ManagedFields only have the fields from one manager (fieldmanager_a_test) and is missing the ManagedFields entry for the second manager ('fieldmanager_b_test'). Is this an intended behavior?

The Apply behavior is different, as the third Apply correctly returns the ManagedFields for both managers with the time changed accordingly.

/triage accepted

One thing that we have in the doc but isn't tested here, is that when a field is removed from a set (because it's stolen by someone else), that set's timestamp doesn't change, i.e.

In mostly pseudo-code:

applyObject("managerA", `{fieldA: true, fieldB: true}`)
time.Sleep(1)
applyObject("managerB", `{fieldA: false}`)
// Check that managerA timestamp hasn't changed.

According to the Server-Side-Apply doc, this is not possible because it raises a conflict error. I tried creating a unit test for that case, but I got the following error:
failed to apply object: Apply failed with 1 conflict: conflict with "fieldmanager_a_test": .data.key_b

The use case that could be tested, is during the Update instead of Apply.

According to the Server-Side-Apply doc, this is not possible because it raises a conflict error. I tried creating a unit test for that case, but I got the following error: failed to apply object: Apply failed with 1 conflict: conflict with "fieldmanager_a_test": .data.key_b

The use case that could be tested, is during the Update instead of Apply.

There is a "Force" field to steal ownership with Apply, which will let you test the same scenario!

Thanks, LGTM once you add the TestTakingOverManagedFieldsDuringApplyDoesNotModifyPreviousManagerTime

That's on me, I should have thought about the force as it's similar to the kubectl command line :)
I added the unit test and all the UT are now passing! Thanks for your help @apelisse

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, glebiller

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Thanks!