refactor(validate): consume cli's structured validation API#347
Open
jcogilvie wants to merge 3 commits into
Open
refactor(validate): consume cli's structured validation API#347jcogilvie wants to merge 3 commits into
jcogilvie wants to merge 3 commits into
Conversation
crossplane/cli#66 lands a structured-result validate API at pkg/validate.SchemaValidate, replacing the previous cmd/crossplane/validate.SchemaValidation entry point that produced its results by writing prefix-marked lines to a writer. Switch schema_validator.go to the new API so that: - Validation errors come from typed FieldValidationError records rather than parsed [x]/[!] lines, removing the extractValidationErrors stdout-parsing helper and its test. - The bytes.Buffer + io.MultiWriter + loggerwriter indirection used to capture rendered output is gone; per-resource debug logging is now explicit via logResultDetails. - Failure detection runs through pkg/validate.ResultError, which preserves the historical error-on-missing-schemas semantic. The previous SchemaValidation fused defaulting with validation, mutating the caller's resources in place. SchemaValidate deep-copies its inputs, so defaulting no longer happens as a side-effect. Apply defaults explicitly via pkg/xr.ApplyCRDDefaults before validation to preserve the downstream invariant that the diff calculator sees fully-defaulted resources (covered by TestDefaultSchemaValidator_ValidateResources_AppliesDefaults). Also rename the call site in diff_processor.go from render.DefaultValues to xr.ApplyCRDDefaults: the helper kept its signature but was promoted out of cmd/crossplane/render into the new pkg/xr library package in the same upstream cleanup wave. go.mod pins crossplane/cli to v2.4.0-rc.0.0.20260615182009-ba59fbfac34b (the merge commit of crossplane/cli#66) and pulls crossplane-runtime/v2 and crossplane/apis/v2 forward to v2.4.0-rc.0 to satisfy the cli's transitive constraints. Bump to a tagged release once one is cut. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors schema validation in crossplane-diff to consume the Crossplane CLI’s new structured validation API (typed ValidationResult / FieldValidationError) instead of parsing rendered stdout, while preserving prior semantics around missing schemas and defaulting-before-diff.
Changes:
- Switch schema validation to
pkg/validate.SchemaValidate+pkg/validate.ResultError, removing the prior stdout parsing approach. - Apply CRD defaults explicitly via
pkg/xr.ApplyCRDDefaultsbefore diff calculation to preserve defaulted-resource invariants. - Update dependencies to consume the required Crossplane CLI pseudo-version and related
crossplane-runtime/apisRC versions.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
go.mod |
Bumps github.com/crossplane/cli/v2 (pseudo-version) and updates Crossplane RC dependencies to satisfy new structured validation API constraints. |
go.sum |
Updates module checksums corresponding to the dependency bumps. |
cmd/diff/diffprocessor/schema_validator.go |
Replaces stdout-based validation parsing with structured ValidationResult handling; adds explicit defaulting and debug logging helpers. |
cmd/diff/diffprocessor/schema_validator_test.go |
Replaces parsing tests with formatValidationErrors tests; keeps regression coverage for in-place defaulting behavior. |
cmd/diff/diffprocessor/diff_processor.go |
Updates XR defaulting call site to use pkg/xr.ApplyCRDDefaults (renamed/promoted from render.DefaultValues). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+331
to
+343
| case pkgvalidate.ValidationStatusInvalid: | ||
| for _, e := range r.Errors { | ||
| if e.Type == pkgvalidate.FieldErrorTypeDefaulting { | ||
| // ResultError treats a defaulting error as a | ||
| // failure only when accompanied by a schema-class | ||
| // error on the same resource. The schema-class | ||
| // error already gets its own entry; emitting the | ||
| // defaulting line too would just be noise. | ||
| continue | ||
| } | ||
|
|
||
| msgs = append(msgs, fmt.Sprintf("schema validation error %s, %s : %s", gvk, r.Name, e.Message)) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Good catch — addressed in 630612c.
You're right that the silent-drop is wrong even though it doesn't trip today. Upstream's statusFromErrors guarantees an Invalid resource has at least one non-defaulting error, so the suppression always lands on a useful entry. But if that contract ever shifted, the formatter would fall through to the generic schema validation failed string and the actual cause would disappear.
Now suppression only happens when another actionable (schema / CEL / unknown-field) error is present on the same resource. With only defaulting errors, they get emitted as-is. New InvalidWithOnlyDefaultingErrorsStillEmitted test case covers it.
Address Copilot review feedback on PR #347. formatValidationErrors previously dropped every FieldErrorTypeDefaulting entry on Invalid resources unconditionally. Upstream's statusFromErrors guarantees an Invalid resource carries at least one non-defaulting error today, so in practice this produced the right output — but if that contract ever shifted, an Invalid resource whose Errors are all defaulting would silently produce zero per-resource messages and the formatter would fall through to the generic "schema validation failed" string, hiding the real cause. Suppress defaulting only when there is another actionable (schema / CEL / unknown-field) error on the same resource. With only defaulting errors present, emit them as-is. Tested by the new InvalidWithOnlyDefaultingErrorsStillEmitted case. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
…rce blocks
The previous formatValidationErrors mirrored the upstream text
renderer's prefix-marked output (`[x] schema validation error <gvk>,
<name> : <msg>`) because the original implementation literally parsed
that text. With structured FieldValidationError records now in hand,
the mirroring is gone: the format is multi-line, grouped per resource,
and surfaces information the line-parser couldn't.
Output shape:
<apiVersion>/<Kind> <ns>/<name>:
<message>[ (got <value>)] [<type>]
...
<apiVersion>/<Kind>: missing schema
Per-resource grouping replaces a flat list with N entries duplicating
the GVK on each line. The error type ([schema] / [cel] /
[unknownField] / [defaulting]) makes the failure class visible
without re-parsing the message. Bad values from
FieldValidationError.Value are appended as "(got <value>)" only when
they aren't already substring-present in the message — k8s-derived
errors typically embed the bad value in the message text, so this
avoids duplication.
The "schema validation failed" fallback is gone: formatValidationErrors
is invoked only after pkgvalidate.ResultError has flagged a failure,
so an empty result is unreachable in production. Returning "" lets
the wrapping SchemaValidationError carry the underlying ResultError
message instead of substituting a misleading generic string.
The integration TestDiffIntegration/SchemaValidationError previously
asserted on `Contains "schema validation"`, which only passed because
the inner formatter happened to repeat that phrase. The wrap prefix
"cannot validate resources" from diff_processor.go is the stable
semantic anchor; assertion updated.
Address Copilot review feedback as part of this redesign:
defaulting-only Invalid resources (currently unreachable per upstream
statusFromErrors, but defensible as a future-proofing measure) emit
their defaulting messages rather than producing an empty block.
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Comment on lines
163
to
165
| composed: []cpd.Unstructured{*composedResource1, *composedResource2}, | ||
| preloadedCRDs: []*extv1.CustomResourceDefinition{createCRDWithStringField(xrCRD)}, | ||
| expectedErr: true, |
Comment on lines
+411
to
+419
| func formatErrorLine(e pkgvalidate.FieldValidationError) string { | ||
| msg := e.Message | ||
| if rendered := renderBadValue(e.Value); rendered != "" && !strings.Contains(msg, rendered) { | ||
| msg = fmt.Sprintf("%s (got %s)", msg, rendered) | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s [%s]", msg, e.Type) | ||
| } | ||
|
|
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
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.
Description of your changes
Adopts the structured-result validation API that landed in crossplane/cli#66, replacing the previous parse-stdout pattern in
cmd/diff/diffprocessor/schema_validator.go. This is the Part 3 follow-up to the structured-validation work whose Part 1 introducedextractValidationErrorsas an interim line-parsing solution; with upstream now exposing typed results, that interim layer goes away.What changes:
FieldValidationErrorrecords viapkg/validate.SchemaValidateinstead of[x]/[!]prefix-line parsing. TheextractValidationErrorshelper and itsTestExtractValidationErrorsare gone, replaced byformatValidationErrors(covered byTestFormatValidationErrors).bytes.Buffer+io.MultiWriter+loggerwriterindirection used to capture rendered output is gone; per-resource debug logging is now explicit vialogResultDetails.pkg/validate.ResultError, which preserves the historical error-on-missing-schemas semantic.pkg/validate.SchemaValidatedeep-copies its inputs and does not mutate caller-owned resources, so the previous side-effect of in-place defaulting fromSchemaValidationis gone. Defaults are applied explicitly viapkg/xr.ApplyCRDDefaultsbefore validation, preserving the invariant that the diff calculator sees fully-defaulted resources.TestDefaultSchemaValidator_ValidateResources_AppliesDefaultsis the regression guard for this.render.DefaultValuescall site indiff_processor.gotoxr.ApplyCRDDefaults. Same signature, but the helper was promoted out ofcmd/crossplane/renderinto the newpkg/xrlibrary package in the same upstream cleanup wave.go.modpinscrossplane/clitov2.4.0-rc.0.0.20260615182009-ba59fbfac34b(the merge commit of crossplane/cli#66) and pullscrossplane-runtime/v2andcrossplane/apis/v2forward tov2.4.0-rc.0to satisfy the cli's transitive constraints. The cli's current latest tagv2.4.0-rc.0predates this merge by 78 commits, so a pseudo-version is the only way to consume the refactor today; bump to a real tagged release once crossplane/cli cuts one.Fixes #
I have:
earthly -P +reviewableto ensure this PR is ready for review.Added or updated e2e tests.— no behavior change at the e2e surface; the cleanup is internal to schema validation and produces equivalent error strings.Documented this change as needed.— no user-facing behavior change.Followed the API promotion workflow if this PR introduces, removes, or promotes an API.— no public API surface change.Need help with this checklist? See the cheat sheet.