-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
🌱 Add new utility function to set LastTransitionTime only when status of condition changes #11176
Conversation
@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 |
09ecb1f
to
0a87b8a
Compare
0a87b8a
to
0d69527
Compare
Please take a look at this PR, when you get chance. Thank you. |
Heyho, @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 |
0d69527
to
aa1e14e
Compare
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 |
LGTM label has been added. Git tree hash: 164d6c8fb15f9f33031276a13a899a6676379035
|
[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 |
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) |
What this PR does / why we need it:
This PR existing behavior of Set condition function as follows
Existing behavior
Proposed behavior
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