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>
Contributor
|
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}
Contributor
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 |
Contributor
✅ 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
jcogilvie
added a commit
to crossplane-contrib/crossplane-diff
that referenced
this pull request
Jun 10, 2026
…#326) * feat(render): switch to crossplane internal render via crossplane/cli Replace the in-process `render.Render()` call (Crossplane v2.2-era) with `crossplane internal render` driven through `github.com/crossplane/cli/v2`'s Engine interface. The binary runs the real composite reconciler, so crossplane-diff now produces output that matches what users would see if they reconciled the same XR in a live cluster. Dependency bump: - crossplane/v2, crossplane-runtime/v2, apis/v2 → v2.3.1 - crossplane/cli/v2 → main @ b88f8a1 (pinned via Go pseudo-version pending the next cli release; carries the `composite_resource_definition` proto field and `CompositionInputs.XRD` Go wire-up that the new render contract requires) - function-sdk-go promoted to a direct dep (replaces crossplane/v2/proto/fn/v1) - crank/* imports moved to crossplane/cli/v2/cmd/crossplane/* Render contract adaptations: - Pass the input XR's XRD through `CompositionInputs.XRD` so the render binary can select the right composite.Schema (Legacy vs Modern). New `DefinitionClient.GetCompositeSchema` reads `spec.scope` instead of apiVersion since v1 XRDs round-tripped through the apiserver come back as v2-form objects with scope=LegacyCluster preserved. - Honour the v2.4 partial-output-on-fatal contract: when a pipeline step FATALs, the binary now exits 3 with stdout populated, surfacing recorded RequiredResources. Our `stderrCapturingLocalEngine` unmarshals the partial output, and `RenderToStableState` keeps iterating on the resolved selectors until the pipeline stabilises. - New `EngineRenderFn` owns the render-engine lifecycle (Setup, function runtimes, Render, Cleanup), replacing `cmd/diff/serial`. Default rendering image flips from `:main` (frozen at xpkg.crossplane.io since the nix migration) to cli's `:stable`, which tracks current releases. Schema/fixture updates driven by the reconciler-faithful output: - Add `status.conditions[].observedGeneration` to ~22 test CRDs (the reconciler now writes it on every condition). - Add `spec.crossplane.*` subtree to nested-XR test fixture CRDs that previously declared only user fields. - Strip `spec.crossplane` from dry-run apply input only when needed (composed resources whose CRD doesn't declare the subtree); keep it for root XRs so revision upgrades surface in the diff. - Synthesize a backing XR for existing claims that lack `spec.resourceRef` so the reconciler's GVK compatibility check accepts the input. - Fix `NewXRWithGenerateName` to emit a placeholder name that passes RFC 1123 validation; display layer continues to render `(generated)`. Skipped tests, with TODOs: - `TestGetRestConfig/EmptyKubeconfigEnvVar`: pre-existing skip, now annotated with how to make it run on developer machines too. - `TestCompDiffIntegration/CrossNamespaceResourceCollision`: blocked on crossplane-contrib/function-extra-resources#106 — the function emits Selector{Namespace:""} for `Reference`-typed extras that omit `ref.namespace`, and the v2.3+ binary fetcher takes it verbatim. Re-enable once a function release defaults to the XR's namespace. Working notes for two upstream issues we drove this round live under .requirements/20260504T230404Z_render_engine_migration/, including the filed cli/proto change (crossplane/crossplane#7452, merged) and the filed function-extra-resources issue. Test result: 609 PASS / 0 FAIL / 2 SKIP. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * test(requirements): restore namespace-collision unit test The test was dropped in da2f192 when ProvideRequirements (step-keyed map) was rewritten as ResolveSelectors (flat selector slice). The underlying behavior — resolveNamespace defaulting empty selector namespace to xrNamespace, and namespace-aware cache keys — is still in the code, but nothing was asserting it. Pairs with the (currently skipped) E2E TestCompDiffIntegration/CrossNamespaceResourceCollision, which is blocked on function-extra-resources#106. The unit test exercises the same defaulting + cache-keying path without depending on the function, so regressions are caught even while the E2E remains skipped. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(render): thread crossplane render binary path explicitly, kill env-var race Previously, integration tests pointed the render engine at a local crossplane binary by mutating the process-global CROSSPLANE_RENDER_BINARY env var inside requireCrossplaneBinary. Both TestDiffIntegration and TestCompDiffIntegration call t.Parallel(), so whichever finished first would restore/unset the var while the other was still running, racing on engine selection. Replace with explicit path threading: - ProcessorConfig.CrossplaneRenderBinary + WithCrossplaneRenderBinary option. - NewEngineRenderFn now takes the path as a positional arg; empty string means "use the upstream docker engine" (production default). - New hidden CLI flag --crossplane-render-binary plumbs the path through defaultProcessorOptions so each kong invocation gets its own copy. - requireCrossplaneBinary now returns the absolute path; runIntegrationTest threads it into args as --crossplane-render-binary=<path>. No env mutation. The local-binary path itself remains a temporary test affordance — documented in render_engine.go as something we can delete once crossplane/cli's localRenderEngine adopts equivalent stderr-capture and exit-code-3 (PR #7455 partial-output-on-fatal) semantics. Upstream PR to follow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(review): address PR #326 review comments Mechanical fixes from the PR review pass: - diffprocessor/requirements_provider.go: drop the orphaned function-stub comments left over from ProvideRequirements, drop the unused `stepName` arg threaded through processSelector / processNameSelector / processLabelSelector / resolveNamespace (callers now always passed ""), and update the resourceCache doc to mention namespace (which has been part of the cache key since dt.MakeDiffKey took on namespace). - diffprocessor/resource_manager.go: dedupe owner refs via dt.MakeDiffKey instead of an ad-hoc "|"-joined string, matching the rest of the diff layer's keying convention. - diffprocessor/diff_processor.go: switch the err==nil/else block in RenderToStableState to a switch (project style), tighten the generateName comment to make clear "placeholder" is purely a render-side RFC1123-valid filler and the (generated) suffix the user sees is produced independently by diff_formatter.go, and update the schema re-stamp comment to point at crossplane/crossplane#7452 (merged on main; v2.3.1 still defaults the binary to SchemaModern). - diffprocessor/diff_processor_test.go: same #7452 update on the schema- plumbing test's documentation. - diffprocessor/render_engine_test.go: add a header comment justifying why the three lifecycle tests (HappyPath / CleanupIdempotent / Serialization) are procedural rather than table-driven, and switch to t.Context(). - diffprocessor/schema_validator.go: rewrite the spec.crossplane stripping comment so it accurately states that real cluster-derived CRDs declare the subtree (Crossplane's CRD generator emits it) — the gap is in our hand-rolled integration-test CRD fixtures, not in production. - client/crossplane/definition_client.go: bring the GetCompositeSchema doc comments (interface + impl) into sync with the actual implementation, which reads spec.scope rather than the XRD's apiVersion (apiserver conversion can rewrite the apiVersion stamp; spec.scope is preserved). - client/crossplane/definition_client_test.go: drop a duplicated comment line; clarify the "ModernClaim" reference (the claim kind in the test is just the fixture's "ClaimedResource" string — there's no upstream type called ModernClaim). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(render): use upstream engine directly, drop local stderr/exit-3 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> * chore(deps): bump crossplane/cli replace to ac63a4f 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> * chore(deps): pin crossplane/cli to merged main (drop fork replace) 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> * chore(deps): bump crossplane stack to v2.3.2 All four crossplane modules cut v2.3.2 today. Pin to those releases: - crossplane/cli/v2: dc7a1fa pseudo-version → v2.3.2 - crossplane/crossplane-runtime/v2: v2.4.0-rc.0 → v2.3.2 - crossplane/crossplane/apis/v2: v2.4.0-rc.0 → v2.3.2 - crossplane/crossplane/v2: v2.3.1 → v2.3.2 cli v2.3.2 includes our PR #91 backported via #95 (crossplane/cli@95), so the partial-output-on-fatal contract and stderr capture are part of a real release tag — no more pre-release pseudo-versions in our go.mod. Verified: full unit + integration tests pass against the pinned set. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(render): support multi-composition function sets across renders EngineRenderFn previously called engine.Setup + StartFunctionRuntimes exactly once on the first render and reused that single batch's addresses for every subsequent render. When a single `xr` invocation processed multiple XRs that resolved to different Compositions with overlapping-but-not-identical function pipelines (e.g. diffing a GitOps directory), the second composition's new functions never got upstream's runtime-docker-network annotation — their containers landed on the default Docker bridge and were unreachable from the render container. Self-contained workaround on the diff side (no upstream changes required against cli v2.3.2): - Track started functions in a startedNames set, dedup'd by fn name. - Accumulate every *FunctionAddresses returned by startRuntimes in fnAddrsList; merge their Addresses() maps into a single addrs map. - On the first render, capture upstream's auto-stamped network name off any first-batch function via render.AnnotationKeyRuntimeDockerNetwork. - On subsequent renders, manually apply that captured annotation to any new function (preserving any caller-set value), then call startRuntimes for new functions only. - BuildCompositeRequest receives FunctionAddrs filtered to exactly this render's function set. - Cleanup iterates fnAddrsList and stops every entry, then runs the network cleanup once. The workaround couples to internal state of dockerRenderEngine via an annotation side-channel. Tracked for unwind in #338 once a clean upstream API lands (crossplane/cli#96 — either idempotent Setup or a new Engine.AnnotateFunctions method). Tests: - Existing TestEngineRenderFn_HappyPath / CleanupIdempotent / Serialization continue to pass (single-comp regression coverage). minimalRenderInputs() now includes one Function so the "first call → start runtimes" assertion is exercised meaningfully. - New TestEngineRenderFn_MultiCompositionFunctionSet covers the staleness scenario: render with [F1, F2], then [F1, F3], then [F1, F2] again. Asserts startRuntimes is called exactly twice (F1+F2, then F3 only), every started fn carries the captured network annotation, and already-running fns are not restarted. - New TestEngineRenderFn_PreservesExistingNetworkAnnotation asserts caller-supplied runtime-docker-network values are not overwritten. - New TestEngineRenderFn_CleanupStopsAllFunctionAddresses asserts Cleanup invokes stopRuntimes for every *FunctionAddresses ever returned, not just the most recent. Includes the per-task REQUIREMENTS.md (.requirements/20260609T220505Z_multi_composition_render/) capturing the As-Is / To-Be / requirements / acceptance criteria / testing / implementation plan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(render): address CI failures and outstanding review feedback - diff_processor.go: extract resolveSchemaAndXRDForRender + resolveFunctionCredentials helpers from RenderToStableState (gocognit was 35 > 30 threshold). update v2.3.1 -> v2.3.2 in the schema-restamp comment now that crossplane/crossplane#7452 is shipped. - definition_client.go: //nolint:interfacebloat with reason. The 6 methods are cohesively about XRD lookup; splitting just to satisfy the linter would create surface without value. - render_engine.go: guard against (nil rsp, nil err) from a misbehaving Engine implementation; previously rsp.GetComposite() would have panicked. - schema_validator.go: rewrite the SchemaValidation defaults-propagation comment to accurately describe the shared-map mechanism (composed.Unstructured embeds unstructured.Unstructured; UnstructuredContent returns Object by reference; defaults DO propagate via the shared map). - render_engine_test.go: drop the structpb dummy import + var (no test referenced it). - diff_it_utils_test.go: rename requireCrossplaneBinary to localCrossplaneBinary; return path-or-empty instead of skipping. CI's go-test target doesn't build _output/bin/crossplane, so the previous skip silently turned IT into a no-op in CI. Now empty path falls through to the docker render engine (slower but real). Local devs can still build the binary for fast iteration; path is overridable via CROSSPLANE_DIFF_RENDER_BINARY env var. - diff_integration_test.go: only add --crossplane-render-binary when a path is present (empty would parse as 'use docker engine' anyway, but explicit absence reads cleaner). - renderer/types/types.go + renderer/diff_formatter.go + diff_processor.go: introduce shared GenerateNamePlaceholder constant ('xrgenplace0000') replacing the prior 'placeholder' string. Formatter now substitutes the sentinel back to '(generated)' for both the displayed resource name and the diff-body metadata, fixing TestDiffNewClaim/CanDiffNewClaim e2e (ANSI golden) and TestDiffIntegration/NewXRWithGenerateName. Extracted stripSyntheticName + normalizedGenerateName helpers to keep cleanupForDiff under the gocognit threshold. - main.go + diff_integration_test.go: lint --fix realignment of struct tags + wsl_v5 whitespace nudges. Verified: full unit + integration suite green; lint clean on touched packages (only pre-existing revive var-naming on cmd/diff/renderer/types remains). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * refactor(validator): drop spec.crossplane strip; fix the offending fixture instead stripCrossplaneManagedFields existed solely to paper over hand-rolled CRD test fixtures that didn't declare the spec.crossplane subtree the composite reconciler populates at runtime. Real cluster CRDs derived from XRDs declare it (Crossplane's CRD generator emits it), so the strip was production code coding around a test-data limitation. Investigation found exactly one fixture actually needed updating: testdata/{diff,comp}/crds/xdefaultresource-ns-crd.yaml — the kind=XTestDefaultResource v2 XR used by TestDiffIntegration/XRDDefaultsAppliedBeforeRendering. Every other v2 XR fixture either already declares spec.crossplane.* or is a v1 (legacy) fixture that puts compositionRef et al. at the top level of spec.properties (no spec.crossplane needed). Claim and managed-resource fixtures don't have spec.crossplane at all and aren't affected. Removed: - DefaultSchemaValidator.stripCrossplaneManagedFields helper - the call site in ValidateResources - the now-stale comment block referencing the strip from the SchemaValidation defaults-propagation explanation Added spec.crossplane.* (canonical Crossplane shape) to both copies of xdefaultresource-ns-crd.yaml. Full unit + integration suite remains green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(deps): go mod tidy after structpb dummy removal google.golang.org/protobuf has no direct usage in our code anymore (the dummy 'var _ = structpb.NewNullValue' was removed in 90fa8b8). 'earthly +generate' moved it to the indirect block accordingly. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * test(requirements): add MatchLabels coverage to ResolveSelectors Per Copilot review on PR #326: ResolveSelectors fans out through processSelector to either processNameSelector (MatchName -> client.GetResource) or processLabelSelector (MatchLabels -> client.GetResourcesByLabel), but the unit test table only exercised MatchName. Adds two cases covering the label path: - MatchLabels: tier=cache selector returns both labelled ConfigMaps via GetResourcesByLabel; verifies the resources flow through ResolveSelectors unchanged. - MatchLabelsFetchError: error path parallel to the existing FetchError case for MatchName, covering propagation from the label-list call site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(render): three Copilot follow-ups + e2e claim regression 1. requirements_provider.go: drop the dead RequirementsProvider.renderFn field. RequirementsProvider stored a renderFn but never used it after the switch from ProvideRequirements (which needed it for env-config FATAL recovery render passes) to ResolveSelectors (which doesn't). The field, the constructor parameter, the ComponentFactories.RequirementsProvider signature, the WithRequirementsProviderFactory option signature, and the call sites in diff_processor.go + tests all drop the renderFn arg. No behaviour change. 2. diff_processor.go + definition_client.go: fold the duplicate XRD lookup in resolveSchemaAndXRDForRender into a single call. GetCompositeSchema internally calls GetXRDForXR / GetXRDForClaim, then resolveSchemaAndXRDForRender called the same lookups again to forward the XRD to the render binary - two cache hits per render iteration. Extracts the spec.scope -> Schema rule into an exported xp.SchemaFromXRD helper that both GetCompositeSchema (when called directly) and resolveSchemaAndXRDForRender (which already has the XRD) can call. Net: one lookup per iteration. 3. renderer/diff_formatter.go: handle composed-resource names that crossplane's reconciler synthesized from a generateName template. Pre-migration, render.Render() left composed resources with generateName + no name. Post-migration, crossplane internal render's reconciler stamps a deterministic name (e.g. test-claim-b0348ce08462) on resources whose template only had generateName. The diff formatter's existing logic only recognised three name shapes - the legacy '(generated)' suffix, our GenerateNamePlaceholder sentinel, and 'no name + generateName set' - so it kept the generated name in the diff body and the test framework's masking turned it into XXXXX. Adds isGeneratedFromGenerateName(desired) which returns true when generateName is non-empty and name starts with generateName (i.e. the suffix is generated). Display path substitutes <generateName>(generated); strip path drops metadata.name and normalises generateName. Both treat the new shape identically to the existing two. Fixes TestDiffNewClaim/CanDiffNewClaim e2e regression. Updates TestDefaultDiffProcessor_RenderToStableState_SchemaPlumbing to mock GetXRDForXR (matching the new single-lookup path) instead of GetCompositeSchema. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(render): align observed owner-ref UIDs to binary's synthesized XR UID The binary at internal/render/composite/render.go:94 (v2.3.2) overwrites the input XR's UID with a deterministic SHA1 of gvk+namespace+name. The composite reconciler then drops any observed composed resource whose controller owner ref UID doesn't match xr.UID (composition_functions.go:824). Cluster-fetched observed resources carry the real XR UID, so they were silently filtered out and templates that look them up by composition-resource-name resolved to "<no value>" — surfacing as `cannot apply composed resource ... because it has an invalid name "<no value>"` in TestDiffCompositionWithGetComposedResource. Pre-rewrite controller owner refs in EngineRenderFn.Render to the binary's predictable fake UID. The formula is deterministic and stable; if upstream changes it, the same E2E will fail and we'll update in lockstep. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * test(e2e): refresh new-parent-xr fixture for v2 schema defaults The fixture pre-dated v2 schema-default propagation. New v2 XRs (scope: Namespaced) now render with spec.crossplane.compositionUpdatePolicy: Automatic — same surface that already shows for managed-resource defaults like deletionPolicy: Delete on NopResource. Regenerated via E2E_DUMP_EXPECTED=1. The Existing variant is unaffected because the cluster state already carries those fields populated by the apiserver. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * test(e2e): refresh remaining new-XR v2 fixtures for schema defaults Same root cause as fc1b43e: v2 XRs (scope: Cluster or Namespaced) now render with spec.crossplane.compositionUpdatePolicy: Automatic from the schema default, surfacing in the new-XR diff. Existing-XR variants are unaffected because cluster state already carries the populated field. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(deps): align k8s.io/* modules at v0.35.3 api/apimachinery had drifted to v0.35.3 via transitive bumps while client-go, apiextensions-apiserver, apiserver, cli-runtime, code-generator, component-base, kms, and kubectl stayed at v0.35.1. Bump all k8s.io/* to v0.35.3 so the dependency set stays in lockstep, matching how these modules are typically released and consumed together. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(review): address review feedback - diff_processor.go: drop dead schema re-stamp on output.CompositeResource; the binary already writes data at the right path post-#7452 and we never call schema-aware accessors on the output (only GetUnstructured()). - diff_processor.go: drop defensive defClient nil check in resolveSchemaAndXRDForRender; nil defClient is a programmer error and silent fallback masked it. - diff_processor_test.go: stamp a Definition mock on the two test sites that previously relied on the nil guard. - diff_processor_test.go: rewrite the SchemaPlumbing test comment to describe current behavior at HEAD instead of historical version pinning. - render_engine.go: convert the if/else if branch in Render to a switch. - render_engine_test.go: drop the pre-fix/post-fix framing on the multi-composition test; describe the required behavior plainly. - diff_formatter.go: drop dead "(generated)" legacy paths in stripSyntheticName / normalizedGenerateName (we never stamp that suffix into metadata.name now). Add a top-of-block doc explaining the generateName-display machinery. - diff_renderer.go: collapse getKindName's two-branch switch into one expression — both branches returned identical values. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(render): unify generated-name handling around upstream's shape Replace the ad-hoc "xrgenplace0000" sentinel with a 12-hex suffix derived deterministically from a fixed seed via the same hash truncation upstream's internal/names.ChildName uses. The synthesized XR name is now shape- compatible with the composed-resource names crossplane's nameGenerator emits ("<gen-with-dash><12 lowercase hex>") AND the suffix value itself is distinctive enough to detect when interpolated downstream. Collapse displayNameFromPlaceholder, isGeneratedFromGenerateName, and normalizedGenerateName into two helpers in renderer/types: - LooksLikeGeneratedName(name, generateName) — single detector covering both the shape-match path (binary-generated composed resources whose template carries a generateName) and the embedded-suffix path (resources whose template interpolated the synthesized XR name into their own). - GeneratedDisplayName(name, generateName) — single display formatter symmetric with the detector. Drop the dead "(generated)"-suffix legacy paths and the stale doc block. Net delete: ~30 LOC, two helpers fewer, one mental model. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * chore(render): use encoding/hex for hex validation Drop the hand-rolled rune scan in isLowerHex; hex.DecodeString does the character validation for us, and we keep the lowercase guard since EncodeToString emits lowercase only. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> * fix(render): drop genNameFound guard in stripSyntheticName The guard short-circuited before LooksLikeGeneratedName ran, which contradicted the function's contract: the embedded-suffix path (XR-synthesis suffix interpolated into a downstream resource's metadata.name via a composition template) typically has no generateName field on the rendered resource, and LooksLikeGeneratedName already handles that case via Contains-on-suffix. Without this fix, those synthetic names leaked into the diff body. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> --------- Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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