feat(validate): structured results and --output json|yaml flag#66
Open
jcogilvie wants to merge 4 commits into
Open
feat(validate): structured results and --output json|yaml flag#66jcogilvie wants to merge 4 commits into
jcogilvie wants to merge 4 commits into
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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 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 labels
Suggested reviewers
Thank you — please tell me if you'd like this split into smaller review checkpoints or a focused security/edge-case checklist. 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}
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" |
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.
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
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
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
Comment on lines
+33
to
+36
| errCannotMarshalJSON = "cannot marshal validation result as JSON" | ||
| errCannotMarshalYAML = "cannot marshal validation result as YAML" | ||
| errCannotWriteOutput = "cannot write validation result" | ||
| errUnknownFormat = "unknown output format" |
Collaborator
There was a problem hiding this comment.
Collaborator
Author
There was a problem hiding this comment.
Read the link as flagging the failed to prefix; I had two of those in validate.go's applyDefaults. Both swapped to cannot ... to match the rest of the package's wrap style. Pushed in 2b3f641.
(🤖)
| // OutputFormat values. | ||
| const ( | ||
| // OutputFormatText renders results in human-readable text format with | ||
| // [x], [!], [✓] markers. |
Collaborator
There was a problem hiding this comment.
how about using constants also for the markers?
Collaborator
Author
There was a problem hiding this comment.
Done — added markerSuccess/markerWarning/markerError constants and used them across Render and textErrorLine. Constants concatenate at compile time so the format strings still read top-to-bottom. Pushed in 2b3f641.
(🤖)
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
0fd8ed2 to
2b3f641
Compare
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.