API: enhance validation with ErrorMatcher #132577
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. |
| @@ -71,19 +71,19 @@ func validateOwnerReference(ownerReference metav1.OwnerReference, fldPath *field | |||
| gvk := schema.FromAPIVersionAndKind(ownerReference.APIVersion, ownerReference.Kind) | |||
| // gvk.Group is empty for the legacy group. | |||
| if len(gvk.Version) == 0 { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVersion"), ownerReference.APIVersion, "version must not be empty")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVersion"), ownerReference.APIVersion, "version must not be empty").WithOrigin("owner-reference")) | |||
There was a problem hiding this comment.
Or field.Required with no details?
Same below.
| @@ -216,12 +216,12 @@ func ValidateFieldManager(fieldManager string, fldPath *field.Path) field.ErrorL | |||
| // considered as not set and is defaulted by the rest of the process | |||
| // (unless apply is used, in which case it is required). | |||
| if len(fieldManager) > FieldManagerMaxLength { | |||
| allErrs = append(allErrs, field.TooLong(fldPath, "" /*unused*/, FieldManagerMaxLength)) | |||
| allErrs = append(allErrs, field.TooLong(fldPath, "" /*unused*/, FieldManagerMaxLength).WithOrigin("field-manager")) | |||
There was a problem hiding this comment.
FieldManagerMaxLength gets embedded in a generated details string.
The use of ErrorMatcher and origins makes it possible to remove special detail strings from the resource.k8s.io validation, which is good because it's shorter and avoids the need to update those tests when the shared validation helpers change. For that to be more complete, several missing WithOrigin calls were added and some were changed to make them consistent. The following rationale was used: - All origins should be named after the code generating them. "labelKey" in ValidateLabelName did not follow that because it passed through errors from IsQualifiedName, so this was changed to "format=qualified-name", making it consistent with ValidateQualifiedName. - WithOrigin is useful when the corresponding error has a non-trivial detail string, because then validation code can check for the origin instead of string. - WithOrigin is not useful for errors with no detail string, for example field.Required, because adding one would just make error matching harder without a benefit. Unfortunately object meta name validation still has to match strings because the current code has no knowledge about which origin to add. Changing that is a bigger internal code change. For resource.k8s.io validation, matching the same wanted error against multiple actual errors is undesirable because it could be a bug in the test case. The new WithUniqueMatches makes the matching stricter than the current default.
5c56512 to
27c1a30
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly 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 |
|
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The use of ErrorMatcher and origins makes it possible to remove special detail strings from the resource.k8s.io validation, which is good because it's shorter and avoids the need to update those tests when the shared validation helpers change.
For that to be more complete, several missing WithOrigin calls were added and some were changed to make them consistent. The following rationale was used:
Unfortunately object meta name validation still has to match strings because the current code has no knowledge about which origin to add. Changing that is a bigger internal code change.
For resource.k8s.io validation, matching the same wanted error against multiple actual errors is undesirable because it could be a bug in the test case. The new WithUniqueMatches makes the matching stricter than the current default.
Which issue(s) this PR is related to:
#132314 (comment)
Special notes for your reviewer:
Does this PR introduce a user-facing change?
/assign @thockin