cluster-api
cluster-api copied to clipboard
🌱 Update conditions.Set function to set LastTransitionTime only when status of condition changes
What this PR does / why we need it:
This PR existing behavior of Set condition function as follows
Existing behavior
- Update condition.LastTransitionTime to current time whenever any one of the fields(Status, Reason, Severity, Message) changes in new condition.
- Sets the condition.LastTransitionTime to newConidtion.LastTransitionTime if defined only when the newCondition does not exist.
Proposed behavior
- Update condition.LastTransitionTime to current time only when Status field of new condition differ from existing condition status.
- Sets the condition.LastTransitionTime to newCondition.LastTransitionTime if defined whether the newCondition already exist or new.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #11033
@fabriziopandini @sbueringer Please review when you get time. Thank you.
/retitle 🌱 Update conditions.Set function to set LastTransitionTime only when status of condition changes
/area util
/assign @fabriziopandini
Please take a look at this PR, when you get chance. Thank you.
Heyho, sorry should have brought this up earlier. The more I'm thinking about this the more I'm wondering if we should really make this change. Basically we are now moving towards v1beta2 conditions and this util will be removed sooner or later. So I'm not sure if we really should make a behavioral change. The util for v1beta2 already behaves the correct way.
@fabriziopandini @vincepri WDYT?
If this is really absolutely needed, I would recommend that we an additional Set func with a different name and keep the current Set exactly as is.
I'm ok in adding a separated func (e.g. SetEx), it is a practical weys to avoid issues on old utils considering the deprecation/removal plan
I can do that, But who will be the caller of new function, is it like if the providers are interested they can make use of it right?
Yup providers and the folks that opened and engaged on the issue. But nothing in this repo, we are moving to v1beta2 conditions instead and the new v1beta2 utils already have this behavior
Created a new function with the requested logic, Please take a look and suggest better name if any. Thanks
Perfect, thank you very much!
/lgtm /approve
LGTM label has been added.
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: sbueringer
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~OWNERS~~ [sbueringer]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Perfect, thank you very much!
/lgtm /approve
Thanks, I forgot to change the PR title, It may mislead.
You can still edit the title now. The release notes tool will use the latest PR title (merge commit won't but it's okay, that happens)