Add validation rule for 'noRetry' parameter in ProvisioningRequestSpec #7547
Conversation
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
added
the
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omerap12 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
added
the
needs-rebase
Signed-off-by: Omer Aplatony <[email protected]>
k8s-ci-robot
removed
the
needs-rebase
|
friendly ping :) @PBundyra |
Signed-off-by: Omer Aplatony <[email protected]>
It lgtm but I don't have approvers rights |
| @@ -113,6 +113,7 @@ type ProvisioningRequestSpec struct { | |||
| // +optional | |||
| // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable" | |||
| // +kubebuilder:validation:MaxProperties=100 | |||
| // +kubebuilder:validation:XValidation:rule="self.all(k, v, if parent.provisioningClassName == 'check-capacity.autoscaling.x-k8s.io' then k == 'noRetry' && v in ['true', 'false'] else true)",message="For check-capacity.autoscaling.x-k8s.io, only 'noRetry' parameter is supported with values 'true' or 'false'" | |||
There was a problem hiding this comment.
I'm not sure we want to validate on the API level that only noRetry parameter is set.
My understanding is that the entire reason Parameters map exists is easy customisation for non-standard controllers, without requiring an API changes. If we're going to introduce it on the API level, we might just as well add a new field for it (although in this case I would suggest using maxRetries, since it's more flexible). It'd also be cleaner than code in comment, especially with a potentially growing set of parameters/classes to allow.
@mwielgus can you PTAL and decide if this is the way to go or not? Since this is effectively an API level change, it would require SIG chair approval anyway. My opinion is that we shouldn't go with it.
There was a problem hiding this comment.
I would rather avoid adding explicit check for parameters. Parameters may be different for different classes and may come and go.
|
/hold As discussed in review comments, I don't think we want to go forward with this approach. It changes the API definition - even though this definition happens to live in CA directory structure (which is somehow unfortunate), it's the only source-of-truth for ProvisioningRequest API, and not just a part of CA - and effectively obsoletes the I believe the scope of #7511 should be limited to validation of values of supported parameters on the CA side (in its code). Something like:
If (and when) needed, we could also introduce either CA flags or known parameters to control CA's exact behaviour, for example if a user wants it to reject requests with unsupported parameters. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/kind api-change |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds parameter validation for check-capacity.autoscaling.x-k8s.io provisioning class to validate that only the 'noRetry' parameter is allowed with values 'true' or 'false'. This helps users by providing immediate feedback when invalid parameters are used rather than discovering issues at runtime.
Which issue(s) this PR fixes:
Fixes #7511
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: