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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 2 commits into from

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
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from fabriziopandini. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2024
fabriziopandini
fabriziopandini reviewed Oct 21, 2024
View reviewed changes
util/conditions/setter_test.go
{
name: "Set a condition that already exists but with different Message should preserve the last transition time",
to: setterWithConditions(fooWithLastTransitionTime),
new: fooWithBarMessage,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding also a test where the new condition has a last transition time time set (but it gets ignored)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new test in separate commit, Please let me know if anything else needs to be added. Thank you.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enxebre enxebre Awaiting requested review from enxebre

@sbueringer sbueringer Awaiting requested review from sbueringer

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@fabriziopandini fabriziopandini

Labels
area/util Issues or PRs related to utils cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

conditions Utility to let specify the entire condition
5 participants
@Karthik-K-N @chrischdi @sbueringer @k8s-ci-robot @fabriziopandini