-
Notifications
You must be signed in to change notification settings - Fork 752
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 validation for the WorkloadSpreadSubset patch field #1237
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #1237 +/- ##
==========================================
- Coverage 48.45% 48.38% -0.08%
==========================================
Files 151 151
Lines 21127 21178 +51
==========================================
+ Hits 10238 10247 +9
- Misses 9772 9807 +35
- Partials 1117 1124 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
pkg/webhook/workloadspread/validating/workloadspread_validation.go
Outdated
Show resolved
Hide resolved
pkg/webhook/workloadspread/validating/workloadspread_validation.go
Outdated
Show resolved
Hide resolved
@@ -169,7 +200,7 @@ func validateWorkloadSpreadSpec(obj *appsv1alpha1.WorkloadSpread, fldPath *field | |||
return allErrs | |||
} | |||
|
|||
func validateWorkloadSpreadSubsets(ws *appsv1alpha1.WorkloadSpread, subsets []appsv1alpha1.WorkloadSpreadSubset, fldPath *field.Path) field.ErrorList { | |||
func validateWorkloadSpreadSubsets(ws *appsv1alpha1.WorkloadSpread, subsets []appsv1alpha1.WorkloadSpreadSubset, workloadTemplate client.Object, fldPath *field.Path) field.ErrorList { |
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.
consider using podtemplate instead of workloadTemplate as function parameters
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.
Sure. I'll change it.
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.
I'm not sure whether the podtemplate is o good idea because it's actually a workload such as CloneSet and Deployment.
allErrs = append(allErrs, field.Invalid(fldPath.Index(i).Child("patch"), newPod, fmt.Sprintf("Convert_v1_Pod_To_core_Pod failed: %v", err))) | ||
} | ||
allErrs = append(allErrs, corevalidation.ValidatePodSpec(&coreNewPod.Spec, &coreNewPod.ObjectMeta, fldPath.Index(i).Child("patch"), webhookutil.DefaultPodValidationOptions)...) | ||
} |
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.
how about the case that workloadTemplate is nil ? in such case, we cannot use ValidatePodSpec directly, instead, we should check whether the patch had change some fields that should not be changed such as affinity
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.
Yes. I'll add validation for this case.
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
af71a64
to
0f3e48f
Compare
Signed-off-by: chengleqi <chengleqi5g@hotmail.com>
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 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: furykerry 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 |
Ⅰ. Describe what this PR does
Add validation for the WorkloadSpreadSubset patch field
When scheduling-related parameters exist in the patch without validation, they may conflict with the subset of workloadspread. Therefore, this type of patch should be excluded.
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. Describe how to verify it
kubectl apply -f config/samples/apps_v1alpha1_cloneset.yaml
kubectl apply -f workloadspread.yaml
It will report the following error:
Ⅳ. Special notes for reviews