Cleanup: Remove field name from invalid field detail message #132513
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @xiaoweim. Thanks for your PR. I'm waiting for a kubernetes 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-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xiaoweim 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 |
|
/priority important-longterm |
|
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. |
|
/sig api-machinery |
|
/cc @aaron-prindle |
|
/test pull-kubernetes-unit-windows-master |
67d095a to
2e14727
Compare
|
/retest |
thockin
reviewed
Jul 1, 2025
| @@ -1009,7 +1009,7 @@ func validateVariable(compiler plugincel.Compiler, v *admissionregistration.Vari | |||
| allErrors = append(allErrors, field.Required(fldPath.Child("name"), "name is not specified")) | |||
| } else { | |||
| if !isCELIdentifier(v.Name) { | |||
| allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), v.Name, "name is not a valid CEL identifier")) | |||
| allErrors = append(allErrors, field.Invalid(fldPath.Child("name"), v.Name, "is not a valid CEL identifier")) | |||
There was a problem hiding this comment.
"must be a valid..." -- there's a convention for errors in the API conventions doc.
| @@ -2904,7 +2904,7 @@ func TestValidateValidatingAdmissionPolicy(t *testing.T) { | |||
| }, | |||
| }, | |||
| }, | |||
| expectedError: `spec.variables[0].name: Invalid value: "4ever": name is not a valid CEL identifier`, | |||
| expectedError: `spec.variables[0].name: Invalid value: "4ever": is not a valid CEL identifier`, | |||
| @@ -514,7 +514,7 @@ func validateJobStatus(job *batch.Job, fldPath *field.Path, opts JobStatusValida | |||
| } | |||
| if opts.RejectCompletionTimeBeforeStartTime { | |||
| if status.StartTime != nil && status.CompletionTime != nil && status.CompletionTime.Before(status.StartTime) { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("completionTime"), status.CompletionTime, "completionTime cannot be set before startTime")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("completionTime"), status.CompletionTime, "cannot be set before startTime")) | |||
There was a problem hiding this comment.
"may not be before" or "must be equal to or after"
| @@ -540,7 +540,7 @@ func validateJobStatus(job *batch.Job, fldPath *field.Path, opts JobStatusValida | |||
| } | |||
| if opts.RejectFinishedJobWithUncountedTerminatedPods { | |||
| if isJobFinished && status.UncountedTerminatedPods != nil && len(status.UncountedTerminatedPods.Failed)+len(status.UncountedTerminatedPods.Succeeded) > 0 { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("uncountedTerminatedPods"), status.UncountedTerminatedPods, "uncountedTerminatedPods needs to be empty for finished job")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("uncountedTerminatedPods"), status.UncountedTerminatedPods, "needs to be empty for finished job")) | |||
thockin
reviewed
Jul 1, 2025
| @@ -692,7 +692,7 @@ func ValidateJobStatusUpdate(job, oldJob *batch.Job, opts JobStatusValidationOpt | |||
| // we don't want to block transitions to completionTime = nil when the job is not finished yet. | |||
| // Setting completionTime = nil for finished jobs is prevented in RejectCompleteJobWithoutCompletionTime. | |||
| if job.Status.CompletionTime != nil && oldJob.Status.CompletionTime != nil && !ptr.Equal(job.Status.CompletionTime, oldJob.Status.CompletionTime) { | |||
| allErrs = append(allErrs, field.Invalid(statusFld.Child("completionTime"), job.Status.CompletionTime, "completionTime cannot be mutated")) | |||
| allErrs = append(allErrs, field.Invalid(statusFld.Child("completionTime"), job.Status.CompletionTime, "cannot be mutated")) | |||
| @@ -78,7 +78,7 @@ func ValidateStorageVersionMigrationStatusUpdate(newSVMBundle, oldSVMBundle *sto | |||
|
|
|||
| // resource version should not change once it has been set | |||
| if len(oldSVMBundle.Status.ResourceVersion) != 0 && oldSVMBundle.Status.ResourceVersion != newSVMBundle.Status.ResourceVersion { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceVersion"), newSVMBundle.Status.ResourceVersion, "resourceVersion cannot be updated")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceVersion"), newSVMBundle.Status.ResourceVersion, "cannot be updated")) | |||
| @@ -136,7 +136,7 @@ func newNodeLogQuery(query url.Values) (*nodeLogQuery, field.ErrorList) { | |||
| // Prevent specifying an empty or blank space query. | |||
| // Example: kubectl get --raw /api/v1/nodes/$node/proxy/logs?query=" " | |||
| if ok && (len(nlq.Files) == 0 && len(nlq.Services) == 0) { | |||
| allErrs = append(allErrs, field.Invalid(field.NewPath("query"), queries, "query cannot be empty")) | |||
| allErrs = append(allErrs, field.Invalid(field.NewPath("query"), queries, "cannot be empty")) | |||
| @@ -243,7 +243,7 @@ func (n *nodeLogQuery) validate() field.ErrorList { | |||
| } | |||
|
|
|||
| if n.Boot != nil && runtime.GOOS == "windows" { | |||
| allErrs = append(allErrs, field.Invalid(field.NewPath("boot"), *n.Boot, "boot is not supported on Windows")) | |||
| allErrs = append(allErrs, field.Invalid(field.NewPath("boot"), *n.Boot, "is not supported on Windows")) | |||
| @@ -241,7 +241,7 @@ func validateClaimValidationRules(compiler authenticationcel.Compiler, state *va | |||
| fldPath := fldPath.Index(i) | |||
|
|
|||
| if len(rule.Expression) > 0 && !structuredAuthnFeatureEnabled { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("expression"), rule.Expression, "expression is not supported when StructuredAuthenticationConfiguration feature gate is disabled")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("expression"), rule.Expression, "is not supported when StructuredAuthenticationConfiguration feature gate is disabled")) | |||
| @@ -251,15 +251,15 @@ func validateClaimValidationRules(compiler authenticationcel.Compiler, state *va | |||
| allErrs = append(allErrs, field.Required(fldPath, "claim or expression is required")) | |||
| case len(rule.Claim) > 0: | |||
| if len(rule.Message) > 0 { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("message"), rule.Message, "message can't be set when claim is set")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("message"), rule.Message, "can't be set when claim is set")) | |||
| } | ||
| if seenClaims.Has(rule.Claim) { | ||
| allErrs = append(allErrs, field.Duplicate(fldPath.Child("claim"), rule.Claim)) | ||
| } | ||
| seenClaims.Insert(rule.Claim) | ||
| case len(rule.Expression) > 0: | ||
| if len(rule.RequiredValue) > 0 { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("requiredValue"), rule.RequiredValue, "requiredValue can't be set when expression is set")) | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("requiredValue"), rule.RequiredValue, "can't be set when expression is set")) |
| @@ -532,7 +532,7 @@ func validatePrefixClaimOrExpression(compiler authenticationcel.Compiler, mappin | |||
| var err *field.Error | |||
|
|
|||
| if mapping.Prefix != nil { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("prefix"), *mapping.Prefix, "prefix can't be set when expression is set")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("prefix"), *mapping.Prefix, "can't be set when expression is set")) | |||
| @@ -753,7 +753,7 @@ func compileMatchConditions(compiler authorizationcel.Compiler, matchConditions | |||
| var allErrs field.ErrorList | |||
| // should fail when match conditions are used without feature enabled | |||
| if len(matchConditions) > 0 && !structuredAuthzFeatureEnabled { | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("matchConditions"), "", "matchConditions are not supported when StructuredAuthorizationConfiguration feature gate is disabled")) | |||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("matchConditions"), "", "are not supported when StructuredAuthorizationConfiguration feature gate is disabled")) | |||
| @@ -990,15 +990,15 @@ func TestValidateClaimValidationRules(t *testing.T) { | |||
| {Claim: "claim", Message: "message"}, | |||
| }, | |||
| structuredAuthnFeatureEnabled: true, | |||
| want: `issuer.claimValidationRules[0].message: Invalid value: "message": message can't be set when claim is set`, | |||
| want: `issuer.claimValidationRules[0].message: Invalid value: "message": can't be set when claim is set`, | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This pull request addresses issue #111698 by removing redundant field name from
field.Invaliddetail message.Validation error message for invalid field already includes the field name, a duplicate field name included in the detail message is unnecessary.
Which issue(s) this PR is related to:
Fixes #111698
Special notes for your reviewer:
Here is a list of all the instances of
field.Invalid: https://gist.github.com/xiaoweim/a5df612c03cead0293ecf25a8ea8b28eDoes this PR introduce a user-facing change?