🌱 Fake client should set CreationTimestamp #3404
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fabriziopandini 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
size/M
|
This is be a very breaking change for any unit test that compares content of the fake client with a reference. I am also not sure if using time.Now is a good idea as opposed to some hardcoded fixed tomrstamp to make comparison easier |
|
@alvaroaleman that's a fair comment, this is why I asked in #3402 before proposing a PR What do you suggest as a next step? use a const instead of time.Now or do you prefer dropping the change entirely? |
|
I think the ask will come up sooner or later anyways, so doing this seems fine to me in principle, we just have to mark it as breaking change. Using a static const as creation timestamp allows the comparison to still work and I think doesn't really have much drawback other than being different from what the real KAS does - Any opinion on this @sbueringer ? |
|
I think the only problem with using a constant is that it won't work in cases where either prod or test code uses the creationTimestamp to check if it's before/after other timestamps (I don't have a use case for this though) |
|
Just remembered a case. In CAPI we have a controller that rolls out new Machines if they have not been created after a certain timestamp. Example
If now the timestamps would be always in the past the controller would never stop creating new MachineSets/Machines. I imagine other folks might have similar use cases. |
|
IIRC we already have the comparison problem with managedFields (only if enabled?) Maybe also with deletionTimestamp, although not that often with deletionTimestamp of course. I'm leaning towards moving the fake client closer to reality (i.e. using time.Now or something similar) We could have an option similar to |
|
Hm yeah, fair point regarding the comparison problem. I was going to point out that creationTimestamp isn't very useful for validating one thing happens after another since it only has second granularity but if there is logic around "happened N time in the past" this still causes issues. It is already possible to set creationTimestamp through the interceptor (Which I think this change breaks?) - Maybe that is enough, as it already provides a configurable opt-in? |
|
I have pushed a new version that makes setting creationTimestamp an opt-in behaviour, without the need to write interceptors. In the future, if there will be ever agreement on this change, it will be easy to make the switch from opt-in to opt-out. |
fabriziopandini
force-pushed
the
fake-client-sets-CreationTimestamp
branch
from
b25ee22 to
066bab4
Compare
fabriziopandini
force-pushed
the
fake-client-sets-CreationTimestamp
branch
from
066bab4 to
d674baa
Compare
|
@fabriziopandini: 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. |
| if err != nil { | ||
| return apierrors.NewInternalError(err) | ||
| } | ||
| obj.SetCreationTimestamp(metav1.Time{ |
There was a problem hiding this comment.
Is this the behavior of the KAS to just override it if its set?
| } | ||
|
|
||
| // WithSetCreationTimestamp configures the fake client to set metadata.creationTimestamp on Create and first Apply. | ||
| // on objects. |
| }, | ||
| } | ||
| err := cl.Create(ctx, newcm) | ||
| Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Maybe also check here that its unset?
| Expect(obj.ObjectMeta.CreationTimestamp.IsZero()).ToNot(BeTrue()) | ||
| }) | ||
|
|
||
| It("sets creatioTimestamp on SSA create when required to do so", func(ctx SpecContext) { |
There was a problem hiding this comment.
typo and that its required is implicit from the Context I think:
| It("sets creatioTimestamp on SSA create when required to do so", func(ctx SpecContext) { | |
| It("sets creationTimestamp on SSA create", func(ctx SpecContext) { |
| if c.setCreationTimestamp { | ||
| now, err := time.Parse(time.RFC3339, time.Now().Format(time.RFC3339)) | ||
| if err != nil { | ||
| return apierrors.NewInternalError(err) |
There was a problem hiding this comment.
Maybe mention what failed so its easier to understand?
4 participants
Fixes #3402