feat(validate): structured results and --output json|yaml flag by jcogilvie · Pull Request #1 · jcogilvie/cli

jcogilvie · 2026-05-31T19:36:03Z

Description of your changes

Adds machine-readable output to crossplane resource validate and exposes the validation logic as a programmatic Go API.

The user-facing change is a -o/--output text|json|yaml flag. 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:

Processing API at 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.
Rendering API at 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.
CLI wiring — the --output flag is decoded directly into a typed render.Renderer via Kong's MapperValue interface, so Cmd.Run works against the dependency rather than threading a format string through validation logic. Exit codes and --skip-success-results semantics are unchanged.

Breaking change for direct Go callers: the previous validate.SchemaValidation(ctx, resources, crds, errorOnMissingSchemas, skipSuccessLogs, w) entry point is removed. Callers migrate to pkgvalidate.SchemaValidate + a renderer from render.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: TestApplyDefaults and TestValidateUnknownFields moved 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 runs crossplane resource validate against fixture YAML files. The pre-populated testdata/cache/ directory keeps the run offline. 8 subtests cover text/json/yaml × valid/invalid/missing × --skip-success-results and --error-on-missing-schemas. TestParse_RejectsUnknownOutputFormat covers parse-time rejection of unknown formats by rendererFlag.Decode.
The previously existing TestValidateResources is removed — it tested the now-deleted SchemaValidation wrapper, and TestSchemaValidate covers the same logic with stronger structural assertions. TestConvertToCRDs and the manager / cache / image tests pass unchanged.

Reviewer attention

Defaulting-failure semantics intentionally match the historical CLI behavior. A defaulting-only failure renders as [!] (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. DefaultingFailureWithSchemaError is the regression guard that schema validation still runs after a defaulting failure.
validate_integration_test.go ships a stand-in crossplane@v0.0.0-test/package.yaml under testdata/cache/ so the test runs offline through the real Manager/LocalCache code paths. Alternative would have been adding a WithFetcher option to Manager for test injection — chose the cache fixture as the smaller change that exercises more production code.
Help text (cmd/crossplane/validate/help/validate.md) was updated with a --output json | jq example.

Fixes crossplane#62

I have:

Read and followed Crossplane's contribution process.
Run ./nix.sh flake check to ensure this PR is ready for review.
Added or updated unit tests.
Linked a PR or a docs tracking issue to document this change. — crossplane/docs#1101
~~Added backport release-x.y labels to auto-backport this PR.~~

Need help with this checklist? See the cheat sheet.

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>

jcogilvie · 2026-05-31T20:15:41Z

Code review

Found 3 issues, all stemming from the new handling of applyDefaults errors. The PR description claims default text output is byte-identical, but defaulting failures are now treated as hard failures rather than warnings. None of these are flagged as breaking.

Defaulting failure now continues and skips schema/CEL validation for the resource. The deleted SchemaValidation printed a [!] warning and fell through to schema/CEL checks, so a resource with both a defaulting issue and real schema errors used to surface every problem; under this change only the defaulting line is emitted.

cli/cmd/crossplane/pkg/validate/validate.go

Lines 72 to 83 in 15da6fb

    
           if err := applyDefaults(r, gvk, crds); err != nil { 
        
           	rvr.Status = ValidationStatusDefaultingFailed 
        
           	rvr.Errors = append(rvr.Errors, FieldValidationError{ 
        
           		Type:    FieldErrorTypeDefaulting, 
        
           		Message: err.Error(), 
        
           	}) 
        
           	result.Resources = append(result.Resources, rvr) 
        
           	continue 
        
           } 
        
           for _, v := range sv {

Defaulting failure now causes a non-zero exit. computeSummary maps ValidationStatusDefaultingFailed to Invalid++, and ResultError returns an error whenever Invalid > 0. The old code only incremented its failure counter when rf > 0 (real schema/CEL errors), so defaulting-only failures previously exited 0.

cli/cmd/crossplane/pkg/validate/validate.go

Lines 193 to 202 in 15da6fb

    
           for _, r := range results { 
        
           	switch r.Status { 
        
           	case ValidationStatusValid: 
        
           		s.Valid++ 
        
           	case ValidationStatusInvalid, ValidationStatusDefaultingFailed: 
        
           		s.Invalid++ 
        
           	case ValidationStatusMissingSchema: 
        
           		s.MissingSchemas++ 
        
           	} 
        
           }

Text output is internally inconsistent for DefaultingFailed. renderText emits the [!] warning marker (same as MissingSchema), but the trailing Total ... failure cases count includes the resource because computeSummary lumped it into Invalid. A user sees zero [x] lines yet a non-zero failure count.

cli/cmd/crossplane/pkg/validate/render/render.go

Lines 101 to 110 in 15da6fb

    
           	} 
        
           case pkgvalidate.ValidationStatusDefaultingFailed: 
        
           	for _, e := range r.Errors { 
        
           		if e.Type == pkgvalidate.FieldErrorTypeDefaulting { 
        
           			if _, err := fmt.Fprintf(w, "[!] failed to apply defaults for %s, %s: %s\n", gvk, r.Name, e.Message); err != nil { 
        
           				return err 
        
           			} 
        
           		} 
        
           	} 
        
           case pkgvalidate.ValidationStatusInvalid:

Suggestion: either preserve the original "warning, then continue" semantics (don't continue, don't count toward Invalid), or escalate the prefix to [x] and call out the breaking change explicitly in the PR description and CHANGELOG.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

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>

jcogilvie · 2026-06-02T15:01:28Z

+// and any resource had no matching schema). It is the core of Cmd.Run,
+// extracted so that tests can exercise the flag-driven behaviour without the
+// file/cache loading machinery.
+func (c *Cmd) validateAndRender(ctx context.Context, resources []*unstructured.Unstructured, crds []*extv1.CustomResourceDefinition, w io.Writer) error {


Does this need to be extracted or can we exercise it via Kong?

Consider how crossplane-diff interfaces with Kong in test: https://github.com/crossplane-contrib/crossplane-diff/blob/main/cmd/diff/diff_test.go

jcogilvie · 2026-06-02T15:02:37Z

+// returned error is non-nil only for setup failures (for example, a CRD that
+// cannot be converted or compiled); per-resource validation failures are
+// reported via ResourceValidationResult.Status and .Errors, not via the error.
+func SchemaValidate(ctx context.Context, resources []*unstructured.Unstructured, crds []*extv1.CustomResourceDefinition) (*ValidationResult, error) { //nolint:gocognit // validation has many branches


Now might be a good time to consider reducing the cyclomatic complexity here. Yes, it's complicated; that's exactly what the linter warns about. I don't think "inherently complex" is a great reason to suppress a cognit warning. It is, however, a good reason to decompose the logic.

jcogilvie · 2026-06-02T15:07:19Z

+)
+
+// OutputFormat specifies how validation results should be rendered.
+type OutputFormat string


If we're already declaring a type for OutputFormat, why not add a function on the type that will render the output in the given format, rather than having discrete methods for renderText and renderJSON and renderYaml? Why not OutputFormat.render() on each of the three format objects? Use object orientation.

jcogilvie · 2026-06-02T15:12:36Z

+// renderText writes the result in the human-readable format that the
+// validate CLI has historically produced, preserving line order and
+// prefixes ([!], [x], [✓]).
+func renderText(result *pkgvalidate.ValidationResult, w io.Writer, opts RenderOptions) error {


Can we coalesce this with writeErrorLine? it seems weird that we have a switch statement feeding another switch statement over the same type. What's the rationale for it? What's the separation of concerns? (It doesn't need to be one method, but it seems like a good time to reassess the distribution of responsibilities.)

It's especially weird to me that writeErrorLine seems like an incomplete switch statement for printing, missing only the [!] and [✓] cases.

One additional consideration I suppose is whether we have just ported this code into a new file unchanged, in which case any suggested refactor would be a separate commit.

jcogilvie · 2026-06-02T15:15:16Z

+	}
+}
+
+const expectedTextWithSuccess = `[✓] test.org/v1alpha1, Kind=Test, ok validated successfully


Is there historical precedent in this repo for full-text output matching? This seems like it might be fragile.

jcogilvie · 2026-06-02T15:23:43Z

+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			result, err := SchemaValidate(context.Background(), tc.args.resources, tc.args.crds)


Use t.Context for all tests that take a context.

jcogilvie · 2026-06-02T15:25:59Z

+
+// containsAllErrorTypes returns true when every wanted type appears at least
+// once in the given FieldValidationError slice.
+func containsAllErrorTypes(errs []FieldValidationError, wantTypes []string) bool {


This seems... a little bespoke. Why only test for "at least once" presence? Don't we have a complete understanding of which errors have which counts and which resources they are associated with? Why dilute that understanding?

jcogilvie · 2026-06-02T15:31:03Z

+limitations under the License.
+*/
+
+package validate


This test file should probably test end to end functionality of validate by passing commands through kong. We should take care not to try to test kong itself, since that's wasted effort.

I would expect to see an integration test flow like:

input XRD and XR files on CLI

invoke validate

assert using machine output

Whether that lives in this file, replaces this file, or lives in a new place is up for discussion.

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>

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>

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 commented Jun 2, 2026

View reviewed changes

jcogilvie force-pushed the validate-refactor branch 15 times, most recently from 0fd8ed2 to 2b3f641 Compare June 4, 2026 19:19

jcogilvie force-pushed the validate-refactor branch from 2b3f641 to d192b93 Compare June 8, 2026 14:25

jcogilvie force-pushed the validate-refactor branch from c0e9a37 to 275bda9 Compare June 15, 2026 17:42

jcogilvie force-pushed the validate-refactor branch from 275bda9 to e7ff988 Compare June 15, 2026 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(validate): structured results and --output json|yaml flag#1

feat(validate): structured results and --output json|yaml flag#1
jcogilvie wants to merge 5 commits into
mainfrom
validate-refactor

jcogilvie commented May 31, 2026 •

edited

Loading

Uh oh!

jcogilvie commented May 31, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

jcogilvie Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jcogilvie commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

Uh oh!

jcogilvie commented May 31, 2026

Code review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jcogilvie commented May 31, 2026 •

edited

Loading