feat(render): capture stderr and handle pipeline-fatal exit code in both engines#91
Merged
adamwg merged 7 commits intoJun 9, 2026
Merged
Conversation
…oth engines Both render engines previously dropped information that downstream tools need: - localRenderEngine piped stderr straight to os.Stderr, so callers couldn't programmatically inspect FATAL pipeline messages. - dockerRenderEngine discarded stderr at the engine layer (the RunContainer return value was assigned to `_`); only on container error did stderr surface, folded into a fmt.Errorf string. - Neither engine implemented the partial-output-on-fatal contract from crossplane/crossplane#7455: when `crossplane internal render` exits with code 3 it populates stdout with a partial RenderResponse so callers can recover output.RequiredResources and iterate, but both engines wrapped the *exec.ExitError / *ContainerExitError and dropped stdout. This change: - Adds ExitCodePipelineFatal (= 3) in cmd/crossplane/render/render.go, documenting the contract. - Adds *ContainerExitError to internal/docker (carries ExitCode + captured stderr) so callers can errors.As against it. RunContainer returns it on non-zero exit instead of an opaque fmt.Errorf. - Captures stderr in localRenderEngine.Render via bytes.Buffer instead of os.Stderr, and surfaces it in the returned error. - Uses RunContainer's stderr return in dockerRenderEngine.Render and surfaces it in the returned error. - On exit-3 with non-empty stdout, both engines parse the partial *RenderResponse and return (rsp, err) — both non-nil — so callers can recover the partial output. - Adds engine_local_test.go (helper-process pattern via TestMain re-exec) and engine_docker_test.go (injectable runContainer fn) covering success, fatal-with-partial, fatal-no-partial, and hard-fail paths. Motivating downstream use case: crossplane-contrib/crossplane-diff currently wraps localRenderEngine to add this behaviour. See crossplane-contrib/crossplane-diff#326 — once this lands the wrapper can be deleted and crossplane-diff can drop its local-binary fallback entirely. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
The runContainer field on dockerRenderEngine and the env-var-driven TestMain dispatch in engine_local_test.go are both standard Go test patterns, but they're not obvious at a glance — and reviewer questions on the downstream PR (crossplane-contrib/crossplane-diff#326) showed both warrant inline justification. - Expand the doc comment on dockerRenderEngine.runContainer to explain why it exists (hermetic failure-mode coverage without a real Docker daemon) and why a per-instance unexported field is preferable to a package-level var (the latter would race in parallel tests). - Expand the doc comment on engine_local_test.go's envHelperMode + TestMain to explain the helper-process / re-exec idiom: why we re-use the test binary as a stand-in for the `crossplane` binary (no shell scripts, no testdata helper binary, deterministic protobuf payloads), how dispatch works (parent t.Setenv -> child inherits -> TestMain sees the var and exits before m.Run), and what the trade-off is (extra TestMain plumbing in exchange for a self-contained, cross-platform fake binary). No functional changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
The dockerRenderEngine test seam was a function-typed field (runContainerFn). Replace it with a one-method interface (containerRunner) plus a realContainerRunner adapter, mirroring the existing pullClient/mockPullClient pattern used in runtime_docker.go in the same package. Tests now use mockContainerRunner with the same struct-of-Mock<Method>-funcs shape as mockPullClient. Same behaviour, same coverage, same hermetic execution — just expressed in the package's established idiom so reviewers can pattern-match against the existing convention rather than evaluating a new one. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
commented
Jun 8, 2026
| // The cost is a few extra lines of TestMain plumbing; the benefit is a | ||
| // fully self-contained, deterministic, cross-platform fake binary with | ||
| // access to the same proto types the tests assert on. | ||
| const envHelperMode = "GO_HELPER_LOCAL_RENDER_MODE" |
Collaborator
Author
There was a problem hiding this comment.
I was a little suspect of this, but apparently this is how go's exec tests itself, so the pattern is established.
Member
There was a problem hiding this comment.
Certainly a little complex, but the comment explains it clearly and I don't see an obviously better solution 👍
- engine.go: document the two-non-nil (rsp, err) return contract on the Engine interface itself. Previously only the concrete implementations carried the doc — anything consuming the interface (or the mock in engine_mock.go) would assume the standard Go "nil-rsp on err" shape. - engine_docker.go: fix double-stderr in the hard-fail path. *docker.ContainerExitError.Error() already embeds stderr as "container exited with status N: <stderr>"; the previous code unconditionally appended the captured stderr buffer on top of that, producing messages like "...status 1: the container is sad: the container is sad". Now we wrap on *ContainerExitError (whose Error already includes stderr) and only append stderr explicitly for non-exit errors (e.g. image pull failures) where err.Error() doesn't. - engine_local.go: trim trailing ": " when stderr is empty. Cosmetic but produced messages like "cannot run crossplane internal render: exit status 1: " on the no-stderr path. - engine_docker_test.go: add wantSingleOccurrence assertion to catch double-formatting bugs at test time. Both FatalWithNoPartialOutput and HardFailWithExitError now assert their stderr text appears exactly once in the returned error. Would have flagged the double-stderr bug above before review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Resolve a conflict in cmd/crossplane/render/render.go: upstream removed the Annotations constants block (commits dd1b775 + b665f70 — render now uses the constants from xcrd directly) while this branch was adding ExitCodePipelineFatal immediately above that block. Kept ExitCodePipelineFatal, dropped the removed Annotations block. No semantic changes to this branch's contribution. All tests + flake check still pass. 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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds ExitCodePipelineFatal and typed container exit errors; documents Engine.Render contract; implements partial-response recovery and stderr preservation in both Docker and local render engines; introduces injectable Docker runner and comprehensive tests simulating success, pipeline-fatal partials, and hard failures. ChangesPipeline-fatal partial response recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Thank you — let me know if you want the hidden stack split into smaller layers or an added diagram for the local-engine helper flow. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (2)
cmd/crossplane/render/engine_docker_test.go (1)
42-59: ⚡ Quick winCould we tighten this table to assert the recovered response, not just its presence?
Thanks for adding the fatal-path coverage here. Right now
FatalWithPartialOutputonly provesrsp != nil, so a regression that returns an emptyRenderResponseinstead of the unmarshaled partial output would still pass. While touching this, could we also align the test with the repo’s_test.goconvention (TestDockerRenderEngineRender,args/want/reason, andcmp.Diffwithcmpopts.EquateErrors()for error assertions) so future cases stay consistent? As per coding guidelines,**/*_test.go: enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, usecmp.Diffwithcmpopts.EquateErrors()for error testing, and check for proper test case naming and reason fields.Also applies to: 125-167
🤖 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/render/engine_docker_test.go` around lines 42 - 59, Rename the test from TestDockerRenderEngine_Render to TestDockerRenderEngineRender and refactor its table to use the args/want/reason pattern; update cases (and the related block at lines ~125-167) so FatalWithPartialOutput asserts the actual unmarshaled renderv1alpha1.RenderResponse equals the expected Composite output (not just non-nil) by unmarshaling rspBytes and comparing to the want struct, use cmp.Diff for comparing responses and cmpopts.EquateErrors() for error comparisons, and keep unique symbols: TestDockerRenderEngineRender, FatalWithPartialOutput, DockerRenderEngine.Render (or runFn), and renderv1alpha1.RenderResponse/CompositeOutput to locate and change the checks.Source: Coding guidelines
cmd/crossplane/render/engine_local_test.go (1)
115-188: ⚡ Quick winTest coverage looks solid.
The four cases exercise all behavioral branches, and the helper-process pattern provides deterministic, cross-platform testing. Nice work!
One optional enhancement: the docker engine test (in
engine_docker_test.go) includes awantSingleOccurrencefield to assert that stderr appears exactly once in error messages, catching double-inclusion bugs. While your current implementation doesn't have this issue, adding the same check here would make the test suite more robust and consistent across both engines.Optional: Add single-occurrence checking
You could add a
wantSingleOccurrencefield to the test cases and check it similarly to the docker test:cases := map[string]struct { mode string wantRsp bool wantErr bool wantInErr []string + wantSingleOccurrence []string }{ // ... existing cases ... "FatalWithPartialOutput": { mode: "fatal-with-partial", wantRsp: true, wantErr: true, wantInErr: []string{ "pipeline returned fatal", "boom: pipeline step requested fatal", }, + wantSingleOccurrence: []string{"boom: pipeline step requested fatal"}, },Then add the check in the test loop:
for _, want := range tc.wantInErr { // ... existing check ... } + + for _, want := range tc.wantSingleOccurrence { + if err != nil && strings.Count(err.Error(), want) != 1 { + t.Errorf("Render(): error should contain %q exactly once, got %d occurrences", want, strings.Count(err.Error(), want)) + } + }🤖 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/render/engine_local_test.go` around lines 115 - 188, Add an optional wantSingleOccurrence test field to TestLocalRenderEngine_Render and assert that any expected stderr snippet appears exactly once in the error string to mirror engine_docker_test.go; update the test case structs in TestLocalRenderEngine_Render to include wantSingleOccurrence (bool) and in the per-case assertion after verifying err contains the substring, use strings.Count(err.Error(), <want>) == 1 when tc.wantSingleOccurrence is true to fail if the snippet appears zero or multiple times; this touches the TestLocalRenderEngine_Render function and the localRenderEngine test cases (refer to envHelperMode and the existing wantInErr logic) so the docker test’s single-occurrence protection is applied here too.
🤖 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/render/engine_docker_test.go`:
- Around line 42-59: Rename the test from TestDockerRenderEngine_Render to
TestDockerRenderEngineRender and refactor its table to use the args/want/reason
pattern; update cases (and the related block at lines ~125-167) so
FatalWithPartialOutput asserts the actual unmarshaled
renderv1alpha1.RenderResponse equals the expected Composite output (not just
non-nil) by unmarshaling rspBytes and comparing to the want struct, use cmp.Diff
for comparing responses and cmpopts.EquateErrors() for error comparisons, and
keep unique symbols: TestDockerRenderEngineRender, FatalWithPartialOutput,
DockerRenderEngine.Render (or runFn), and
renderv1alpha1.RenderResponse/CompositeOutput to locate and change the checks.
In `@cmd/crossplane/render/engine_local_test.go`:
- Around line 115-188: Add an optional wantSingleOccurrence test field to
TestLocalRenderEngine_Render and assert that any expected stderr snippet appears
exactly once in the error string to mirror engine_docker_test.go; update the
test case structs in TestLocalRenderEngine_Render to include
wantSingleOccurrence (bool) and in the per-case assertion after verifying err
contains the substring, use strings.Count(err.Error(), <want>) == 1 when
tc.wantSingleOccurrence is true to fail if the snippet appears zero or multiple
times; this touches the TestLocalRenderEngine_Render function and the
localRenderEngine test cases (refer to envHelperMode and the existing wantInErr
logic) so the docker test’s single-occurrence protection is applied here too.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a09384f5-364b-4faf-bb8b-65a40b3cffbe
📒 Files selected for processing (7)
cmd/crossplane/render/engine.gocmd/crossplane/render/engine_docker.gocmd/crossplane/render/engine_docker_test.gocmd/crossplane/render/engine_local.gocmd/crossplane/render/engine_local_test.gocmd/crossplane/render/render.gointernal/docker/docker.go
…bbit Both engine_docker_test.go and engine_local_test.go now follow the existing convention used by runtime_docker_test.go and the rest of cmd/crossplane/render: PascalCase test names without underscore (TestDockerRenderEngineRender, TestLocalRenderEngineRender), an args/want/reason struct shape, and cmp.Diff for response comparisons (via protocmp.Transform). While restructuring, also tighten the success / partial-output cases to compare the actual recovered *RenderResponse against the canned input rather than just asserting non-nil — a regression that returned an empty response would have slipped past the previous assertion. The canned response now carries a distinguishing CompositeResource struct so the round-trip exercises the unmarshal path. For symmetry, propagate wantSingleOccurrence to the local engine test. The local engine can't produce double-stderr today (its *exec.ExitError doesn't embed stderr the way *ContainerExitError does), but adding the assertion keeps the two test files structurally identical and guards against future drift. No production code changes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
adamwg
reviewed
Jun 9, 2026
adamwg
left a comment
adamwg
left a comment
Member
There was a problem hiding this comment.
This lgtm overall - thanks for picking it up. I've left a few small readability/simplification suggestions.
Comment on lines
+37
to
+40
| // containerRunner is the subset of internal/docker the engine depends on. It | ||
| // follows the same one-method-interface mock pattern as pullClient in | ||
| // runtime_docker.go, letting tests substitute a mockContainerRunner without a | ||
| // real Docker daemon. |
Member
There was a problem hiding this comment.
Nit: I would keep just the first sentence of this comment :-). The rest is likely to go stale.
Comment on lines
+169
to
+177
| // On a *ContainerExitError, err.Error() already embeds stderr | ||
| // (ContainerExitError.Error stringifies it as "container exited with | ||
| // status N: <stderr>"), so wrapping err is sufficient. For non-exit | ||
| // failures (e.g. image pull errors), append the captured stderr | ||
| // buffer so its content isn't lost when err.Error() doesn't include | ||
| // it. | ||
| if errors.As(err, &exitErr) { | ||
| return nil, errors.Wrap(err, "cannot run crossplane internal render in Docker") | ||
| } |
Member
There was a problem hiding this comment.
Could we handle this case in the block above, so we're not checking the same errors.As condition twice?
Comment on lines
178
to
181
| if len(stderr) > 0 { | ||
| return nil, errors.Errorf("cannot run crossplane internal render in Docker: %s: %s", err.Error(), stderr) | ||
| } | ||
| return nil, errors.Wrap(err, "cannot run crossplane internal render in Docker") |
Member
There was a problem hiding this comment.
For readability, I think we could collapse this into a single errors.Wrapf, which would also make it explicit if there's no stderr output:
Suggested change
| if len(stderr) > 0 { | |
| return nil, errors.Errorf("cannot run crossplane internal render in Docker: %s: %s", err.Error(), stderr) | |
| } | |
| return nil, errors.Wrap(err, "cannot run crossplane internal render in Docker") | |
| return nil, errors.Wrapf(err, "crossplane internal render returned error with output: %s", stderr) |
Comment on lines
82
to
85
| if stderr.Len() > 0 { | ||
| return nil, errors.Errorf("cannot run crossplane internal render: %s: %s", err.Error(), stderr.String()) | ||
| } | ||
| return nil, errors.Wrap(err, "cannot run crossplane internal render") |
Member
There was a problem hiding this comment.
Same as above, I'd do a single Wrapf here.
| // The cost is a few extra lines of TestMain plumbing; the benefit is a | ||
| // fully self-contained, deterministic, cross-platform fake binary with | ||
| // access to the same proto types the tests assert on. | ||
| const envHelperMode = "GO_HELPER_LOCAL_RENDER_MODE" |
Member
There was a problem hiding this comment.
Certainly a little complex, but the comment explains it clearly and I don't see an obviously better solution 👍
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 9, 2026
… wrapper crossplane/cli#91 (in flight) lands stderr capture and exit-code-3 partial-output handling in both the local and docker render engines. That makes our stderrCapturingLocalEngine wrapper redundant — both upstream engines now capture stderr and return (rsp, err) pairs on exit 3, which is the contract our EngineRenderFn.Render already expects. This commit: - Pins github.com/crossplane/cli/v2 via a `replace` directive at the upstream PR's branch SHA (jcogilvie/cli@f9700864). Will swap to a real upstream version once #91 merges. - Deletes stderrCapturingLocalEngine (and its stand-alone Setup / Render / CheckContextSupport plumbing) entirely. NewEngineRenderFn now constructs the engine via render.NewEngineFromFlags with EngineFlags.CrossplaneBinary set from the threaded binaryPath — no conditional, no wrapper, just upstream. - Drops the now-unused bytes / os/exec / proto / renderv1alpha1 imports (~80 lines of code gone). EngineRenderFn.Render's caller-level handling (the rsp != nil && err != nil → "partial output after pipeline fatal" branch) is unchanged — it was already engine-agnostic and matches the upstream contract verbatim. The WithCrossplaneRenderBinary option, the hidden --crossplane-render-binary CLI flag, and requireCrossplaneBinary test helper all stay: they're how integration tests thread a path into EngineFlags.CrossplaneBinary without an env var. Their purpose is now "expose upstream's first-class flag through our config" rather than "drive our own custom wrapper." Verified: full unit tests + integration smoke (XR + comp) green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
- engine_docker.go: shorten containerRunner doc to a single sentence.
The pattern-justification ("follows pullClient...") is rationale
that can drift if pullClient ever changes, and isn't load-bearing
for understanding the type.
- internal/docker/docker.go: drop stderr from ContainerExitError.Error().
Error() now returns just "container exited with status N"; callers
that want the stderr text wrap with errors.Wrapf using the .Stderr
field directly. This eliminates the doubled-stderr risk that
previously required defensive errors.As branching at every call site.
- engine_docker.go + engine_local.go: collapse the post-Run hard-fail
branches to a single errors.Wrapf("...returned error with output:
%s", stderr) per adamwg's suggestion. Previously each engine had
multiple branches (errors.As for *ContainerExitError, len(stderr)>0
conditional, plain Wrap fallback). With stderr no longer in
ContainerExitError.Error(), the uniform Wrapf produces clean output
in all four shapes:
*ContainerExitError + stderr → "...with output: <stderr>: container exited with status N"
*ContainerExitError + empty → "...with output: : container exited with status N"
*exec.ExitError + stderr → "...with output: <stderr>: exit status N"
non-exit + stderr → "...with output: <stderr>: <wrapped err>"
- engine_*_test.go: update assertions for the new error format. The
wantSingleOccurrence guards still work and confirm stderr appears
exactly once across all four shapes.
No production behaviour change beyond the error message wording.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 9, 2026
Track the latest revision of the upstream PR (crossplane/cli#91), which addresses adamwg's review feedback: - ContainerExitError.Error() no longer embeds stderr (kept in .Stderr field for callers). - Both engines collapsed their post-Run error tails to a single errors.Wrapf("...returned error with output: %s", stderr). - Cosmetic shortening of the containerRunner doc comment. No diff-side change required — our caller-level handling in EngineRenderFn.Render is engine-agnostic (rsp != nil + err != nil => partial output) and the new upstream behaviour matches that contract unchanged. Verified: full unit + integration tests still pass against the bumped replace. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
|
Backport failed because this pull request contains merge commits. You can either backport this pull request manually, or configure the action to skip merge commits. |
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 9, 2026
crossplane/cli#91 merged as dc7a1fa on origin/main. Drop the temporary `replace` directive that pointed at the user-fork branch and pin github.com/crossplane/cli/v2 to the upstream pseudo-version v2.4.0-rc.0.0.20260609191853-dc7a1fa2788a. No code changes — the upstream contract is identical to what we were already testing against on the fork branch; the bump just shifts the source of truth back to crossplane/cli. Verified: full unit + integration tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
adamwg
pushed a commit
that referenced
this pull request
Jun 9, 2026
- engine.go: document the two-non-nil (rsp, err) return contract on the Engine interface itself. Previously only the concrete implementations carried the doc — anything consuming the interface (or the mock in engine_mock.go) would assume the standard Go "nil-rsp on err" shape. - engine_docker.go: fix double-stderr in the hard-fail path. *docker.ContainerExitError.Error() already embeds stderr as "container exited with status N: <stderr>"; the previous code unconditionally appended the captured stderr buffer on top of that, producing messages like "...status 1: the container is sad: the container is sad". Now we wrap on *ContainerExitError (whose Error already includes stderr) and only append stderr explicitly for non-exit errors (e.g. image pull failures) where err.Error() doesn't. - engine_local.go: trim trailing ": " when stderr is empty. Cosmetic but produced messages like "cannot run crossplane internal render: exit status 1: " on the no-stderr path. - engine_docker_test.go: add wantSingleOccurrence assertion to catch double-formatting bugs at test time. Both FatalWithNoPartialOutput and HardFailWithExitError now assert their stderr text appears exactly once in the returned error. Would have flagged the double-stderr bug above before review. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> (cherry picked from commit 32e497a)
adamwg
pushed a commit
that referenced
this pull request
Jun 9, 2026
- engine_docker.go: shorten containerRunner doc to a single sentence.
The pattern-justification ("follows pullClient...") is rationale
that can drift if pullClient ever changes, and isn't load-bearing
for understanding the type.
- internal/docker/docker.go: drop stderr from ContainerExitError.Error().
Error() now returns just "container exited with status N"; callers
that want the stderr text wrap with errors.Wrapf using the .Stderr
field directly. This eliminates the doubled-stderr risk that
previously required defensive errors.As branching at every call site.
- engine_docker.go + engine_local.go: collapse the post-Run hard-fail
branches to a single errors.Wrapf("...returned error with output:
%s", stderr) per adamwg's suggestion. Previously each engine had
multiple branches (errors.As for *ContainerExitError, len(stderr)>0
conditional, plain Wrap fallback). With stderr no longer in
ContainerExitError.Error(), the uniform Wrapf produces clean output
in all four shapes:
*ContainerExitError + stderr → "...with output: <stderr>: container exited with status N"
*ContainerExitError + empty → "...with output: : container exited with status N"
*exec.ExitError + stderr → "...with output: <stderr>: exit status N"
non-exit + stderr → "...with output: <stderr>: <wrapped err>"
- engine_*_test.go: update assertions for the new error format. The
wantSingleOccurrence guards still work and confirm stderr appears
exactly once across all four shapes.
No production behaviour change beyond the error message wording.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
(cherry picked from commit ac63a4f)
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 9, 2026
… wrapper crossplane/cli#91 (in flight) lands stderr capture and exit-code-3 partial-output handling in both the local and docker render engines. That makes our stderrCapturingLocalEngine wrapper redundant — both upstream engines now capture stderr and return (rsp, err) pairs on exit 3, which is the contract our EngineRenderFn.Render already expects. This commit: - Pins github.com/crossplane/cli/v2 via a `replace` directive at the upstream PR's branch SHA (jcogilvie/cli@f9700864). Will swap to a real upstream version once #91 merges. - Deletes stderrCapturingLocalEngine (and its stand-alone Setup / Render / CheckContextSupport plumbing) entirely. NewEngineRenderFn now constructs the engine via render.NewEngineFromFlags with EngineFlags.CrossplaneBinary set from the threaded binaryPath — no conditional, no wrapper, just upstream. - Drops the now-unused bytes / os/exec / proto / renderv1alpha1 imports (~80 lines of code gone). EngineRenderFn.Render's caller-level handling (the rsp != nil && err != nil → "partial output after pipeline fatal" branch) is unchanged — it was already engine-agnostic and matches the upstream contract verbatim. The WithCrossplaneRenderBinary option, the hidden --crossplane-render-binary CLI flag, and requireCrossplaneBinary test helper all stay: they're how integration tests thread a path into EngineFlags.CrossplaneBinary without an env var. Their purpose is now "expose upstream's first-class flag through our config" rather than "drive our own custom wrapper." Verified: full unit tests + integration smoke (XR + comp) green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 9, 2026
Track the latest revision of the upstream PR (crossplane/cli#91), which addresses adamwg's review feedback: - ContainerExitError.Error() no longer embeds stderr (kept in .Stderr field for callers). - Both engines collapsed their post-Run error tails to a single errors.Wrapf("...returned error with output: %s", stderr). - Cosmetic shortening of the containerRunner doc comment. No diff-side change required — our caller-level handling in EngineRenderFn.Render is engine-agnostic (rsp != nil + err != nil => partial output) and the new upstream behaviour matches that contract unchanged. Verified: full unit + integration tests still pass against the bumped replace. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 9, 2026
crossplane/cli#91 merged as dc7a1fa on origin/main. Drop the temporary `replace` directive that pointed at the user-fork branch and pin github.com/crossplane/cli/v2 to the upstream pseudo-version v2.4.0-rc.0.0.20260609191853-dc7a1fa2788a. No code changes — the upstream contract is identical to what we were already testing against on the fork branch; the bump just shifts the source of truth back to crossplane/cli. Verified: full unit + integration tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
The
crossplane internal renderengines (localRenderEngineinengine_local.goanddockerRenderEngineinengine_docker.go) currently drop two kinds of information that callers and tooling need:stderr is not surfaced in returned errors.
engine_local.gosetscmd.Stderr = os.Stderr, so any FATAL message printed by the binary is visible on the terminal but never appears in the returned Go error.engine_docker.godiscardsRunContainer's stderr return value at the engine layer (stdout, _, err := docker.RunContainer(...)); only on container exit-non-zero does stderr appear, folded into an opaquefmt.Errorfstring.The pipeline-fatal exit-code-3 contract is not honoured.
fix(render): return requirements even on fatal errors crossplane#7455 made
crossplane internal renderexit with code 3 (instead of 1) when a pipeline step returnsSEVERITY_FATAL, with stdout populated by a partialRenderResponseso callers can recoveroutput.RequiredResourcesand iterate. Both engines currently wrap the resulting*exec.ExitError/ opaque container error and discard stdout, dropping the partial response on the floor.Changes
cmd/crossplane/render/render.go: addExitCodePipelineFatal = 3, with a doc comment pointing at fix(render): return requirements even on fatal errors crossplane#7455 so the contract is discoverable.internal/docker/docker.go: introduce*ContainerExitError(carryingExitCodeand captured stderr).RunContainernow returns it on non-zero exit instead offmt.Errorf, so callers canerrors.Asagainst it and branch on the exit code.cmd/crossplane/render/engine_local.go: capture stderr into abytes.Bufferinstead of piping toos.Stderr. On exit code 3 with non-empty stdout, parse the partial*RenderResponseand return(rsp, err)— both non-nil — so callers can recover the partial output. On other failures, surface stderr in the returned error.cmd/crossplane/render/engine_docker.go: read the stderr return fromRunContainerand use the new*ContainerExitErrorto detect the pipeline-fatal exit code, with the same(rsp, err)recovery shape on exit 3. Adds an internalrunContainerfield defaulting todocker.RunContainerso unit tests can inject a fake without standing up real Docker.Tests added
cmd/crossplane/render/engine_local_test.go: helper-process pattern (re-execs the test binary as the "binary" viaTestMain). Cases: success, fatal-with-partial-output, fatal-no-partial-output, hard-fail with stderr.cmd/crossplane/render/engine_docker_test.go: injects a fakerunContainer. Cases: success, fatal-with-partial-output, fatal-no-partial-output, hard-fail with*ContainerExitError, hard-fail with non-exit error (e.g. image pull failure).Motivating consumer
crossplane-contrib/crossplane-diffruns the engine programmatically and needs to (a) surface FATAL stderr to users via its own error reporting and (b) iterate onRequiredResourceseven after a pipeline FATAL — the contract crossplane/crossplane#7455 was designed to enable. It currently ships its own wrapper around the local engine to add both behaviours; once this lands the wrapper goes away. Tracking PR: crossplane-contrib/crossplane-diff#326.Fixes #
I have:
./nix.sh flake checkto ensure this PR is ready for review.Linked a PR or a docs tracking issue to document this change.(internal engine behaviour; no user-facing docs change required)backport release-x.ylabels to auto-backport this PR.🤖 Generated with Claude Code