fix: validate v1beta1 CloneSet progress deadline#2465
Conversation
Signed-off-by: immanuwell <pchpr.00@list.ru>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @immanuwell! It looks like this is your first PR to openkruise/kruise 🎉 |
There was a problem hiding this comment.
Pull request overview
Adds missing validation for spec.progressDeadlineSeconds in the v1beta1 CloneSet validating webhook to match existing v1alpha1 behavior, and introduces a focused unit test to prevent regressions.
Changes:
- Add v1beta1 validation to reject negative
progressDeadlineSecondsand values<= minReadySeconds(exceptMaxInt32). - Add a new unit test covering valid/invalid
progressDeadlineSecondscombinations for v1beta1 CloneSet validation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/webhook/cloneset/validating/validation.go | Adds v1beta1 progressDeadlineSeconds validation parity with v1alpha1. |
| pkg/webhook/cloneset/validating/validation_test.go | Adds a unit test ensuring v1beta1 validation rejects invalid progressDeadlineSeconds values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if spec.ProgressDeadlineSeconds != nil { | ||
| allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.ProgressDeadlineSeconds), fldPath.Child("progressDeadlineSeconds"))...) | ||
| if *spec.ProgressDeadlineSeconds != math.MaxInt32 && *spec.ProgressDeadlineSeconds <= spec.MinReadySeconds { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("progressDeadlineSeconds"), spec.ProgressDeadlineSeconds, "must be greater than minReadySeconds")) |
| hasError := false | ||
| for _, err := range errs { | ||
| if err.Field == "spec.progressDeadlineSeconds" { | ||
| hasError = true | ||
| break | ||
| } | ||
| } | ||
| if hasError != tt.expectError { | ||
| t.Errorf("expected progressDeadlineSeconds error: %v, got errors: %v", tt.expectError, errs) | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2465 +/- ##
==========================================
+ Coverage 49.08% 49.15% +0.07%
==========================================
Files 325 325
Lines 28112 28116 +4
==========================================
+ Hits 13798 13820 +22
+ Misses 12745 12714 -31
- Partials 1569 1582 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
I. Describe what this PR does
Adds the missing v1beta1 CloneSet validation for
spec.progressDeadlineSeconds.v1alpha1 already rejects negative values, and values <=
minReadySecondsexceptMaxInt32. v1beta1 missed that check. Tiny parity fix, nothing fancy.II. Does this pull request fix one issue?
NONE
III. Describe how to verify it
The added test reproduces the old bug: invalid v1beta1 values like
-1, or10withminReadySeconds: 10, got nospec.progressDeadlineSecondserror. Pretty easy to miss tbh.IV. Special notes for reviews
No matching issue found. Closest context is #2225, which added the v1beta1 CloneSet path.