Fake Client .Update updates both spec and status?

[ZakMiller]

The fake client's Update call updates both the spec and status of the object.

This is different from how the real client behaves. This can lead to situations where someone writes a unit test, the unit test passes, but the behavior will be different when dealing with a real client.

Is this known/intended? It seems like the fake client should update the spec and replace the status with whatever the value is on the "server".

[fovgrubby128]

Hey @vincepri @alvaroaleman, sorry to ping you here. I also found the same weird behavior from fakeClient. Could you help to have a look on this issue? thanks!

[alvaroaleman]

It depends if resources have or do not have a status subresource and today there is no way in the fake client to figure that out. Never updating the status would be just as wrong and also breaking.

Ideas on how to improve this are always welcome :)

[sbueringer]

[Only for CRDs] I wonder if it's an option to change behavior if the corresponding CRD has also been added to the fake client. With that information the fake client could handle the update correctly

Thinking about this a bit more, it's a bit strange and a difference to Kubernetes / envtest that the fake client does not require to add the CRDs before CRs can be added. But I think changing the fake client to require CRDs would break to many users of fake client.

[ZakMiller]

I'm not sure I understand.

We're using the fake client like this:

	fakeClient := fake.NewClientBuilder().
		WithScheme(s).
		Build()

If I do a fakeClient.Create call with a resource not included in that scheme, then I'll get back an error " no kind is registered for the type ..."

I guess there's some other way of interacting with it where you don't have to register the type?

Regardless, for our case, we're primarily concerned with the behavior for registered CRDs. If we changed the fake client Update to not update known subresources because the CRD is registered, would that work?

[alvaroaleman]

[Only for CRDs] I wonder if it's an option to change behavior if the corresponding CRD has also been added to the fake client

I think what @sbueringer was talking about is if the CRD object has been added to the fakeclient rather than the type registered in the scheme, as the CRD has the setting if the status subresource is or isn't enabled

[sbueringer]

Yes exactly!

If you write your unit tests with env test instead of fake client or move unit tests from fake client to envtest the first thing you have to do is actually deploy your CRDs :).

So it doesn't seem to far of to make the fake client more realistic in the case the corresponding CRD was actually added to the fake client.

[ZakMiller]

Okay cool. That all makes sense to me.

I thought the behavior was different with Patch, but it looks to be the same (I was using Patch incorrectly when I was checking the behavior).

So is my understanding correct that right now when you update a resource in any way using the fake client, whether a CRD is registered or not, all fields are potentially updatable (regardless of whether it includes a subresource), with the entire object being replaced by Update and Patch updating whatever's in the patch)?

And what we're talking about here is that we could change the behavior to be more in line with the client if someone has added the CRDs to the fake client because then we could inspect the CRD to know what fields on the CR are subresources. Then, we could basically strip the subresources out when doing the update/patch to mimic the behavior of the real client?

A few remaining questions:

1. Is this what's meant by this line? I might open a PR to clarify that a bit
2. Is this something that is likely to be done soon (the CRD-specific behavior)? If not, would you be open to me writing a PR?
3. Is there an envtest vs fake client comparison anywhere? I see it says "When in doubt, it's almost always better not to use this package and instead use envtest.Environment with a real client and API server." I'd imagine tests run much more quickly with fake client, and some of the downsides are listed on that same doc page, but I'm thinking about pitching that my project should switch to envtest and it would be nice to have some more details.

[alvaroaleman]

2: Is this something that is likely to be done soon (the CRD-specific behavior)? If not, would you be open to me writing a PR?

Sure

[alvaroaleman]

Took a stab at this: #2259

[fovgrubby128]

Sorry, a dumb question, this might be a breaking change, right? Since I would imagine some unit tests might break if we introduce this change.

[alvaroaleman]

Sorry, a dumb question, this might be a breaking change, right? Since I would imagine some unit tests might break if we introduce this change.

Yes it is, which is why the corresponding PR is marked as breaking and will be highlighted as such in the release notes