-
Notifications
You must be signed in to change notification settings - Fork 41.6k
Graduate PodLifecycleSleepAction to GA #132595
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
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
we have the tests running without issues, have you verified? |
The aggregated results shows no error in recent 15 days: |
/lgtm |
|
LGTM label has been added. Git tree hash: 4d665364032cff9c261847cf07c6464575dfa395
|
|
I do not see those tests running on the testgrid , I'm looking for "Lifecycle Sleep Hook" |
|
I've merged my changes for KEP 4818. I guess I should've used the squash and merge option? I can clean up the merge commit too. @AxeZhan what do you think? |
I think you can still squash on this branch? You should have the permissions. @sreeram-venkitesh |
31a8c4a to
83e92dd
Compare
|
I've cleaned the merge commit and squashed my changes into a single commit. CC: @thockin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
as a follow up these tests may be moved to Conformance
|
LGTM label has been added. Git tree hash: b3117746f56b7bade63755915b2b17753318890e
|
| invalidStr := fmt.Sprintf("must be non-negative and less than terminationGracePeriodSeconds (%d)", *gracePeriod) | ||
| allErrors = append(allErrors, field.Invalid(fldPath, sleep.Seconds, invalidStr)) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to leave the gated logic in until we remove the gate - compatVersions mean someone can configure this to behave as 1.33 or 1.32, where the gate can be disabled. It seems minor for this particular KEP, but its a principle thing.
But also, if you WERE killing this logic, shouldn't you ALSO remove the definiton of opts.AllowPodLifecycleSleepActionZeroValue (pkg/api/pod/util.go)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yes, I didn't think about the emulated versions.
But also, if you WERE killing this logic, shouldn't you ALSO remove the definiton of opts.AllowPodLifecycleSleepActionZeroValue (pkg/api/pod/util.go)?
Yep, I'll add the gated logic back in validation and remove everything together when the gate is removed.
83e92dd to
00bf3b3
Compare
|
/gtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/approve
|
LGTM label has been added. Git tree hash: 20b3ffa0eea248b88a2d5158dc738125678eabfd
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, AxeZhan, SergeyKanzhelev, thockin 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Graduate PodLifecycleSleepAction to GA
Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: