Improve logic for setting LastTransitionTime #16
Conversation
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
jetstack-bot
added
dco-signoff: yes
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
jetstack-bot
added
size/XL
| // Update the status with the provided condition details & return | ||
| // the added condition. | ||
| // NOTE: this code is just a workaround for cmutil only accepting a GenericIssuer interface |
There was a problem hiding this comment.
Instead of calling the cert-manager code, we now just host this logic in this repo. This makes it much clearer what is going on & allowed me to customize the logic for our needs (SSA).
There was a problem hiding this comment.
Definitely makes sense for his repo's issuer type to have these kinds of functions be more specific!
| fakeTime1 := time.Now().Truncate(time.Second) | ||
| fakeTimeObj1 := metav1.NewTime(fakeTime1) | ||
| fakeClock1 := clocktesting.NewFakeClock(fakeTime1) | ||
|
|
||
| fakeTime2 := time.Now().Add(4 * time.Hour).Truncate(time.Second) | ||
| fakeTimeObj2 := metav1.NewTime(fakeTime2) | ||
| fakeClock2 := clocktesting.NewFakeClock(fakeTime2) |
There was a problem hiding this comment.
suggestion: I think using time.Now here adds complexity which doesn't benefit us and might allow us to make more mistakes in the future.
It's entirely possible we could see a function which mistakenly calls time.Now() instead of clock.Now(). It's possible that in that situation it could pass a test when it shouldn't - especially if we're using Truncate(time.Second)
If we used a fixed test time here, there'd be no risk of that type of mistake creeping in. For example:
fakeTime1 := time.UnixMicro(10)What do you think?
There was a problem hiding this comment.
Thanks, I think there definitely could be an issue like that in the future. However, I think that some tests might still succeed incorrectly when the date is set far in the past or far in the future (depending on the test). I think it might be useful to use a random date here, such a test will fail eventually, indicating that we aren't using the correct clock somewhere in code.
| // NOTE: this code is just a workaround for cmutil only accepting the certificaterequest object | ||
| func SetCertificateRequestStatusCondition( |
There was a problem hiding this comment.
comment: It's a shame to have this code here - functions which act on cert-manager resources such as CertificateRequest seem like a natural fit for the cert-manager codebase. Better to have one source of truth for "how do I update a CR status condition".
Don't think it's a dealbreaker - but I wonder if we could add something to the cert-manager codebase instead? Doesn't really matter though
| // Update the status with the provided condition details & return | ||
| // the added condition. | ||
| // NOTE: this code is just a workaround for cmutil only accepting a GenericIssuer interface |
There was a problem hiding this comment.
Definitely makes sense for his repo's issuer type to have these kinds of functions be more specific!
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon, SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR fixes a bug that caused the
LastTransitionTimeto be updated every time a condition is changed.Instead, we now only change the
LastTransitionTimevalue when the status of the condition changed (as described in the comments for this field).This PR also makes it so the Clock is passed explicitly in the reconcilers/ functions. This allows us to better test using fake Clocks.