Add CEL rules to ResourceFlavor #1958
Conversation
k8s-ci-robot
added
release-note-action-required
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @IrvingMg. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
k8s-ci-robot
added
needs-ok-to-test
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
/assign |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -78,6 +79,11 @@ type ResourceFlavorSpec struct { | |||
| // +optional | |||
| // +listType=atomic | |||
| // +kubebuilder:validation:MaxItems=8 | |||
| // +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.key) ? x.operator == 'Exists' : true)", message="operator must be Exists when 'key' is empty, which means 'match all values and all keys'" | |||
There was a problem hiding this comment.
Just for discussion, from maintenance, do you guys think this is more simple comparing to the codes.
There was a problem hiding this comment.
From maintenance POV it depends on how the rules look, From a runtime POV if we are able to fully drop the webhook for a type (like it was the case for local queue) then it's a win.
There was a problem hiding this comment.
Make sense, apply the same policy through the whole project is important to avoid confusion, unless we can't fully remove the validation.
There was a problem hiding this comment.
/lgtm
Even if we ware not able to do too much about this.
@alculquicondor maybe for a followup, If we move the finalizer addition from the defaulter (and fully manage that in the controller) we can drop the mutation web-hook for this.
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 354007c50dbf3b3d2fc28d08bae7dbde2d433f29
|
k8s-ci-robot
removed
the
lgtm
| @@ -111,8 +111,6 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL | |||
| if errs := validation.IsValidLabelValue(currTaint.Value); len(errs) != 0 { | |||
| allErrors = append(allErrors, field.Invalid(idxPath.Child("value"), currTaint.Value, strings.Join(errs, ";"))) | |||
| } | |||
| // validate the taint effect | |||
| allErrors = append(allErrors, validateTaintEffect(&currTaint.Effect, false, idxPath.Child("effect"))...) | |||
|
|
|||
| // validate if taint is unique by <key, effect> | |||
| if len(uniqueTaints[currTaint.Effect]) > 0 && uniqueTaints[currTaint.Effect].Has(currTaint.Key) { | |||
There was a problem hiding this comment.
I think we can replace uniqueness checks by using +listType=map and adding the appropriate keys to use.
But we can leave that for a follow up PR.
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 1a112b525742df57bc6e54b1afc3c99638fdbb95
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, IrvingMg 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 |
k8s-ci-robot
added
the
approved
* Add CEL rules to ResourceFlavor * Update comments * Add issue reference to TODO comment * Group similar test cases with DescribeTable * Update validation rule
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR replaces some of the validations executed by webhooks for the
resourceflavortype with CRD validation rules.Which issue(s) this PR fixes:
Relates to #463
Special notes for your reviewer:
Due to the resource constraints imposed by the API Server, only have been replaced those validations which costs don't exceed the limits.
Does this PR introduce a user-facing change?