From 90aa5a1bbb23d5ae786c496417251725a0530666 Mon Sep 17 00:00:00 2001 From: Jonathan Ogilvie Date: Tue, 20 Oct 2026 21:03:17 -0400 Subject: [PATCH] fix(diff): print errors once, from the command, not inline The renderers wrote every OutputError to stderr as "ERROR: Kind/name: ...", and the processors also returned the failure, which main printed again as "Error: ...". A failing run reported each error twice in two formats. Stop the renderers printing errors and drop OutputError.FormatError. Structured output keeps errors[] unchanged, and warnings still go to stderr when raised. main prints every returned error, schema-validation ones included, since the renderers no longer print them. DiffComposition now names each failing resource in its returned error, as PerformDiff already did. The integration harness prints a returned error as main does so stderr assertions see what a user sees. Backport of crossplane/cli#436 (19817b2). Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: Jonathan Ogilvie --- CLAUDE.md | 6 +- README.md | 12 ++-- cmd/diff/diff_integration_test.go | 6 ++ cmd/diff/diffprocessor/comp_processor.go | 31 +++++++-- cmd/diff/diffprocessor/comp_processor_test.go | 8 +-- cmd/diff/diffprocessor/diff_processor.go | 2 +- cmd/diff/diffprocessor/diff_processor_test.go | 6 +- cmd/diff/diffprocessor/errors.go | 4 +- cmd/diff/diffprocessor/warnings.go | 6 +- cmd/diff/main.go | 7 +- cmd/diff/renderer/comp_diff_renderer.go | 48 +++---------- cmd/diff/renderer/comp_diff_renderer_test.go | 37 ++++------ cmd/diff/renderer/diff_formatter.go | 4 +- cmd/diff/renderer/diff_renderer.go | 26 +++---- cmd/diff/renderer/diff_renderer_test.go | 69 +++++-------------- cmd/diff/renderer/structured_renderer.go | 19 ++--- cmd/diff/renderer/structured_renderer_test.go | 10 +-- cmd/diff/result/types.go | 19 +---- design/design-doc-cli-diff.md | 16 ++--- 19 files changed, 128 insertions(+), 208 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index db92c890..db62cf04 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -330,9 +330,11 @@ The tool prioritizes **accuracy over convenience**: When using structured output (`--output json` or `--output yaml`): -1. **Errors go to BOTH places**: Write errors to stderr (for human visibility) AND include them in the structured output (for machine parsing). This ensures: +1. **Errors are printed once, and also go in the structured output**: The renderers never print errors. The + processor returns them (each naming its resource) and `main` prints the returned error to stderr as `Error: ...`; + the renderer includes them in the structured output (for machine parsing). This ensures: - CI/CD tools can parse errors programmatically from stdout - - Humans see errors immediately in stderr + - Humans see each error once, on stderr - The structured output is always valid JSON/YAML, even when errors occur 2. **Consistent error structure**: Use the same `OutputError` type (defined in `result/types.go`) across all commands: diff --git a/README.md b/README.md index 1f262538..e3c9ce95 100644 --- a/README.md +++ b/README.md @@ -189,7 +189,7 @@ otherwise not warned about. Its message then begins "did not verify added resour apiserver (--dry-run-on=existing)" and has no cause key. Built-in types, single-version CRDs and resources requested at their storage version never get this note. A failed XR's diffs are withheld, so no summary is shown for it: the summary says how far to trust the diffs that are shown, and the XR's error is reported in -`errors[]` and on stderr; re-running after fixing it gives the full summary. (Before this was derived +`errors[]` and the command's error; re-running after fixing it gives the full summary. (Before this was derived from the diffs, a failed XR's warning could appear incidentally, because it was raised mid-calculation.) For `comp` the summary is run-wide, in the top-level `warnings[]`, not per composition. @@ -1145,11 +1145,11 @@ Exit-code precedence (per `DetermineExitCode`): any error in the run beats diff **Human-readable output** (`crossplane-diff xr invalid-xr.yaml`): -Per Unix convention, errors go to stderr and diff content goes to stdout. When validation is the only failure, stdout is empty and the structured failure detail appears on stderr, prefixed by an `ERROR: :` marker: +Per Unix convention, errors go to stderr and diff content goes to stdout. When validation is the only failure, stdout is empty and the structured failure detail appears on stderr, printed once, as the error the command fails with (`Error: ...`): ``` # stderr -ERROR: XNopResource/invalid-schema-xr: cannot validate resources: ns.diff.example.org/v1alpha1/XNopResource default/invalid-schema-xr: +Error: unable to process resource XNopResource/invalid-schema-xr: cannot validate resources: ns.diff.example.org/v1alpha1/XNopResource default/invalid-schema-xr: spec.coolField: Invalid value: "number": ... [schema] ns.nop.example.org/v1alpha1/XDownstreamResource default/invalid-schema-xr: spec.forProvider.configData: Invalid value: "boolean": ... [schema] @@ -1166,9 +1166,9 @@ Each per-resource block starts with a header that includes the resource identity - Namespaced resource: `/ /:` - Resource without `metadata.name` (e.g. a resource discovered missing a schema before it was named): collapses to just `/:` -Each indented error line has the shape ` []`, where `` is one of `[schema]`, `[cel]`, `[unknownField]`, or `[defaulting]`. A bad value is appended as `(got )` when it isn't already substring-present in the message. When some inputs in a batched run succeed and others fail validation, the successful diffs appear on stdout and the failing inputs' `ERROR:` blocks appear on stderr. +Each indented error line has the shape ` []`, where `` is one of `[schema]`, `[cel]`, `[unknownField]`, or `[defaulting]`. A bad value is appended as `(got )` when it isn't already substring-present in the message. When some inputs in a batched run succeed and others fail validation, the successful diffs appear on stdout and the failing inputs' `Error:` blocks appear on stderr. -This per-resource block form belongs to the local validator. A cluster rejection has nothing to break into field-level lines — the apiserver hands back one message — so it prints as a single `ERROR:` line and carries no `[]` suffix. Its `"admission"` type is visible in structured output only. +This per-resource block form belongs to the local validator. A cluster rejection has nothing to break into field-level lines — the apiserver hands back one message — so it prints as a single `Error:` line and carries no `[]` suffix. Its `"admission"` type is visible in structured output only. **Machine-readable output** (`crossplane-diff xr invalid-xr.yaml --output json`): @@ -1222,7 +1222,7 @@ The `OutputError` schema: | Field | Type | Description | |-------|------|-------------| | `resourceID` | string | Identifies which user-supplied input the diff was processing (one entry per batched run). Format: `/`. | -| `message` | string | Human-readable error string — the same text written to stderr. | +| `message` | string | Human-readable error string — the same text the command prints to stderr as part of its error. | | `validationFailures` | `[]ResourceValidationFailure`, optional | Structured per-resource breakdown. Set for local schema-validation failures and for cluster rejections during the dry run; `nil` for tool, IO, render, and scope-check errors. | `ResourceValidationFailure` carries `apiVersion`, `kind`, `name`, `namespace`, `status` (one of `"invalid"` or `"missingSchema"` — `"valid"` rows are filtered out), and `errors`, a list of `FieldValidationError` records: diff --git a/cmd/diff/diff_integration_test.go b/cmd/diff/diff_integration_test.go index 7d91b5ee..3bd4e94d 100644 --- a/cmd/diff/diff_integration_test.go +++ b/cmd/diff/diff_integration_test.go @@ -485,6 +485,12 @@ func runIntegrationTest(t *testing.T, testType DiffTestType, tt IntegrationTestC t.Errorf("expected exit code %d, got %d", tt.expectedExitCode, exitCode.Code) } + // Print a returned error as main does, the only place errors are printed, so the stderr + // assertions see what a user sees. + if err != nil { + fmt.Fprintf(&stderr, "Error: %v\n", err) + } + // Assert stderr expectations before any of the early returns below, so they apply in every // output mode (human-readable, JSON, and expected-error cases alike). for _, want := range tt.expectedStderrContains { diff --git a/cmd/diff/diffprocessor/comp_processor.go b/cmd/diff/diffprocessor/comp_processor.go index c0f1a8fb..2422c397 100644 --- a/cmd/diff/diffprocessor/comp_processor.go +++ b/cmd/diff/diffprocessor/comp_processor.go @@ -270,18 +270,37 @@ func (p *DefaultCompDiffProcessor) DiffComposition(ctx context.Context, composit totalXRErrors := len(output.Errors) output.Errors = withInterruption(ctx, output.Errors) - // Return an error so CI/CD pipelines get a non-zero exit code when impact analysis failed. The - // report is returned with it, so the caller still renders it (structured output stays valid). + // Return an error so CI/CD pipelines get a non-zero exit code. The report is returned with it, so + // the caller still renders it (structured output stays valid). The renderers do not print errors, + // so the returned error names each failing resource; the command prints it once. A run in which + // only some compositions failed still succeeds: each failure is in the output, under its composition. + var errs []error + if totalXRErrors > 0 { - return output, errors.Errorf("impact analysis failed for %d XR(s)", totalXRErrors) + errs = append(errs, errors.Wrapf(outputErrorsJoined(output.Errors[:totalXRErrors]), "impact analysis failed for %d XR(s)", totalXRErrors)) } - // Return error if all compositions failed if compositionErrors > 0 && compositionErrors == len(output.Compositions) { - return output, errors.New("failed to process all compositions") + failed := make([]dt.OutputError, 0, compositionErrors) + for _, comp := range output.Compositions { + failed = append(failed, *comp.Error) + } + + errs = append(errs, errors.Wrap(outputErrorsJoined(failed), "failed to process all compositions")) + } + + return output, errors.Join(errs...) +} + +// outputErrorsJoined returns errs as one error that names each one's resource, the way PerformDiff +// names the resources it failed to process. +func outputErrorsJoined(errs []dt.OutputError) error { + joined := make([]error, 0, len(errs)) + for _, e := range errs { + joined = append(joined, errors.Errorf("unable to process resource %s: %s", e.ResourceID, e.Message)) } - return output, nil + return errors.Join(joined...) } // preflightResourceRefs resolves user --resource refs against every supplied composition before diff --git a/cmd/diff/diffprocessor/comp_processor_test.go b/cmd/diff/diffprocessor/comp_processor_test.go index 0415a0fd..cca93f33 100644 --- a/cmd/diff/diffprocessor/comp_processor_test.go +++ b/cmd/diff/diffprocessor/comp_processor_test.go @@ -1640,15 +1640,15 @@ func TestDefaultCompDiffProcessor_DiffComposition_ReportErrorOutput(t *testing.T t.Error("Expected error due to XR processing failure, got nil") } - // The renderer writes each as FormatError() does: "ERROR: {ResourceID}: {Message}". if output == nil || len(output.Errors) != 1 { t.Fatalf("Expected one error in the report, got: %+v", output) } - got := output.Errors[0].FormatError() - for _, want := range []string{"ERROR:", "XResource/fail-xr", "render pipeline failed"} { + // The returned error is the only place a human sees the failure, so it names the resource. + got := err.Error() + for _, want := range []string{"impact analysis failed for 1 XR(s)", "XResource/fail-xr", "render pipeline failed"} { if !strings.Contains(got, want) { - t.Errorf("Expected the report's error to contain %q, got: %q", want, got) + t.Errorf("Expected the returned error to contain %q, got: %q", want, got) } } } diff --git a/cmd/diff/diffprocessor/diff_processor.go b/cmd/diff/diffprocessor/diff_processor.go index 35b87f41..ee73a6af 100644 --- a/cmd/diff/diffprocessor/diff_processor.go +++ b/cmd/diff/diffprocessor/diff_processor.go @@ -331,7 +331,7 @@ func (p *DefaultDiffProcessor) PerformDiff(ctx context.Context, resources []*un. } // Third: renders that reach one resource in ways no single diff can represent. Collected into the - // report so they reach errors[] and stderr like any other error; the report is still rendered, so + // report so they reach errors[] and the returned error like any other error; the report is still rendered, so // structured output stays valid. for _, err := range validator.RenderOverlaps(groups) { errs = append(errs, err) diff --git a/cmd/diff/diffprocessor/diff_processor_test.go b/cmd/diff/diffprocessor/diff_processor_test.go index b4740df9..4c25c833 100644 --- a/cmd/diff/diffprocessor/diff_processor_test.go +++ b/cmd/diff/diffprocessor/diff_processor_test.go @@ -1178,13 +1178,13 @@ func TestDefaultDiffProcessor_PerformDiff_ReportErrorOutput(t *testing.T) { t.Fatal("PerformDiff() expected error but got none") } - // The renderer writes each as FormatError() does: "ERROR: {ResourceID}: {Message}". + // The returned error is the only place a human sees the failure, so it names the resource. if len(report.Errors) != 1 { t.Fatalf("Expected one error in the report, got: %v", report.Errors) } - if got := report.Errors[0].FormatError(); !strings.Contains(got, "ERROR: XR1/my-xr-1:") || !strings.Contains(got, "composition not found") { - t.Errorf("Expected the report's error to name XR1/my-xr-1 and 'composition not found', got: %q", got) + if got := err.Error(); !strings.Contains(got, "XR1/my-xr-1") || !strings.Contains(got, "composition not found") { + t.Errorf("Expected the returned error to name XR1/my-xr-1 and 'composition not found', got: %q", got) } } diff --git a/cmd/diff/diffprocessor/errors.go b/cmd/diff/diffprocessor/errors.go index 270ba2b0..6da3046f 100644 --- a/cmd/diff/diffprocessor/errors.go +++ b/cmd/diff/diffprocessor/errors.go @@ -46,7 +46,7 @@ const ( ExitCodeInterrupted = 130 ) -// ErrInterrupted is the error an interrupted run reports, in errors[] and on stderr, in place of the +// ErrInterrupted is the error an interrupted run reports, in errors[] and as the command's error, in place of the // cancelled calls the interruption caused. var ErrInterrupted = errors.New("run interrupted by signal; results are incomplete") @@ -197,7 +197,7 @@ func NewOutputError(resourceID string, err error) dt.OutputError { // // The inner SchemaValidationError deliberately gets an empty ResourceID: its // Error() would otherwise prefix "schema validation error for : ", which -// both duplicates the id that OutputError.FormatError already prints and +// both duplicates the id OutputError.ResourceID already carries and // mislabels an admission rejection as a schema problem. func NewAdmissionRejectionError(rejectedDesc string, obj *un.Unstructured, err error) *SchemaValidationError { msg := fmt.Sprintf("the cluster rejected %s: %s", rejectedDesc, err.Error()) diff --git a/cmd/diff/diffprocessor/warnings.go b/cmd/diff/diffprocessor/warnings.go index 98777fa5..94ef5b1b 100644 --- a/cmd/diff/diffprocessor/warnings.go +++ b/cmd/diff/diffprocessor/warnings.go @@ -43,9 +43,9 @@ import ( // // # Why it writes to stderr itself // -// Owning the human rendering here means one mechanism produces both views, the same dual-emission -// contract OutputError follows, and the text stays fit for a CLI rather than taking a logger's -// development-mode format (timestamp, level, caller). +// Owning the human rendering here means one mechanism produces both views, and the text stays fit +// for a CLI rather than taking a logger's development-mode format (timestamp, level, caller). +// Errors differ: they are returned to the command, which prints them once. // // # Why identical advisories are collapsed // diff --git a/cmd/diff/main.go b/cmd/diff/main.go index c9ebb02f..a34efd14 100644 --- a/cmd/diff/main.go +++ b/cmd/diff/main.go @@ -256,11 +256,8 @@ func main() { err := ctx.Run() // Handle error output - commands set exitCode.Code based on their results if err != nil { - // Schema validation errors are already rendered by the processors as part of the - // diff output, so don't duplicate them here. Only print other (tool) errors. - if !dp.IsSchemaValidationError(err) { - fmt.Fprintf(os.Stderr, "Error: %v\n", err) - } + // The renderers do not print errors, so this is the only place a human sees them. + fmt.Fprintf(os.Stderr, "Error: %v\n", err) // If the command returned an error but didn't set an exit code, default to tool error if exitCode.Code == dp.ExitCodeSuccess { exitCode.Code = dp.ExitCodeToolError diff --git a/cmd/diff/renderer/comp_diff_renderer.go b/cmd/diff/renderer/comp_diff_renderer.go index 9efad069..21e20b24 100644 --- a/cmd/diff/renderer/comp_diff_renderer.go +++ b/cmd/diff/renderer/comp_diff_renderer.go @@ -40,8 +40,9 @@ const ( // Both human-readable and structured (JSON/YAML) renderers implement this interface. type CompDiffRenderer interface { // RenderCompDiff renders the complete composition diff output. - // Top-level and tool errors go to DiffOptions.Stderr and are included in - // structured output payloads. Per-composition messages (diffs, "no changes", + // Top-level errors are included in structured output payloads but not + // written to stderr: the processor returns them to the command, which + // prints them once. Per-composition messages (diffs, "no changes", // per-composition errors) go to DiffOptions.Stdout as part of the diff narrative. RenderCompDiff(output *dt.CompDiffOutput) error } @@ -63,8 +64,8 @@ func NewDefaultCompDiffRenderer(logger logging.Logger, diffRenderer DiffRenderer } // RenderCompDiff renders the composition diff in human-readable format. -// Top-level errors go to r.opts.Stderr. Per-composition output (diffs, status -// messages, per-composition errors) goes to r.opts.Stdout. +// Per-composition output (diffs, status messages, per-composition errors) goes +// to r.opts.Stdout. Top-level errors are not printed; see CompDiffRenderer. func (r *DefaultCompDiffRenderer) RenderCompDiff(output *dt.CompDiffOutput) error { stdout := r.opts.Stdout @@ -82,18 +83,7 @@ func (r *DefaultCompDiffRenderer) RenderCompDiff(output *dt.CompDiffOutput) erro // One run-wide summary of unverified additions; see DiffRenderer.RenderDiffs for why it is derived // at render time, and why the per-composition reuse of the diff renderer above does not emit it. - if err := writeWarnings(r.opts.Stderr, dt.CompDryRunWarnings(output)); err != nil { - return err - } - - // Write top-level errors to stderr - for _, e := range output.Errors { - if _, err := fmt.Fprintln(r.opts.Stderr, e.FormatError()); err != nil { - return errors.Wrap(err, "failed to write error to stderr") - } - } - - return nil + return writeWarnings(r.opts.Stderr, dt.CompDryRunWarnings(output)) } // renderComposition renders every section belonging to a single composition: its own changes, @@ -612,8 +602,8 @@ func NewStructuredCompDiffRenderer(logger logging.Logger, opts DiffOptions) Comp } // RenderCompDiff renders the composition diff in structured format (JSON/YAML). -// Top-level errors go to both r.opts.Stderr (for human visibility) and the -// structured output payload. Per-composition data goes to r.opts.Stdout. +// Top-level errors go into the payload only; see CompDiffRenderer. +// Per-composition data goes to r.opts.Stdout. func (r *StructuredCompDiffRenderer) RenderCompDiff(output *dt.CompDiffOutput) error { // One run-wide summary of unverified additions, derived here from the diffs and appended after the // warnings raised during the run. See DiffRenderer.RenderDiffs. @@ -626,14 +616,9 @@ func (r *StructuredCompDiffRenderer) RenderCompDiff(output *dt.CompDiffOutput) e v2.Warnings = warnings // Anything but v2, including the empty value, is v1. - var ( - jsonOutput any = v2 - errs = v2.Errors - ) - + var jsonOutput any = v2 if r.opts.OutputVersion != OutputVersionV2 { - v1 := v2.v1() - jsonOutput, errs = v1, v1.Errors + jsonOutput = v2.v1() } var ( @@ -662,18 +647,7 @@ func (r *StructuredCompDiffRenderer) RenderCompDiff(output *dt.CompDiffOutput) e } // Of the warnings, only the ones derived here go to stderr: the rest went there when raised. - if err := writeWarnings(r.opts.Stderr, dryRun); err != nil { - return err - } - - // Write errors to stderr for human visibility (they're also included in the structured output) - for _, e := range errs { - if _, err := fmt.Fprintln(r.opts.Stderr, e.FormatError()); err != nil { - return errors.Wrap(err, "failed to write error to stderr") - } - } - - return nil + return writeWarnings(r.opts.Stderr, dryRun) } // buildStructuredCompOutput converts internal CompDiffOutput to the v2 document, from which v1 is diff --git a/cmd/diff/renderer/comp_diff_renderer_test.go b/cmd/diff/renderer/comp_diff_renderer_test.go index 65453315..eb62db4a 100644 --- a/cmd/diff/renderer/comp_diff_renderer_test.go +++ b/cmd/diff/renderer/comp_diff_renderer_test.go @@ -649,10 +649,10 @@ func TestDefaultCompDiffRenderer_RenderCompDiff(t *testing.T) { } } -// TestDefaultCompDiffRenderer_RenderCompDiff_TopLevelErrorsToStderr verifies that +// TestDefaultCompDiffRenderer_RenderCompDiff_TopLevelErrorsNotPrinted verifies that // top-level errors in CompDiffOutput.Errors are written to stderr (not stdout) // to follow Unix conventions. -func TestDefaultCompDiffRenderer_RenderCompDiff_TopLevelErrorsToStderr(t *testing.T) { +func TestDefaultCompDiffRenderer_RenderCompDiff_TopLevelErrorsNotPrinted(t *testing.T) { output := &dt.CompDiffOutput{ Compositions: []dt.CompositionDiff{}, Errors: []dt.OutputError{ @@ -680,23 +680,21 @@ func TestDefaultCompDiffRenderer_RenderCompDiff_TopLevelErrorsToStderr(t *testin t.Fatalf("RenderCompDiff() failed: %v", err) } - stderrOut := stderr.String() - for _, e := range output.Errors { - if !strings.Contains(stderrOut, e.FormatError()) { - t.Errorf("Expected stderr to contain %q, got: %q", e.FormatError(), stderrOut) - } + if stderr.Len() != 0 { + t.Errorf("Expected nothing on stderr, got: %q", stderr.String()) + } - // Verify errors were NOT written to stdout - if strings.Contains(stdout.String(), e.FormatError()) { - t.Errorf("Expected stdout to NOT contain error %q, got: %q", e.FormatError(), stdout.String()) + for _, e := range output.Errors { + if strings.Contains(stdout.String(), e.Message) { + t.Errorf("Expected stdout to NOT contain error %q, got: %q", e.Message, stdout.String()) } } } -// TestStructuredCompDiffRenderer_RenderCompDiff_TopLevelErrorsToStderr verifies that +// TestStructuredCompDiffRenderer_RenderCompDiff_TopLevelErrorsNotPrinted verifies that // top-level errors in CompDiffOutput.Errors are written to stderr in addition to being // included in the structured output. -func TestStructuredCompDiffRenderer_RenderCompDiff_TopLevelErrorsToStderr(t *testing.T) { +func TestStructuredCompDiffRenderer_RenderCompDiff_TopLevelErrorsNotPrinted(t *testing.T) { output := &dt.CompDiffOutput{ Compositions: []dt.CompositionDiff{}, Errors: []dt.OutputError{ @@ -725,12 +723,9 @@ func TestStructuredCompDiffRenderer_RenderCompDiff_TopLevelErrorsToStderr(t *tes t.Fatalf("RenderCompDiff() failed: %v", err) } - // Verify errors in stderr - stderrOut := stderr.String() - for _, e := range output.Errors { - if !strings.Contains(stderrOut, e.FormatError()) { - t.Errorf("Expected stderr to contain %q, got: %q", e.FormatError(), stderrOut) - } + // Errors are returned to the command, which prints them once; the renderer does not + if stderr.Len() != 0 { + t.Errorf("Expected nothing on stderr, got: %q", stderr.String()) } // Verify errors are ALSO in structured output (stdout) @@ -1447,7 +1442,6 @@ func TestStructuredCompDiffRenderer_OutputVersions(t *testing.T) { // v1 does not list the failed composition here. Errors: []dt.OutputError{*xrErr}, }, - wantStderr: []string{xrErr.FormatError()}, }, "V2": { version: OutputVersionV2, @@ -1474,7 +1468,6 @@ func TestStructuredCompDiffRenderer_OutputVersions(t *testing.T) { // v2 appends the failed composition, so the list is the union of the document's errors. Errors: []dt.OutputError{*xrErr, *compErr}, }, - wantStderr: []string{xrErr.FormatError(), compErr.FormatError()}, }, } @@ -1504,8 +1497,8 @@ func TestStructuredCompDiffRenderer_OutputVersions(t *testing.T) { t.Errorf("structured output mismatch (-want +got):\n%s", diff) } - if diff := gcmp.Diff(tc.wantStderr, strings.Split(strings.TrimSpace(stderr.String()), "\n")); diff != "" { - t.Errorf("stderr mismatch (-want +got):\n%s", diff) + if stderr.Len() != 0 { + t.Errorf("expected nothing on stderr, got: %q", stderr.String()) } }) } diff --git a/cmd/diff/renderer/diff_formatter.go b/cmd/diff/renderer/diff_formatter.go index c79ab380..dac78dbc 100644 --- a/cmd/diff/renderer/diff_formatter.go +++ b/cmd/diff/renderer/diff_formatter.go @@ -16,8 +16,8 @@ type DiffOptions struct { // Stdout is the writer for diff output (defaults to os.Stdout) Stdout io.Writer - // Stderr is the writer for error output (defaults to os.Stderr) - // Errors are written here following Unix conventions (errors to stderr, output to stdout) + // Stderr is the writer for warnings (defaults to os.Stderr). Errors are not + // written here: they are returned to the command, which prints them once. Stderr io.Writer // Format specifies the output format (diff, json, yaml) diff --git a/cmd/diff/renderer/diff_renderer.go b/cmd/diff/renderer/diff_renderer.go index dc320d47..627ca6eb 100644 --- a/cmd/diff/renderer/diff_renderer.go +++ b/cmd/diff/renderer/diff_renderer.go @@ -30,10 +30,11 @@ func flattenGroups(groups []dt.XRDiffGroup) map[string]*dt.ResourceDiff { // DiffRenderer handles rendering diffs to output. type DiffRenderer interface { - // RenderDiffs formats and outputs diffs, grouped by input XR. - // Diff output goes to DiffOptions.Stdout, errors go to DiffOptions.Stderr. - // The errs parameter contains the union of resource processing errors to - // include in output (the top-level/global error list). + // RenderDiffs formats and outputs diffs, grouped by input XR, to + // DiffOptions.Stdout. The errs parameter contains the union of resource + // processing errors (the top-level/global error list). A structured + // renderer includes them in its output; no renderer writes them to stderr, + // because the processor returns them to the command, which prints them once. // // The warnings parameter carries non-fatal advisories for structured output only. Unlike errors, // warnings have ALREADY been written to stderr, at the moment they were raised (see @@ -203,8 +204,8 @@ func identitylessGroups(diffs map[string]*dt.ResourceDiff) []dt.XRDiffGroup { return []dt.XRDiffGroup{{Diffs: diffs}} } -// RenderDiffs formats and prints the diffs. -// Diff output goes to r.diffOpts.Stdout, errors go to r.diffOpts.Stderr. +// RenderDiffs formats and prints the diffs to r.diffOpts.Stdout. It does not +// print errs; the command prints the error the processor returns. // // Identity-bearing groups (the xr command) render as per-input-XR sections, // each with a header and per-section summary, followed by an aggregate footer @@ -248,18 +249,7 @@ func (r *DefaultDiffRenderer) RenderDiffs(groups []dt.XRDiffGroup, errs []dt.Out return err } - if err := writeWarnings(r.diffOpts.Stderr, dt.XRDryRunWarnings(xrGroups)); err != nil { - return err - } - - // Write errors to stderr following Unix conventions - for _, e := range errs { - if _, err := fmt.Fprintln(r.diffOpts.Stderr, e.FormatError()); err != nil { - return errors.Wrap(err, "failed to write error to stderr") - } - } - - return nil + return writeWarnings(r.diffOpts.Stderr, dt.XRDryRunWarnings(xrGroups)) } // renderFlat renders a single flat block of diffs plus a "Summary:" line. This diff --git a/cmd/diff/renderer/diff_renderer_test.go b/cmd/diff/renderer/diff_renderer_test.go index db268bda..6d917c29 100644 --- a/cmd/diff/renderer/diff_renderer_test.go +++ b/cmd/diff/renderer/diff_renderer_test.go @@ -346,63 +346,30 @@ func TestDefaultDiffRenderer_RenderDiffs(t *testing.T) { } } -func TestDefaultDiffRenderer_RenderDiffs_WithErrors(t *testing.T) { - tests := map[string]struct { - errs []dt.OutputError - expected []string - }{ - "ErrorsWithResourceID": { - errs: []dt.OutputError{ - {ResourceID: "Resource/my-resource", Message: "failed to render"}, - {ResourceID: "OtherResource/other", Message: "connection refused"}, - }, - expected: []string{ - "ERROR: Resource/my-resource: failed to render", - "ERROR: OtherResource/other: connection refused", - }, - }, - "ErrorsWithEmptyResourceID": { - errs: []dt.OutputError{ - {ResourceID: "", Message: "cluster connection timeout"}, - {ResourceID: "Resource/has-id", Message: "specific error"}, - }, - expected: []string{ - "ERROR: : cluster connection timeout", - "ERROR: Resource/has-id: specific error", - }, - }, - "NoErrors": { - errs: []dt.OutputError{}, - expected: []string{}, - }, +// TestDefaultDiffRenderer_RenderDiffs_ErrorsNotPrinted verifies the renderer writes no error to +// stderr: the processor returns them to the command, which prints them once. +func TestDefaultDiffRenderer_RenderDiffs_ErrorsNotPrinted(t *testing.T) { + errs := []dt.OutputError{ + {ResourceID: "Resource/my-resource", Message: "failed to render"}, + {Message: "cluster connection timeout"}, } - for name, tt := range tests { - t.Run(name, func(t *testing.T) { - logger := tu.TestLogger(t, false) - - // Errors go to stderr now, so capture stderr - var stderr bytes.Buffer + var stdout, stderr bytes.Buffer - opts := DefaultDiffOptions() - opts.Stdout = &bytes.Buffer{} // discard stdout - opts.Stderr = &stderr + opts := DefaultDiffOptions() + opts.Stdout = &stdout + opts.Stderr = &stderr - renderer := NewDiffRenderer(logger, opts) + renderer := NewDiffRenderer(tu.TestLogger(t, false), opts) - err := renderer.RenderDiffs(identitylessGroups(map[string]*dt.ResourceDiff{}), tt.errs, nil) - if err != nil { - t.Fatalf("RenderDiffs() failed with error: %v", err) - } - - output := stderr.String() + if err := renderer.RenderDiffs(identitylessGroups(map[string]*dt.ResourceDiff{}), errs, nil); err != nil { + t.Fatalf("RenderDiffs() failed with error: %v", err) + } - for _, expectedMsg := range tt.expected { - if !strings.Contains(output, expectedMsg) { - t.Errorf("Expected stderr to contain %q but it didn't\nStderr: %s", expectedMsg, output) - } - } - }) + for _, e := range errs { + if strings.Contains(stderr.String(), e.Message) || strings.Contains(stdout.String(), e.Message) { + t.Errorf("Expected error %q to be printed nowhere, got stdout %q, stderr %q", e.Message, stdout.String(), stderr.String()) + } } } diff --git a/cmd/diff/renderer/structured_renderer.go b/cmd/diff/renderer/structured_renderer.go index 59b7bb86..1b4470be 100644 --- a/cmd/diff/renderer/structured_renderer.go +++ b/cmd/diff/renderer/structured_renderer.go @@ -3,7 +3,6 @@ package renderer import ( "cmp" "encoding/json" - "fmt" "maps" "reflect" "slices" @@ -389,20 +388,10 @@ func (r *StructuredDiffRenderer) RenderDiffs(groups []dt.XRDiffGroup, errs []dt. return errors.Wrap(err, "failed to write newline") } - // Write errors to stderr for human visibility (they're also included in the structured output). - // Of the warnings, only the ones derived here are written: the rest already went to stderr when - // they were raised. - if err := writeWarnings(r.opts.Stderr, dryRun); err != nil { - return err - } - - for _, e := range errs { - if _, err := fmt.Fprintln(r.opts.Stderr, e.FormatError()); err != nil { - return errors.Wrap(err, "failed to write error to stderr") - } - } - - return nil + // Errors are in the structured output, and the command prints the error the processor returns, + // so they are not written to stderr here. Of the warnings, only the ones derived here are + // written: the rest already went to stderr when they were raised. + return writeWarnings(r.opts.Stderr, dryRun) } // buildChangeSet walks a diff map into the (summary, changes) pair shared by diff --git a/cmd/diff/renderer/structured_renderer_test.go b/cmd/diff/renderer/structured_renderer_test.go index 6240bb32..3da6c07d 100644 --- a/cmd/diff/renderer/structured_renderer_test.go +++ b/cmd/diff/renderer/structured_renderer_test.go @@ -6,7 +6,6 @@ import ( "maps" "reflect" "slices" - "strings" "testing" "github.com/crossplane-contrib/crossplane-diff/cmd/diff/compare" @@ -586,12 +585,9 @@ func TestStructuredDiffRenderer_RenderDiffs_ErrorsToStderr(t *testing.T) { t.Fatalf("RenderDiffs() failed: %v", err) } - // Verify errors are in stderr - stderrOut := stderr.String() - for _, e := range errs { - if !strings.Contains(stderrOut, e.FormatError()) { - t.Errorf("Expected stderr to contain %q, got: %q", e.FormatError(), stderrOut) - } + // Errors are returned to the command, which prints them once; the renderer does not + if stderr.Len() != 0 { + t.Errorf("Expected nothing on stderr, got: %q", stderr.String()) } // Verify errors are ALSO in structured output (stdout) diff --git a/cmd/diff/result/types.go b/cmd/diff/result/types.go index e3e4d9fb..99419399 100644 --- a/cmd/diff/result/types.go +++ b/cmd/diff/result/types.go @@ -435,20 +435,8 @@ type FieldValidationError struct { Value any `json:"value,omitempty"` } -// FormatError returns a human-readable error string. -// If ResourceID is empty, it uses "" to indicate a system-level error -// not tied to any specific resource (e.g., cluster connection issues). -func (e OutputError) FormatError() string { - resourceID := e.ResourceID - if resourceID == "" { - resourceID = "" - } - - return fmt.Sprintf("ERROR: %s: %s", resourceID, e.Message) -} - // OutputWarning is a non-fatal advisory: something the user should know that does not invalidate the -// diff or stop the run. Warnings follow the same dual-emission contract as OutputError — written to +// diff or stop the run. Warnings follow a dual-emission contract — written to // stderr for humans and included in structured output for machines — but deliberately do NOT affect // the exit code. A consumer gating on failure checks errors[], not warnings[]. // @@ -472,9 +460,8 @@ type Advisories interface { Raise(w OutputWarning) } -// FormatWarning renders the warning as a single human-readable stderr line, mirroring -// OutputError.FormatError. Context pairs are appended in sorted key order so output is stable -// across runs (Go map iteration is not). +// FormatWarning renders the warning as a single human-readable stderr line. Context pairs are +// appended in sorted key order so output is stable across runs (Go map iteration is not). func (w OutputWarning) FormatWarning() string { if len(w.Context) == 0 { return fmt.Sprintf("WARNING: %s", w.Message) diff --git a/design/design-doc-cli-diff.md b/design/design-doc-cli-diff.md index 27dff18f..8b20b4cc 100644 --- a/design/design-doc-cli-diff.md +++ b/design/design-doc-cli-diff.md @@ -365,8 +365,8 @@ The integration test cases cover: - **Summary Output**: Tests that the summary of changes (added, modified, removed) is correctly generated. - **Structured Output (JSON / YAML)**: Field-level assertions on `--output json` / `--output yaml` payloads using `tu.AssertStructuredDiff` and `tu.AssertStructuredCompDiff` builders. These let tests assert on specific old/new - values and on summary counts without depending on ANSI escape sequences. Errors must appear in both stderr and the - structured output. + values and on summary counts without depending on ANSI escape sequences. Errors must appear in both stderr (printed + once, by the command) and the structured output. - **Typed Validation Failures**: Schema-validation tests use the fluent `WithError` / `WithValidationFailure` / `WithFieldError` builder chain to pin specific GVKs, namespaces, statuses, error types (`schema`, `cel`, `unknownField`, `defaulting`), and field paths. Message wording is intentionally not asserted so apimachinery @@ -666,7 +666,7 @@ The data flow through the system follows these key steps: 3. **Output**: - Clean up function containers - - Render the diffs as text, JSON or YAML to stdout; errors go to stderr + - Render the diffs as text, JSON or YAML to stdout; the returned error is printed once, to stderr, by the command ## 6. Component Design @@ -1654,13 +1654,13 @@ diffs do not") and a `versions` context key (`"requested v1alpha1, stored v1"`). such a group: the user chose `--dry-run-on=existing`, so its unverified additions are not news, but the version mismatch is. Its message has no cause key. The summaries are sorted (diffs arrive in map order), so they change the order of `warnings[]`: they come after the warnings raised during the run, grouped, and on -stderr just before any errors. For `comp` there is one run-wide summary, across every composition, +stderr (an error is printed after, by the command). For `comp` there is one run-wide summary, across every composition, because `warnings[]` is top-level; the human comp renderer reuses `DefaultDiffRenderer` per composition with identity-less groups, so `DefaultDiffRenderer` summarises identity-bearing groups only and leaves the run-wide summary to `DefaultCompDiffRenderer`. A failed XR's diffs are withheld, so no dry-run summary is shown for it: the summary says how far to -trust the diffs that are shown, and the XR's error is reported in `errors[]` and on stderr; +trust the diffs that are shown, and the XR's error is reported in `errors[]` and the command's error; re-running after fixing it gives the full summary. This differs from the calculator-raised warning it replaces, which appeared for a failed XR only incidentally, because it fired mid-calculation. Carrying a failed XR's unverified additions through to the renderer was considered and rejected: it would have @@ -1696,7 +1696,7 @@ The processors build the same internal types either way; only the structured ren Every structured document names its version in a top-level `outputVersion` field; adding it is the only change to v1 output. -Under `json`/`yaml`, errors are written to **both** stderr (for human visibility) and the structured output (so CI/CD +Under `json`/`yaml`, errors go to **both** stderr (for human visibility; the processor returns them and the command prints them once, each naming its resource) and the structured output (so CI/CD pipelines can parse them programmatically). The structured renderers always emit a valid document — even when every XR fails — by attaching errors to an `OutputError` field. `OutputError` uses JSON struct tags only (the YAML library reads JSON tags), so the field naming is consistent across formats. @@ -1957,8 +1957,8 @@ contract: `Context` under whatever key the call site used. `FormatWarning` renders the stderr line, sorting context keys so output is byte-stable across runs. - Warnings follow the same dual-emission contract as errors — stderr for humans, structured output for - machines — with two deliberate differences: they never affect the exit code (a consumer gating on + Warnings are emitted twice — stderr for humans, structured output for machines — whereas an error is + returned to the command, which prints it once, and also goes in structured output. Two further differences: they never affect the exit code (a consumer gating on failure reads `errors[]`), and the stderr half is written when the warning is *raised* rather than at render time. Emitting at render time would lose any warning raised during a run that fails before rendering, and would report warnings out of chronological order with the work that produced them.