feat(validate): structured results and --output json|yaml flag#66
Merged
Conversation
Split the validate command into a processing layer that returns a structured *ValidationResult and a rendering layer that writes it as text, JSON, or YAML. The processing API (SchemaValidate, ResultError, types) now lives at cmd/crossplane/pkg/validate/ so programmatic consumers — for example crossplane-diff — can call it directly without parsing free-form text output. The renderer at cmd/crossplane/pkg/validate/render/ is a sibling subpackage so consumers can depend on validation data without pulling in YAML/JSON encoding deps. The CLI gains a -o/--output text|json|yaml flag (default text). Default text output is byte-identical to the historical format. JSON and YAML emit the full ValidationResult including per-resource status and field-level error details (type, field path, message, bad value). The previous SchemaValidation(...) entry point is removed; callers move to SchemaValidate + RenderValidationResult + ResultError. errWriteOutput moves to cmd.go alongside its remaining caller in manager.go. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Code review surfaced three related issues with how the refactor handled applyDefaults errors. The new SchemaValidate `continue`d after a defaulting failure (skipping schema/CEL validation), counted DefaultingFailed as Invalid (causing a non-zero exit), and let renderText emit a `[!]` warning marker for resources that the summary line then counted as failure cases — output that read as self-contradictory and exit-code behavior that diverged from the deleted SchemaValidation entry point. Restore the historical "warning, then continue" semantics: - SchemaValidate no longer aborts the per-resource loop on a defaulting failure. It records a FieldErrorTypeDefaulting entry and falls through to schema, CEL, and unknown-field checks so every problem surfaces at once. - Per-resource Status is decided after validation by statusFromErrors: any real schema/CEL/unknown error means Invalid, a defaulting-only failure means DefaultingFailed, no errors means Valid. - computeSummary counts DefaultingFailed toward Valid, mirroring the pre-refactor accounting where defaulting errors did not increment the failure counter. ResultError no longer escalates a defaulting-only failure to a CLI error. - renderText delegates to a new writeErrorLine helper that picks the prefix per error type ([!] for defaulting, [x] for schema/CEL/unknown). A resource with both a defaulting error and a schema error now produces both lines, matching the historical output. New tests: - TestSchemaValidate/DefaultingFailureWithSchemaError — regression guard that schema validation still runs (and surfaces errors) when defaulting fails. - TestRenderValidationResult_TextDefaulting — exercises the per-error prefix selection for a fixture with one DefaultingFailed resource and one Invalid resource carrying both error types. DefaultingFailureOnly (renamed from DefaultingFailure) now asserts the resource is summarized as Valid, locking in the historical behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Reworks the validate package to address eight comments from the PR review pass. Renderer: polymorphic dispatch * render.Renderer is now an interface; textRenderer, jsonRenderer, and yamlRenderer each implement it as their own concrete type. Adding a new format means adding a Renderer and registering it in the renderers map — no switch on string. (review crossplane#3, crossplane#4) * OutputFormat.Render dispatches via the registry; the empty string defaults to text for safety of zero-value callers. * renderText handles all per-resource line emission inline; the per-error prefix selection is encapsulated in a private helper that does not pretend to handle the [✓] or "[!] could not find" cases. SchemaValidate: decomposed * Extracted validateResource as a per-resource helper. SchemaValidate's body is now a clean fan-out and the //nolint:gocognit suppression is gone. (review crossplane#2) Tests: tightened, idiomatic, end-to-end * TestSchemaValidate now asserts on the full []FieldValidationError per resource (Type + Field), with cmpopts.IgnoreFields hiding the Message and Value strings that drift across k8s validator versions. Dropped the bespoke containsAllErrorTypes "at least once" matcher. (review crossplane#7) * All tests that take a context now use t.Context() instead of context.Background(). (review crossplane#6) * render text tests no longer compare full output bytes against a string constant; they assert on line counts plus per-line substrings with a generated summary line. (review crossplane#5) * Dropped the validateAndRender helper on Cmd. The validation + rendering + error-shaping logic lives directly in Cmd.Run. cmd_test.go now drives the command end-to-end through kong.Parse + ctx.Run against real YAML fixtures, capturing stdout from a real kong.Context. To keep the e2e run offline, testdata/cache contains a stand-in crossplane image package.yaml and the tests pass a matching --crossplane-image. (review #1, crossplane#8) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts validation into an I/O-free pkg/validate API with structured ValidationResult types, implements pluggable renderers (text/json/yaml), wires renderer selection into the CLI, and adds unit + integration tests covering defaulting, unknown fields, rendering, and CLI behavior. ChangesValidation API refactoring and rendering
Sequence Diagram(s)sequenceDiagram
participant CLI
participant SchemaValidate
participant Renderer
participant ResultError
CLI->>SchemaValidate: call SchemaValidate(resources, crds)
SchemaValidate-->>CLI: return ValidationResult
CLI->>Renderer: Renderer.Render(ValidationResult, w, opts)
Renderer-->>CLI: writes formatted output
CLI->>ResultError: ResultError(ValidationResult, errorOnMissingSchemas)
ResultError-->>CLI: returns error or nil (exit code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Thank you — if you'd like this split into smaller review checkpoints or a focused security/edge-case checklist, I can do that. Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
cmd/crossplane/pkg/validate/types.go (1)
58-80: ⚡ Quick winType
FieldValidationError.TypeasFieldErrorType.Thanks for exposing this as a programmatic API. Right now
Typeis a plainstring, so producers and consumers can drift from the four supported values without the compiler helping. Could we make this field a dedicatedFieldErrorTypebefore the API settles?Possible tightening
+type FieldErrorType string + type FieldValidationError struct { - Type string `json:"type"` + Type FieldErrorType `json:"type"` Field string `json:"field,omitempty"` Message string `json:"message"` Value any `json:"value,omitempty"` } const ( - FieldErrorTypeSchema = "schema" - FieldErrorTypeCEL = "cel" - FieldErrorTypeUnknownField = "unknownField" - FieldErrorTypeDefaulting = "defaulting" + FieldErrorTypeSchema FieldErrorType = "schema" + FieldErrorTypeCEL FieldErrorType = "cel" + FieldErrorTypeUnknownField FieldErrorType = "unknownField" + FieldErrorTypeDefaulting FieldErrorType = "defaulting" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/pkg/validate/types.go` around lines 58 - 80, Change the Type field on FieldValidationError from a plain string to a dedicated FieldErrorType: introduce a new type declaration "type FieldErrorType string" (keeping the existing consts) and update FieldValidationError.Type's type to FieldErrorType (preserving the `json:"type"` tag); then update any code, constructors, or tests that assign raw strings to this field to use FieldErrorType("...") or the existing constants (FieldErrorTypeSchema, FieldErrorTypeCEL, FieldErrorTypeUnknownField, FieldErrorTypeDefaulting) so compilation stays correct.cmd/crossplane/pkg/validate/validate.go (1)
279-293: ⚡ Quick winUse
crossplane-runtime/pkg/errorsin the defaulting error path.Thanks for the new defaulting flow. These branches switch back to
fmt.Errorf, which makes this file inconsistent with the repo’s wrapping convention. Could we usecrossplane-runtime/pkg/errorshere as well?As per coding guidelines,
**/*.go: Use crossplane-runtime/pkg/errors for wrapping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/pkg/validate/validate.go` around lines 279 - 293, Replace fmt.Errorf usage in the defaulting error path with crossplane-runtime/pkg/errors helpers: for the nil schemaProps case (reference schemaProps and the surrounding logic) return errors.Errorf("no schema found for version %s in CRD %s", gvk.Version, matchingCRD.Name); for the conversion error in extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps wrap the underlying err with errors.Wrapf or errors.Wrap (e.g. errors.Wrapf(err, "failed to convert schema")); and for schema.NewStructural failures wrap the err similarly (e.g. errors.Wrapf(err, "failed to create structural schema")). Ensure you import "github.com/crossplane/crossplane-runtime/pkg/errors" and replace the three fmt.Errorf occurrences accordingly.cmd/crossplane/pkg/validate/render/render.go (2)
101-112: ⚡ Quick winConsider wrapping write errors for consistency.
Similar to the JSON renderer, the
yaml.Marshalerror is wrapped but thefmt.Fprintwrite error at line 110 is returned unwrapped. Wrapping would provide consistent error context.Suggested refinement
_, err = fmt.Fprint(w, string(out)) - return err + return errors.Wrap(err, "cannot write YAML output")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/pkg/validate/render/render.go` around lines 101 - 112, The write error from fmt.Fprint in yamlRenderer.Render is returned unwrapped; update the function to wrap that error like the marshal error for consistency by returning errors.Wrap(err, errCannotMarshalYAML) (or a small distinct constant/message such as errCannotWriteYAML) when fmt.Fprint fails. Locate the yamlRenderer.Render method and modify the fmt.Fprint error handling to wrap the error before returning, referencing yaml.Marshal and errCannotMarshalYAML for consistency or introduce errCannotWriteYAML to clarify the write failure.
88-99: ⚡ Quick winConsider wrapping write errors for consistency.
The marshal error is wrapped with context (
errors.Wrap(err, errCannotMarshalJSON)), but thefmt.Fprintlnwrite error at line 97 is returned unwrapped. While write failures are rare, wrapping them would provide consistent error handling throughout the function.Suggested refinement
_, err = fmt.Fprintln(w, string(out)) - return err + return errors.Wrap(err, "cannot write JSON output")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/pkg/validate/render/render.go` around lines 88 - 99, The write error returned from fmt.Fprintln in jsonRenderer.Render should be wrapped for consistency with the marshal error; update the code in jsonRenderer.Render (the fmt.Fprintln call) to capture the returned error and return errors.Wrap(err, "<contextual message>") (e.g., "error writing JSON output" or similar) so both json.MarshalIndent and the write path return wrapped errors with contextual messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/crossplane/pkg/validate/validate.go`:
- Around line 99-111: The code is running schema, unknown-field, and CEL
validations multiple times when there are duplicate CRDs for the same GVK; fix
by deduplicating validators per GVK or by skipping duplicate runs inside
validateResource: for example, in newValidatorsAndStructurals ensure you insert
only one validator/structural per GVK into validators[gvk], or inside
validateResource add a local seen map keyed by the GVK and only run validation
blocks that call validation.ValidateCustomResource, validateUnknownFields, and
cel.NewValidator(...).Validate once per unique GVK; update references to
validators[gvk], newValidatorsAndStructurals, validateResource,
validateUnknownFields, and cel.NewValidator/Validate accordingly so duplicate
CRDs no longer produce repeated errors.
- Around line 54-55: The validator is mutating caller-owned Unstructured objects
(applyDefaults and validateUnknownFields modify r.Object), so make a deep copy
of the incoming resource before any mutation to preserve caller inputs; e.g., in
validateResource (and the other validation path referenced around lines 92-108)
call r = r.DeepCopy() (or otherwise deep-copy r.UnstructuredContent()) at the
start so subsequent calls to SchemaValidate/validateUnknownFields/applyDefaults
operate on the copy rather than the original.
In `@cmd/crossplane/validate/cmd.go`:
- Around line 44-46: The constant errWriteOutput is declared but not used;
either replace the literal error string used when wrapping I/O failures ("cannot
render validation result") with the errWriteOutput constant (so update the error
wrap site in the validate command to use errWriteOutput) or remove the unused
errWriteOutput declaration entirely; locate the validate output-writing code
(the error wrap around rendering/printing the validation result) and apply one
of these two changes so there is no dead/duplicated error message.
---
Nitpick comments:
In `@cmd/crossplane/pkg/validate/render/render.go`:
- Around line 101-112: The write error from fmt.Fprint in yamlRenderer.Render is
returned unwrapped; update the function to wrap that error like the marshal
error for consistency by returning errors.Wrap(err, errCannotMarshalYAML) (or a
small distinct constant/message such as errCannotWriteYAML) when fmt.Fprint
fails. Locate the yamlRenderer.Render method and modify the fmt.Fprint error
handling to wrap the error before returning, referencing yaml.Marshal and
errCannotMarshalYAML for consistency or introduce errCannotWriteYAML to clarify
the write failure.
- Around line 88-99: The write error returned from fmt.Fprintln in
jsonRenderer.Render should be wrapped for consistency with the marshal error;
update the code in jsonRenderer.Render (the fmt.Fprintln call) to capture the
returned error and return errors.Wrap(err, "<contextual message>") (e.g., "error
writing JSON output" or similar) so both json.MarshalIndent and the write path
return wrapped errors with contextual messages.
In `@cmd/crossplane/pkg/validate/types.go`:
- Around line 58-80: Change the Type field on FieldValidationError from a plain
string to a dedicated FieldErrorType: introduce a new type declaration "type
FieldErrorType string" (keeping the existing consts) and update
FieldValidationError.Type's type to FieldErrorType (preserving the `json:"type"`
tag); then update any code, constructors, or tests that assign raw strings to
this field to use FieldErrorType("...") or the existing constants
(FieldErrorTypeSchema, FieldErrorTypeCEL, FieldErrorTypeUnknownField,
FieldErrorTypeDefaulting) so compilation stays correct.
In `@cmd/crossplane/pkg/validate/validate.go`:
- Around line 279-293: Replace fmt.Errorf usage in the defaulting error path
with crossplane-runtime/pkg/errors helpers: for the nil schemaProps case
(reference schemaProps and the surrounding logic) return errors.Errorf("no
schema found for version %s in CRD %s", gvk.Version, matchingCRD.Name); for the
conversion error in
extv1.Convert_v1_JSONSchemaProps_To_apiextensions_JSONSchemaProps wrap the
underlying err with errors.Wrapf or errors.Wrap (e.g. errors.Wrapf(err, "failed
to convert schema")); and for schema.NewStructural failures wrap the err
similarly (e.g. errors.Wrapf(err, "failed to create structural schema")). Ensure
you import "github.com/crossplane/crossplane-runtime/pkg/errors" and replace the
three fmt.Errorf occurrences accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a94757d0-1a55-4fc7-a79c-541e7ec49813
⛔ Files ignored due to path filters (5)
cmd/crossplane/validate/testdata/cache/xpkg.crossplane.io/crossplane/crossplane@v0.0.0-test/package.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/crd.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_invalid.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_missing.yamlis excluded by!**/testdata/**and included by**/*.yamlcmd/crossplane/validate/testdata/cmd/resources_valid.yamlis excluded by!**/testdata/**and included by**/*.yaml
📒 Files selected for processing (15)
cmd/crossplane/pkg/validate/apply_defaults_test.gocmd/crossplane/pkg/validate/doc.gocmd/crossplane/pkg/validate/render/doc.gocmd/crossplane/pkg/validate/render/render.gocmd/crossplane/pkg/validate/render/render_test.gocmd/crossplane/pkg/validate/types.gocmd/crossplane/pkg/validate/unknown_fields.gocmd/crossplane/pkg/validate/unknown_fields_test.gocmd/crossplane/pkg/validate/validate.gocmd/crossplane/pkg/validate/validate_test.gocmd/crossplane/validate/cmd.gocmd/crossplane/validate/help/validate.mdcmd/crossplane/validate/validate.gocmd/crossplane/validate/validate_integration_test.gocmd/crossplane/validate/validate_test.go
💤 Files with no reviewable changes (1)
- cmd/crossplane/validate/validate.go
Comment on lines
+44
to
+46
| // errWriteOutput is the error message wrapped around I/O failures when the | ||
| // validate command writes to its output writer. | ||
| const errWriteOutput = "cannot write output" |
Contributor
There was a problem hiding this comment.
Unused constant: errWriteOutput is defined but never referenced.
Thanks for the thoughtful organization here! I notice this constant is declared but the actual error wrapping on line 166 uses a different string ("cannot render validation result"). Could you clarify whether this constant was intended to be used there, or should it be removed as dead code?
🧹 Suggested fix: either use the constant or remove it
Option A — use the constant:
- if err := c.Output.Render(result, k.Stdout, render.Options{SkipSuccessResults: c.SkipSuccessResults}); err != nil {
- return errors.Wrap(err, "cannot render validation result")
- }
+ if err := c.Output.Render(result, k.Stdout, render.Options{SkipSuccessResults: c.SkipSuccessResults}); err != nil {
+ return errors.Wrap(err, errWriteOutput)
+ }Option B — remove the unused constant:
-// errWriteOutput is the error message wrapped around I/O failures when the
-// validate command writes to its output writer.
-const errWriteOutput = "cannot write output"
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // errWriteOutput is the error message wrapped around I/O failures when the | |
| // validate command writes to its output writer. | |
| const errWriteOutput = "cannot write output" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/crossplane/validate/cmd.go` around lines 44 - 46, The constant
errWriteOutput is declared but not used; either replace the literal error string
used when wrapping I/O failures ("cannot render validation result") with the
errWriteOutput constant (so update the error wrap site in the validate command
to use errWriteOutput) or remove the unused errWriteOutput declaration entirely;
locate the validate output-writing code (the error wrap around
rendering/printing the validation result) and apply one of these two changes so
there is no dead/duplicated error message.
Collaborator
Author
There was a problem hiding this comment.
Skipping this one — errWriteOutput is used in manager.go:315 (the "schemas does not exist, downloading: ..." Fprintln wrap), so removing it would break the build. The constant lives in cmd.go because the prior monolithic validate.go was where it was originally declared; cmd.go is now the only top-level non-test file in the package, hence the placement. Happy to move it to a dedicated errors.go if you'd prefer it not appear adjacent to a Cmd that doesn't use it.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "errWriteOutput" --type=go -C3Length of output: 1221
@jcogilvie, you're absolutely right — confirmed in manager.go:315:
return errors.Wrapf(err, errWriteOutput)My apologies for the false positive! I missed the cross-file usage within the same package. The placement in cmd.go as the package's sole non-test top-level file makes complete sense, and no change is needed here. Thanks for the clear explanation!
🧠 Learnings used
Learnt from: adamwg
Repo: crossplane/cli PR: 13
File: cmd/crossplane/xr/generate.go:100-100
Timestamp: 2026-05-20T21:52:31.054Z
Learning: In this repository’s Go code (including usages of `errors.New` and `errors.Wrap`), ensure the string argument (the human-readable error message) starts with a lowercase letter, following standard Go error-string conventions. For example, prefer `errors.Wrap(err, "cannot unmarshal claim")` over `errors.Wrap(err, "Unmarshalling Error")`. This helps keep error messages consistent and idiomatic across the codebase.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: cmd/crossplane/function/generate.go:242-244
Timestamp: 2026-05-19T19:10:34.938Z
Learning: In `cmd/crossplane/function/generate.go`, errors from filesystem discovery calls such as `filesystem.FindNestedFoldersWithPattern`, `afero.DirExists`, and `afero.Exists` on the schemas filesystem (c.schemasFS) are intentionally ignored (blank identifier `_`). This is by design to gracefully handle the common case where no schemas exist yet (e.g., the user has not added any project dependencies). In this scenario, the calls return empty results/errors but the function scaffold should still be generated without failing.
Learnt from: CR
Repo: crossplane/cli PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-05-29T21:44:10.201Z
Learning: Applies to **/*.go : Add short descriptions for commands and flags using kong struct tags. Return longer help text from each command's `Help()` method formatted as Markdown.
Learnt from: CR
Repo: crossplane/cli PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-05-29T21:44:10.201Z
Learning: Applies to **/*.go : Use kong as the CLI framework. Each command must be defined as a struct in which fields become subcommands, positional arguments, or flags. Use kong struct tags to control validation, auto-completion, and documentation.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: When reviewing Go code targeting Go 1.26+ (per go.mod and/or CI build settings), do not flag `new(expr)` as a compile error. In Go 1.26, the built-in `new` accepts an expression (not only a type): `new(expr)` allocates a variable of the type of `expr`, initializes it to the value of `expr`, and returns a pointer to it (`*T`). This means usages like `new(someFunc())` or `new(someStringConstant)` are valid syntax and should be allowed under Go 1.26 or later.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: internal/project/build.go:361-364
Timestamp: 2026-05-12T22:54:33.936Z
Learning: When reviewing Go code, allow the Go 1.26+ syntax `new(expr)` where `expr` is any expression (not just a type). In Go 1.26, `new(expr)` allocates a value of the type of `expr`, initializes it to the value of `expr`, and returns a `*T` pointer to it (e.g., `new(someFunc())`, `new(someStringConstant)`, `new(pkg.Const)`). If the repository targets Go 1.26 or later (e.g., module `go` directive >= 1.26 and/or build/CI uses Go >= 1.26), do not flag these usages as compilation errors.
Learnt from: adamwg
Repo: crossplane/cli PR: 10
File: cmd/crossplane/dependency/dependency.go:21-25
Timestamp: 2026-05-13T18:59:33.289Z
Learning: In crossplane/cli, experimental/beta CLI command gating is done via the kong command registration struct tag `maturity:"beta"` (or similar) placed on the *parent/top-level* command fields (e.g., top-level `Cmd` fields in `cmd/crossplane/main.go`). Subcommands registered under a parent are already considered gated by that parent maturity. During code review, do not request/introduce programmatic feature flags or add a wrapper function to conditionally register commands; treat the `maturity` tag as the correct and sufficient mechanism. Instead, focus review attention on whether the appropriate maturity tag is set on the relevant parent command.
Learnt from: adamwg
Repo: crossplane/cli PR: 13
File: cmd/crossplane/xr/generate.go:136-143
Timestamp: 2026-05-20T21:52:28.707Z
Learning: In the crossplane/cli Go CLI code, prefer concise user-facing error messages for invalid resource/claim input (e.g., missing required fields like “Claim has no apiVersion” or “Claim has no kind section”). Avoid verbose, instructional messages that primarily list suggested next steps. In code review, do not flag short-form error strings as insufficiently user-friendly if they clearly state the specific problem; reserve longer remediation guidance for cases where it is necessary to understand or correct the failure.
jcogilvie
force-pushed
the
validate-refactor
branch
3 times, most recently
from
11df1ed to
87ed65b
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/crossplane/pkg/validate/render/render.go (1)
156-163: ⚡ Quick winWrap write errors for consistency and user-friendly error messages.
The write errors at lines 157 and 162 are returned unwrapped, while
jsonRenderer(line 99) andyamlRenderer(line 112) wrap write errors witherrCannotWriteOutput. This inconsistency makes error handling harder to reason about, and unwrapped errors may surface raw OS errors without context (e.g., "write /dev/stdout: broken pipe" instead of "cannot write validation result: write /dev/stdout: broken pipe").The same issue exists in
writeTextErrorLineat line 183.🔧 Proposed fix
For lines 156-158:
if _, err := fmt.Fprint(w, line); err != nil { - return err + return errors.Wrap(err, errCannotWriteOutput) }For lines 160-162:
_, err := fmt.Fprintf(w, "Total %d resources: %d missing schemas, %d success cases, %d failure cases\n", result.Summary.Total, result.Summary.MissingSchemas, result.Summary.Valid, result.Summary.Invalid) -return err +return errors.Wrap(err, errCannotWriteOutput)Apply the same pattern to
writeTextErrorLineline 183:_, err := fmt.Fprint(w, line) -return err +return errors.Wrap(err, errCannotWriteOutput)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/pkg/validate/render/render.go` around lines 156 - 163, In writeTextResult (the block that calls fmt.Fprint/Fprintf to write each line and the summary) and in writeTextErrorLine (the single-line writer), wrap any write errors with the existing sentinel errCannotWriteOutput before returning, matching the jsonRenderer/yamlRenderer pattern: replace direct returns of the raw err from fmt.Fprint/Fprintf with returning fmt.Errorf("%w: %v", errCannotWriteOutput, err) (or using errors.Wrap if available) so callers get the consistent "cannot write validation result" context; locate the write sites by looking for fmt.Fprint/ fmt.Fprintf uses in writeTextResult and the write in writeTextErrorLine and change their error returns accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/crossplane/pkg/validate/render/render.go`:
- Around line 156-163: In writeTextResult (the block that calls
fmt.Fprint/Fprintf to write each line and the summary) and in writeTextErrorLine
(the single-line writer), wrap any write errors with the existing sentinel
errCannotWriteOutput before returning, matching the jsonRenderer/yamlRenderer
pattern: replace direct returns of the raw err from fmt.Fprint/Fprintf with
returning fmt.Errorf("%w: %v", errCannotWriteOutput, err) (or using errors.Wrap
if available) so callers get the consistent "cannot write validation result"
context; locate the write sites by looking for fmt.Fprint/ fmt.Fprintf uses in
writeTextResult and the write in writeTextErrorLine and change their error
returns accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 575cea1b-84b7-426b-bc1e-9cfcc323f4a4
📒 Files selected for processing (8)
cmd/crossplane/pkg/validate/render/render.gocmd/crossplane/pkg/validate/render/render_test.gocmd/crossplane/pkg/validate/types.gocmd/crossplane/pkg/validate/unknown_fields.gocmd/crossplane/pkg/validate/validate.gocmd/crossplane/validate/cmd.gocmd/crossplane/validate/validate_integration_test.gocmd/crossplane/validate/validate_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/crossplane/pkg/validate/unknown_fields.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/crossplane/pkg/validate/types.go
- cmd/crossplane/validate/cmd.go
- cmd/crossplane/pkg/validate/render/render_test.go
- cmd/crossplane/validate/validate_integration_test.go
- cmd/crossplane/validate/validate_test.go
- cmd/crossplane/pkg/validate/validate.go
jcogilvie
force-pushed
the
validate-refactor
branch
2 times, most recently
from
ff913ed to
cbfeb73
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/crossplane/validate/validate_integration_test.go (1)
76-86: ⚡ Quick winTest naming + missing
reasonfields drift from the repo's test conventions.Thanks for the thorough table — the closure-based
assertText/JSON/YAMLsplit reads really nicely! One small thing to align with house style: the guidelines call for PascalCase test names without underscores and areasonfield per case. HereTestParse_RejectsUnknownOutputFormatand the case keys (DefaultText_Valid,Text_InvalidExitsNonZero, etc.) use underscores, and the cases don't carry areason.Would you be open to dropping the underscores (e.g.
TestParseRejectsUnknownOutputFormat) and adding a shortreasonto each case? Happy to defer if you feel the underscore-separated case keys read better for this end-to-end suite — just flagging the convention.As per coding guidelines: "Enforce table-driven test structure: PascalCase test names (no underscores)... Check for proper test case naming and reason fields."
Also applies to: 92-92, 107-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/crossplane/validate/validate_integration_test.go` around lines 76 - 86, Rename the test function TestParse_RejectsUnknownOutputFormat to PascalCase without underscores (e.g., TestParseRejectsUnknownOutputFormat), and update all table-driven test case keys (e.g., DefaultText_Valid, Text_InvalidExitsNonZero, etc.) to PascalCase without underscores; for each case in the tables used by the parse-related tests (the ones exercising parseCmd and the assertText/assertJSON/assertYAML closures) add a short reason string field to the case struct and populate it with a one-line description, then update the t.Run calls to include case.reason in the subtest name or use it in the test output so each case carries the required reason; apply the same renaming and reason-field addition consistently to the other parse tests referenced in the comment (the subsequent table-driven tests in the same file).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/crossplane/validate/validate_integration_test.go`:
- Around line 76-86: Rename the test function
TestParse_RejectsUnknownOutputFormat to PascalCase without underscores (e.g.,
TestParseRejectsUnknownOutputFormat), and update all table-driven test case keys
(e.g., DefaultText_Valid, Text_InvalidExitsNonZero, etc.) to PascalCase without
underscores; for each case in the tables used by the parse-related tests (the
ones exercising parseCmd and the assertText/assertJSON/assertYAML closures) add
a short reason string field to the case struct and populate it with a one-line
description, then update the t.Run calls to include case.reason in the subtest
name or use it in the test output so each case carries the required reason;
apply the same renaming and reason-field addition consistently to the other
parse tests referenced in the comment (the subsequent table-driven tests in the
same file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b60d11e-e2d4-469b-8b0d-27d1c01b0e38
📒 Files selected for processing (8)
cmd/crossplane/pkg/validate/render/render.gocmd/crossplane/pkg/validate/render/render_test.gocmd/crossplane/pkg/validate/types.gocmd/crossplane/pkg/validate/unknown_fields.gocmd/crossplane/pkg/validate/validate.gocmd/crossplane/validate/cmd.gocmd/crossplane/validate/validate_integration_test.gocmd/crossplane/validate/validate_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- cmd/crossplane/pkg/validate/unknown_fields.go
- cmd/crossplane/validate/cmd.go
- cmd/crossplane/pkg/validate/render/render.go
- cmd/crossplane/validate/validate_test.go
- cmd/crossplane/pkg/validate/render/render_test.go
- cmd/crossplane/pkg/validate/types.go
- cmd/crossplane/pkg/validate/validate.go
jcogilvie
force-pushed
the
validate-refactor
branch
from
cbfeb73 to
0fd8ed2
Compare
tampakrap
reviewed
Jun 4, 2026
jcogilvie
force-pushed
the
validate-refactor
branch
from
0fd8ed2 to
2b3f641
Compare
tampakrap
reviewed
Jun 8, 2026
tampakrap
left a comment
tampakrap
left a comment
Collaborator
There was a problem hiding this comment.
I left a couple of comments, apart from that I don't see any other issue. I plan also to give it a spin today to make sure it works fine
Reviewer pointed out that even with the polymorphic registry, the
OutputFormat string was still flowing through the command — we were
dispatching at every Render call site instead of resolving once at the
boundary. Subsequent passes worked through the implications: the
renderer types are stateless empty structs, so the map cache was
needless ceremony; the dispatch can collapse to a single fallible
boundary that takes a string-shaped format identifier and returns a
typed Renderer; and the natural place for that boundary is Kong itself,
which has first-class support for typed flag decoding.
Final shape:
* render: a single typed boundary.
* OutputFormat is a defined string type with three named constants
(OutputFormatText, OutputFormatJSON, OutputFormatYAML) so call
sites use symbolic names rather than embedded "json"/"yaml"
string literals.
* RendererFor(OutputFormat) (Renderer, error) is the only public
factory. Empty maps to text for ergonomics with zero-valued
config; any other unrecognised value returns an error.
* No format identifier or wrapper type is otherwise exposed;
downstream code receives only the Renderer interface and works
against the typed dependency.
* validate.Cmd: gained a private rendererFlag wrapper that implements
Kong's MapperValue interface, decoding --output straight into a
typed render.Renderer at parse time. Cmd.Output is now of type
rendererFlag (which embeds render.Renderer), so Cmd.Run calls
c.Output.Render(...) directly. AfterApply only sets the filesystem;
the renderer is already resolved by the time it runs. The wrapper
lives in this package, not in render, so the render package stays
free of any kong dependency and remains importable from non-CLI
consumers like crossplane-diff.
* Kong's enum:"" tag doesn't apply to MapperValue-backed fields, so
rendererFlag.Decode performs the validation itself; --help text
enumerates the valid values for users.
* render tests: switched to OutputFormat constants throughout. The
format-boundary test (TestRendererFor_FormatBoundary) covers the
named constants, the empty-string-as-text ergonomics, and unknown
rejection via OutputFormat("xml").
Smoke: `crossplane resource validate ... --output=json` emits valid
JSON; `--output=xml` is rejected by Kong with the wrapped Decode error
"--output: unknown output format: \"xml\"".
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
force-pushed
the
validate-refactor
branch
from
2b3f641 to
d192b93
Compare
tampakrap
approved these changes
Jun 14, 2026
tampakrap
left a comment
tampakrap
left a comment
Collaborator
There was a problem hiding this comment.
LGTM thank you! I tested it locally, passing all the three options of the new --output flag and got the expected results. One nitpick would be not to use render for the package name to avoid confusion with the popular subcommand, but that's minor so I'll approve and let you decide if you want to keep that name or change it
tampakrap
reviewed
Jun 14, 2026
tampakrap
left a comment
tampakrap
left a comment
Collaborator
There was a problem hiding this comment.
one more comment: how about moving cmd/crossplane/pkg/validate to topdir pkg/validate?
jcogilvie
added a commit
to jcogilvie/cli
that referenced
this pull request
Jun 15, 2026
Per review feedback on crossplane#66: - Move pkg/validate from cmd/crossplane/pkg/validate to top-level pkg/validate so the programmatic API is discoverable as a public package. - Rename the render subpackage to output to avoid confusion with the unrelated `crossplane render` command. - Rename the OutputFormat type to Format (and OutputFormat* constants to Format*) since it now lives inside the output package. No behavioural changes.
jcogilvie
added a commit
to jcogilvie/cli
that referenced
this pull request
Jun 15, 2026
Per review feedback on crossplane#66: - Move pkg/validate from cmd/crossplane/pkg/validate to top-level pkg/validate so the programmatic API is discoverable as a public package. - Rename the render subpackage to output to avoid confusion with the unrelated `crossplane render` command. - Rename the OutputFormat type to Format (and OutputFormat* constants to Format*) since it now lives inside the output package. No behavioural changes.
jcogilvie
force-pushed
the
validate-refactor
branch
from
c0e9a37 to
275bda9
Compare
Collaborator
Author
|
@tampakrap done in 275bda9 — moved |
Per review feedback on crossplane#66: - Move pkg/validate from cmd/crossplane/pkg/validate to top-level pkg/validate so the programmatic API is discoverable as a public package. - Rename the render subpackage to output to avoid confusion with the unrelated `crossplane render` command. - Rename the OutputFormat type to Format (and OutputFormat* constants to Format*) since it now lives inside the output package. No behavioural changes. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
force-pushed
the
validate-refactor
branch
from
275bda9 to
e7ff988
Compare
6 tasks
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 16, 2026
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>
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 17, 2026
* refactor(validate): consume cli's structured validation API 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> * fix: emit defaulting-only errors instead of dropping them 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> * refactor(validate): redesign error format around structured per-resource 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> * feat(validate): expose typed validation failures in JSON / YAML output Schema validation now surfaces structured per-resource failure detail through OutputError.ValidationFailures so machine consumers of crossplane-diff's JSON / YAML output don't have to parse the human-readable Message string. The string is still emitted alongside, unchanged, for operators reading the JSON directly. Wire shape: { "errors": [{ "resourceID": "XNopResource/my-xr", "message": "...", "validationFailures": [ { "apiVersion": "ns.example.org/v1alpha1", "kind": "XNopResource", "name": "my-xr", "namespace": "default", "status": "invalid", "errors": [ { "type": "schema", "field": "spec.coolField", "message": "spec.coolField: Invalid value: \"number\": ...", "value": "number" } ] } ] }] } ResourceID and ValidationFailures play complementary roles: ResourceID identifies which user input the diff was processing (one entry per batched run), while ValidationFailures lists every resource inside that input's render tree that failed validation, with full GVK, namespace, and typed errors. They overlap on Kind+Name when the input itself is among the failing resources — that's intentional: consumers iterating ValidationFailures should never miss an XR-level rejection. Wire types live in renderer/types/types.go (ResourceValidationFailure, FieldValidationError) so crossplane-diff owns its public schema rather than re-exporting crossplane/cli's pkg/validate types — the upstream shape can evolve independently of ours. SchemaValidationError gains an optional Result field carrying the *pkgvalidate.ValidationResult that produced it; ValidateResources attaches it. NewOutputError handles the type assertion + conversion in one place, used at both XR diff and comp diff error-construction sites. Non-validation paths (scope check, tool errors, IO errors) leave ValidationFailures nil. The integration test infra splits stdout/stderr into separate buffers (previously combined via kong.Writers(&stdout, &stdout), which broke JSON parsing as soon as the renderer wrote a stderr ERROR line) and allows expectedStructuredOutput to coexist with expectedError when no expectedErrorContains is set. ExpectedDiff gains a fluent WithError / WithValidationFailure / WithFieldError builder chain so tests assert on validationFailures structurally. The SchemaValidationError integration test switches from substring matching the human-readable error string to JSON-mode + structured assertion: it now pins the failing GVKs, namespaces, statuses, error types, and field paths for both the failing XR and its composed resource. The k8s-emitted message wording in between is intentionally not asserted so apimachinery upgrades can shift the phrasing without breaking the test. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * test(validate): adopt project's reason-field convention; address review feedback Three changes, all in response to review feedback on PR #347: - Adopt the established reason-field test convention (used in diff_integration_test and the client/crossplane test suites) for the new TestFormatValidationErrors, TestNewOutputError, and TestValidationFailuresFromResult cases. Inline `// comment` blocks move into a `reason string` table-struct field that surfaces in failure messages, so a failing case prints both what went wrong and why the case exists. - Match the bad-value duplication check to k8s emit conventions: string values get %q (quoted) for both display and the Contains-search, so an unquoted incidental substring (e.g. Value="k" against message "spec.kind: Required value") no longer suppresses the "(got <value>)" tail. Numbers / bools / structs keep %v unchanged. New regression cases: BadValueNotSuppressedByIncidentalSubstring and NumericBadValueRendersUnquoted. - Drop the misleading preloadedCRDs entry in the TestDefaultSchemaValidator_ValidateResources ValidationError case. The field is dead state — `tt.preloadedCRDs` is never read in any test body in this file; CRDs flow through the mock SchemaClient set up by setupClients. Removing the wider unused field is out-of-scope for this PR (touches many unrelated tests), so just empty this entry to stop implying it influences validation behavior. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * test(validate): drop dead preloadedCRDs state and orphaned helpers The preloadedCRDs field was historical residue: a previous refactor moved CRD plumbing through a mock SchemaClient (set up by each test case's setupClients / setupClient closure) but left the original preloadedCRDs slice on three test struct types. The field was never read; every assignment went straight to the GC. Comments like "// No longer needed" at several call sites confirmed the previous author knew the data was unused but stopped short of removing the field. Sweep the field from all three table-driven test functions in cmd/diff/diffprocessor/schema_validator_test.go: - TestDefaultSchemaValidator_ValidateResources (4 cases) - TestDefaultSchemaValidator_LoadCRDs (1 case) - TestDefaultSchemaValidator_ValidateScopeConstraints (8 cases) Removing the field exposed two more pieces of dead state in the same file: - The ValidationError closure in TestDefaultSchemaValidator_ValidateResources had a composedCRDUn block that built an unstructured via runtime.DefaultUnstructuredConverter and discarded the result. Drop the block; with no remaining caller, MustToUnstructured and the runtime import go with it. - The const testComposedResource = "ComposedResource" was defined but never referenced — every call site uses the literal string "testComposedResource" (matching the const's name, not its value). Drop the const. No behavior change; all unit tests still pass under earthly -P +reviewable. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(validate): address final round of review feedback Three issues caught by Copilot on the rebased branch: 1. SchemaValidate setup-error path returned a generic errors.Wrap rather than a SchemaValidationError, so IsSchemaValidationError / DetermineExitCode would classify it as ExitCodeToolError (1) instead of ExitCodeSchemaValidation (2). Pre-refactor, the old validate.SchemaValidation entry point produced a SchemaValidationError on every error path; the structured-API split inadvertently broke that contract. Restore it: setup errors are wrapped via NewSchemaValidationError(...) with no Result attached (the validator never produced one). 2. resourceHeader produced "<gvk> <ns>/" with a stray trailing slash when a resource had metadata.namespace set but no metadata.name. The empty-name guard checked the *computed* name string ("ns/") rather than r.Name. Gate on r.Name directly so an empty name collapses to GVK-only regardless of namespace, matching the intent already documented for the empty-namespace case. Regression case: NamespaceWithoutNameCollapsesToGVK. 3. Non-string Value duplicate check used a raw strings.Contains, so Value=42 against message containing "420" would suppress the "(got 42)" tail incorrectly — same class of false-positive Copilot caught earlier for unquoted strings, just on the numeric path. Split the duplicate check out as valueAlreadyInMessage: strings still use the quoted-form substring search (delimiter-safe via the surrounding %q quotes), non-strings use a word-boundary regex so 42 doesn't match inside 420. Two new regression cases: NumericBadValueNotSuppressedByDigitPrefix (Value=42 vs "420" — tail correctly rendered) and NumericBadValueSuppressedAsToken (Value=42 vs "Invalid value: 42:" — tail correctly suppressed because 42 appears as a standalone token). Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> --------- Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
5 tasks
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
2 participants
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
Adds machine-readable output to
crossplane resource validateand exposes the validation logic as a programmatic Go API.The user-facing change is a
-o/--output text|json|yamlflag. The default text output is byte-identical to the historical format (same[✓]/[x]/[!]markers, same line order, same summary line); JSON and YAML emit the full structured result, including per-resource status and field-level error details — type, field path, message, and bad value.The structural changes that enable it:
cmd/crossplane/pkg/validate/—SchemaValidate(ctx, resources, crds) (*ValidationResult, error)returns structured data with no I/O. Programmatic consumers (the motivating example: crossplane-diff) call it directly and inspect the result, replacing the previous parse-stdout pattern.cmd/crossplane/pkg/validate/render/—RendererFor(OutputFormat) (Renderer, error)returns the format-specific renderer. Lives in a sibling subpackage so consumers depending only on validation data don't pull in YAML/JSON encoding deps.--outputflag is decoded directly into a typedrender.Renderervia Kong'sMapperValueinterface, soCmd.Runworks against the dependency rather than threading a format string through validation logic. Exit codes and--skip-success-resultssemantics are unchanged.Breaking change for direct Go callers: the previous
validate.SchemaValidation(ctx, resources, crds, errorOnMissingSchemas, skipSuccessLogs, w)entry point is removed. Callers migrate topkgvalidate.SchemaValidate+ a renderer fromrender.RendererFor+pkgvalidate.ResultError. The CLI surface (text bytes, exit codes, flag semantics) is preserved.Sample JSON output:
{ "summary": { "total": 2, "valid": 1, "invalid": 1, "missingSchemas": 0 }, "resources": [ { "apiVersion": "smoke.org/v1alpha1", "kind": "Test", "name": "ok-instance", "status": "valid" }, { "apiVersion": "smoke.org/v1alpha1", "kind": "Test", "name": "bad-instance", "status": "invalid", "errors": [ { "type": "schema", "field": "spec.replicas", "message": "spec.replicas: Invalid value: \"string\": spec.replicas in body must be of type integer: \"string\"", "value": "string" } ] } ] }Tests
pkg/validate/validate_test.go:TestSchemaValidate— 9 table-driven cases covering Valid, InvalidSchema, InvalidCEL, MissingSchema, UnknownField, DefaultingFailureOnly, DefaultingFailureWithSchemaError, Empty, and MixedOrder.TestResultError— 5 cases covering Clean, InvalidPresent, MissingIgnored, MissingWithFlag, InvalidAndMissing.pkg/validate/{apply_defaults,unknown_fields}_test.go:TestApplyDefaultsandTestValidateUnknownFieldsmoved here alongside the helpers they test.pkg/validate/render/render_test.go:TestRendererFor_Text(per-line substring assertions, not full-string golden),TestRendererFor_JSON,TestRendererFor_YAML(round-trip),TestRendererFor_FormatBoundary(text/json/yaml/empty/unknown).validate/validate_integration_test.go: a Kong-driven end-to-end test (TestRun) that runscrossplane resource validateagainst fixture YAML files. The pre-populatedtestdata/cache/directory keeps the run offline. 8 subtests cover text/json/yaml × valid/invalid/missing ×--skip-success-resultsand--error-on-missing-schemas.TestParse_RejectsUnknownOutputFormatcovers parse-time rejection of unknown formats byrendererFlag.Decode.TestValidateResourcesis removed — it tested the now-deletedSchemaValidationwrapper, andTestSchemaValidatecovers the same logic with stronger structural assertions.TestConvertToCRDsand the manager / cache / image tests pass unchanged.Reviewer attention
[!](warning) and counts as a success in the summary; a defaulting failure combined with a schema error produces both lines and counts as a failure.DefaultingFailureWithSchemaErroris the regression guard that schema validation still runs after a defaulting failure.validate_integration_test.goships a stand-incrossplane@v0.0.0-test/package.yamlundertestdata/cache/so the test runs offline through the realManager/LocalCachecode paths. Alternative would have been adding aWithFetcheroption toManagerfor test injection — chose the cache fixture as the smaller change that exercises more production code.cmd/crossplane/validate/help/validate.md) was updated with a--output json | jqexample.Fixes #62
I have:
./nix.sh flake checkto ensure this PR is ready for review.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.