refactor(render): unwind multi-composition workaround using cli#159#359
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes the prior multi-composition “captured Docker network” workaround in EngineRenderFn by relying on the now-idempotent render.Engine.Setup behavior from crossplane/cli (cli#159), and updates dependencies accordingly.
Changes:
- Call
engine.Setupon every render and accumulate returned cleanup funcs, executing them in LIFO order duringCleanup. - Remove the previous first-vs-subsequent render network bookkeeping and related helpers in
render_engine.go. - Inline Docker network annotation stamping for the
CROSSPLANE_DIFF_DOCKER_NETWORKenv-var path and bumpgithub.com/crossplane/cli/v2to the referenced pseudo-version (plus indirect dep updates).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Pins github.com/crossplane/cli/v2 to a pseudo-version containing the idempotent Setup change; updates related indirect deps. |
| go.sum | Updates checksums for the new cli pseudo-version and transitive dependency bumps. |
| cmd/diff/diffprocessor/render_engine.go | Refactors EngineRenderFn to call Setup every render and run accumulated cleanups in reverse order; removes the prior workaround state/helpers. |
| cmd/diff/diffprocessor/function_provider.go | Inlines Docker network annotation stamping using render.AnnotationKeyRuntimeDockerNetwork for the env-var path. |
| cmd/diff/diffprocessor/function_provider_test.go | Import ordering cleanup to reflect the new dependency usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crossplane/cli#159 makes dockerRenderEngine.Setup idempotent in the network-already-set branch by calling injectNetworkAnnotation there too. That removes the need for EngineRenderFn to capture the docker network name off the first batch's annotations and re-stamp it on later batches itself. EngineRenderFn now calls engine.Setup(ctx, newFns) on every render and accumulates the returned cleanups in a slice that Cleanup walks in LIFO order. The first cleanup is the real one; the rest are no-ops, so the order keeps network teardown last without any first-vs-subsequent bookkeeping. The applyNetworkAnnotation helper was also used by function_provider's CROSSPLANE_DIFF_DOCKER_NETWORK env-var path. That's an independent use case (pre-stamping fns so dockerRenderEngine.Setup's don't-overwrite clause picks up the user's network), so the 6-line loop is inlined into applyDockerNetworkAnnotation, its only remaining caller. The go.mod replace directive points at the PR #159 head while it's in review. Drop it once that PR merges and a crossplane/cli release ships with the fix. Fixes #338 Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
…li#159 Upstream PR crossplane/cli#159 — the idempotent-Setup fix this branch relies on — merged to crossplane/cli main at 86f5f7a0. Drop the fork replace directive and pin directly to that commit via the v2.5.0-rc.0.0.20260626181113-86f5f7a0c4bf pseudo-version so this PR can land without waiting for a tagged cli release. The transitive bumps in go.sum (go-openapi/swag/* 0.25.5 → 0.26.0, docker/cli 29.5.3 → 29.6.0, klog/v2 2.130.1 → 2.140.0) come from upstream cli main's newer indirect deps; nothing in crossplane-diff touches those modules directly. Once a tagged cli release containing cli#159 ships (likely v2.4.1 or v2.5.0), a follow-up should bump the pseudo-version to that tag. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
- render_engine.go: rewrite Cleanup's reverse loop as slices.Backward (Go 1.23+ idiom, suggested by intrange/copyloopvar lint). - function_provider_test.go: regroup the render import added in the workaround unwind alongside the other third-party imports. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
jcogilvie
force-pushed
the
unwind-workaround
branch
from
17a3a40 to
88e3122
Compare
…eFlags Now that we no longer manage the docker network ourselves, the CROSSPLANE_DIFF_DOCKER_NETWORK env var doesn't need its own stamping pass over fetched functions. NewEngineRenderFn reads it once and threads the value through render.EngineFlags.CrossplaneDockerNetwork (crossplane/cli#65). The docker engine then: - runs the crossplane-render container on that network (which fixes the other half of crossplane/cli#75 — our prior implementation only placed function containers on the network, leaving the render container unreachable from inside-a-container deployments); - annotates each fn at Setup time so its container joins it too, honouring the don't-overwrite contract for fns the caller has already pinned. What's deleted: - applyDockerNetworkAnnotation helper. - The DefaultFunctionProvider/CachedFunctionProvider calls into it. - The CachedFunctionProvider cache-hit re-stamp dance and its accompanying TestCachedFunctionProvider_DockerNetworkAnnotation_CacheHit test (cache entries no longer need to track env state). - TestApplyDockerNetworkAnnotation (the helper's gone; engine.Setup owns this behaviour now and has its own coverage upstream). Also tightens the EngineRenderFn / Render docstrings and the startRuntimes-error rollback comment to match the merged upstream Engine.Setup contract (cli#159 + follow-up commit ad8307749). The new phrasing acknowledges that no Setup call may ever return a real cleanup (--crossplane-docker-network preset, or local engine), and frames the rollback as "if this call created the environment" rather than "if this was the very first call". Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
| // Roll back this Setup. If this call created the environment, | ||
| // cleanup releases it; otherwise cleanup is a no-op and no harm | ||
| // is done. | ||
| cleanup() |
Collaborator
Author
There was a problem hiding this comment.
The upstream Engine.Setup docstring guarantees the returned cleanup is callable: "callers can safely defer every returned cleanup in LIFO order without coordinating which one is real." A nil cleanup would panic under defer, so the contract excludes it. All three upstream implementations (docker, local, MockEngine) return func() {} for the no-op case rather than nil. Adding a nil guard here would be validation for a scenario the contract forbids, so leaving it as-is.
| maps.Copy(e.addrs, fa.Addresses()) | ||
| } | ||
|
|
||
| e.cleanups = append(e.cleanups, cleanup) |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed. Setup (and the cleanups append) now only runs when len(newFns) > 0, so renders where all fns are already started don't accumulate no-op cleanups or make a wasted engine call. Coming in the next push.
…tegrate Per Copilot review feedback on PR #359: we were calling engine.Setup and appending its returned cleanup to e.cleanups on every Render, even when newFns was empty (all fns already started by a prior render). The slice would grow without bound across long-running engines and the Setup call itself was wasted work. Gate the whole Setup+startRuntimes+bookkeeping block on len(newFns) > 0. Renders where every fn is already running now skip straight to building the render request, leaving e.cleanups bounded by the number of distinct fn batches the engine has actually integrated. No behavioural change for the renders that DO integrate new fns: Setup, startRuntimes, the rollback-on-startRuntimes-error path, and the LIFO cleanup ordering all work the same. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.com>
Comment on lines
+133
to
+135
| // Render performs one render. It is safe for concurrent use — calls are | ||
| // serialized internally. Setup runs on the first invocation (creating the | ||
| // docker network); subsequent invocations annotate any newly-encountered | ||
| // functions with the captured network name and start their runtimes. | ||
| // serialized internally. Setup runs on every invocation; upstream's Engine | ||
| // contract makes that safe across batches (see the EngineRenderFn docstring). |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 544f451. Render's doc now reads "Setup runs only on invocations that introduce previously-unseen functions; renders whose fns are all already running skip straight to building the request."
Comment on lines
+327
to
+331
| // Run cleanups in LIFO order. Per the upstream Engine.Setup contract, | ||
| // at most one cleanup is real (the one that created the environment, | ||
| // if any); the rest are no-ops. LIFO matches the natural deferred- | ||
| // cleanup shape and is correct even if a future engine wants ordering. | ||
| for _, v := range slices.Backward(e.cleanups) { |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 544f451. Cleanup's doc now reads "runs the cleanups accumulated from each engine.Setup call in LIFO order. The effect of those cleanups is engine-specific — for the docker engine the one real cleanup releases the docker network it created; other engines (local, or docker pre-configured with an externally-managed network) accumulate only no-op cleanups."
Two Copilot review comments on PR #359, both valid drift introduced by the prior commit that gated Setup on len(newFns) > 0: - Render's doc said "Setup runs on every invocation" — it doesn't anymore. Rewrite to "Setup runs only on invocations that introduce previously-unseen functions; renders whose fns are all already running skip straight to building the request." - Cleanup's doc said it "releases the docker network" — that's only one possible effect now. The cleanups slice is engine-agnostic, so rewrite to "runs the cleanups accumulated from each engine.Setup call in LIFO order. The effect of those cleanups is engine-specific — docker creates and releases a network; local and pre-configured docker accumulate only no-op cleanups." No code change. Signed-off-by: Jonathan Ogilvie <jonathan.ogilvie@sumologic.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
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
Unwinds the captured-network workaround in
EngineRenderFnintroduced by #326 and, while we're here, drops the parallel docker-network plumbing we no longer need. Tracked in #338.What changed in
EngineRenderFnengine.Setupis now called on every render. Upstream made it idempotent in crossplane/cli#159 (merged) — at most one Setup call returns a real cleanup, and only when that call actually creates a new environment. Calls that integrate fns into an environment that already exists (because a prior Setup established it, or because the engine was pre-configured with--crossplane-docker-network), or calls on engines with nothing to clean up (the local engine), return a no-op cleanup.EngineRenderFnaccumulates every returned cleanup in a slice thatCleanupwalks in LIFO order — without coordinating which one is real, per the upstreamEngine.Setupdocstring contract.networkName,started,networkCleanupfields;firstNetworkAnnotationandapplyNetworkAnnotationhelpers; the multi-composition workaround docstring; the first-vs-subsequent switch inRender.Render'sstartRuntimeserror path now rolls back unconditionally viacleanup()— if this call created the environment, that releases it; otherwise it's a no-op.Bonus: route
CROSSPLANE_DIFF_DOCKER_NETWORKthroughEngineFlags.CrossplaneDockerNetworkWith the workaround gone, we don't need to manage the docker network ourselves at all — the env var path that pre-stamps every fetched fn was symmetrically obsolete.
NewEngineRenderFnnow readsCROSSPLANE_DIFF_DOCKER_NETWORKonce and threads it throughrender.EngineFlags.CrossplaneDockerNetwork(added in crossplane/cli#65). The docker engine then:Deletions that fall out:
applyDockerNetworkAnnotationhelper.DefaultFunctionProvider/CachedFunctionProvidercalls into it.CachedFunctionProvidercache-hit re-stamp dance — cache entries no longer need to track env-var state.TestApplyDockerNetworkAnnotationandTestCachedFunctionProvider_DockerNetworkAnnotation_CacheHit. Coverage moves upstream into the cli'sdockerRenderEngine.Setuptests.Dependency pin
go.modpinsgithub.com/crossplane/cli/v2to a pseudo-version of the merge commit of cli#159 oncrossplane/climain:No
replacedirective. Once a tagged cli release containing cli#159 ships (likely v2.4.1 or v2.5.0), a follow-up should bump this to the tag.The transitive bumps in
go.sum(go-openapi/swag/*0.25.5 → 0.26.0,docker/cli29.5.3 → 29.6.0,klog/v22.130.1 → 2.140.0) come from upstream cli main's newer indirects; nothing incrossplane-difftouches those modules directly.Coverage
TestEngineRenderFn_*suite —MultiCompositionFunctionSet,PreservesExistingNetworkAnnotation,CleanupStopsAllFunctionAddresses, plusHappyPath,CleanupIdempotent,Serialization— asserts caller-visible behavior, not the workaround internals, so it covers the new implementation without changes.MockEngine.Setupalready tolerated multi-call.TestCachedFunctionProvider_*(lazy-loading, cache-hit, error, multiple-compositions, cleanup, container-name tracking) cover the cached provider's remaining responsibilities.Fixes #338
I have:
earthly -P +reviewableto ensure this PR is ready for review.Added or updated e2e tests.Refactor with no caller-visible behavior change; existing e2e coverage applies.Documented this change as needed.No user-facing surface changes.Followed the API promotion workflow if this PR introduces, removes, or promotes an API.No API surface changes.Need help with this checklist? See the cheat sheet.