cluster-api icon indicating copy to clipboard operation
cluster-api copied to clipboard

🌱 Update conditions.Set function to set LastTransitionTime only when status of condition changes

Open Karthik-K-N opened this issue 1 year ago • 5 comments

What this PR does / why we need it:

This PR existing behavior of Set condition function as follows

Existing behavior

  1. Update condition.LastTransitionTime to current time whenever any one of the fields(Status, Reason, Severity, Message) changes in new condition.
  2. Sets the condition.LastTransitionTime to newConidtion.LastTransitionTime if defined only when the newCondition does not exist.

Proposed behavior

  1. Update condition.LastTransitionTime to current time only when Status field of new condition differ from existing condition status.
  2. 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

Karthik-K-N avatar Sep 12 '24 07:09 Karthik-K-N

@fabriziopandini @sbueringer Please review when you get time. Thank you.

Karthik-K-N avatar Sep 12 '24 07:09 Karthik-K-N

/retitle 🌱 Update conditions.Set function to set LastTransitionTime only when status of condition changes

chrischdi avatar Sep 12 '24 12:09 chrischdi

/area util

chrischdi avatar Sep 12 '24 12:09 chrischdi

/assign @fabriziopandini

sbueringer avatar Oct 09 '24 08:10 sbueringer

Please take a look at this PR, when you get chance. Thank you.

Karthik-K-N avatar Nov 04 '24 06:11 Karthik-K-N

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.

sbueringer avatar Nov 15 '24 13:11 sbueringer

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

fabriziopandini avatar Nov 18 '24 08:11 fabriziopandini

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?

Karthik-K-N avatar Nov 19 '24 04:11 Karthik-K-N

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

sbueringer avatar Nov 19 '24 07:11 sbueringer

Created a new function with the requested logic, Please take a look and suggest better name if any. Thanks

Karthik-K-N avatar Nov 19 '24 13:11 Karthik-K-N

Perfect, thank you very much!

/lgtm /approve

sbueringer avatar Nov 19 '24 18:11 sbueringer

LGTM label has been added.

Git tree hash: 164d6c8fb15f9f33031276a13a899a6676379035

k8s-ci-robot avatar Nov 19 '24 18:11 k8s-ci-robot

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot avatar Nov 19 '24 18:11 k8s-ci-robot

Perfect, thank you very much!

/lgtm /approve

Thanks, I forgot to change the PR title, It may mislead.

Karthik-K-N avatar Nov 20 '24 03:11 Karthik-K-N

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)

sbueringer avatar Nov 20 '24 05:11 sbueringer