Backport #91 to release-2.3#95
Merged
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> (cherry picked from commit 1843d3d)
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> (cherry picked from commit b196c89)
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> (cherry picked from commit 40354bd)
- 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)
…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> (cherry picked from commit f970086)
- 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)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
jcogilvie
approved these changes
Jun 9, 2026
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 auto-backport failed due to a merge commit in the history. The conflict resolved in the merge commit was with #83, which was not backported to release-2.3, so skipping it here is fine.
I have:
./nix.sh flake checkto ensure this PR is ready for review.- [ ] Added or updated unit tests.- [ ] Linked a PR or a docs tracking issue to document this change.- [ ] Addedbackport release-x.ylabels to auto-backport this PR.