Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Add new utility function to set LastTransitionTime only when status of condition changes #11176

Merged
merged 3 commits into from
Nov 19, 2024

Conversation

Karthik-K-N
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 12, 2024
@Karthik-K-N
Copy link
Contributor Author

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

@chrischdi
Copy link
Member

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

@k8s-ci-robot k8s-ci-robot changed the title 🌱 Update condition Set function to set Last Trasition Time only when status of condition changes 🌱 Update conditions.Set function to set LastTransitionTime only when status of condition changes Sep 12, 2024
@chrischdi
Copy link
Member

/area util

@k8s-ci-robot k8s-ci-robot added area/util Issues or PRs related to utils and removed do-not-merge/needs-area PR is missing an area label labels Sep 12, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2024
@sbueringer
Copy link
Member

/assign @fabriziopandini

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2024
@Karthik-K-N
Copy link
Contributor Author

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

@sbueringer
Copy link
Member

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.

@fabriziopandini
Copy link
Member

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

@Karthik-K-N
Copy link
Contributor Author

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?

@sbueringer
Copy link
Member

sbueringer commented Nov 19, 2024

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

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024
@Karthik-K-N
Copy link
Contributor Author

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

@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 19, 2024
@sbueringer
Copy link
Member

Perfect, thank you very much!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 164d6c8fb15f9f33031276a13a899a6676379035

@k8s-ci-robot
Copy link
Contributor

[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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2024
@k8s-ci-robot k8s-ci-robot merged commit a121afc into kubernetes-sigs:main Nov 19, 2024
20 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Nov 19, 2024
@Karthik-K-N
Copy link
Contributor Author

Perfect, thank you very much!

/lgtm /approve

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

@sbueringer
Copy link
Member

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)

@Karthik-K-N Karthik-K-N changed the title 🌱 Update conditions.Set function to set LastTransitionTime only when status of condition changes 🌱 Add new utility function to set LastTransitionTime only when status of condition changes Nov 20, 2024
@Karthik-K-N Karthik-K-N deleted the update_condition branch November 20, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/util Issues or PRs related to utils cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

conditions Utility to let specify the entire condition
5 participants