fix(render): annotate functions when reusing an existing network

[jcogilvie]

Description of your changes

dockerRenderEngine.Setup short-circuits when e.network is already set — either pre-configured via --crossplane-docker-network or set by a prior Setup call on the same engine. Until now that short-circuit skipped injectNetworkAnnotation, leaving the supplied fns un-annotated and their containers unreachable from the render container.

This PR annotates fns in that branch too. injectNetworkAnnotation already declines to overwrite a fn that carries its own runtime-docker-network annotation (PR #65), so users who pin fns to a specific network are unaffected.

The result is that Setup is idempotent for in-process multi-batch rendering: a downstream tool like crossplane-contrib/crossplane-diff that calls Render across many compositions in a single process can call Setup once per composition, and every batch's fns end up on the same network. The first call returns the real cleanup; subsequent calls return no-ops, so defer cleanup() per batch is safe in LIFO order. Discussion of the alternative (a new AddFunctions interface method) is in the issue #96 thread; the short version is that the uniform-per-batch caller shape Option A provides is what callers actually want, and Option B's interface delta doesn't earn its keep.

Production change is one line of new behavior in the early-return branch of dockerRenderEngine.Setup, plus a small variable-declaration tidy. The Engine interface signature is unchanged; localRenderEngine, MockEngine, and all callers in xr / op are untouched.

Tests are additive: TestDockerRenderEngineSetup covers the early-return branch directly (annotates on preset network, preserves user-set annotation, empty-fns boundary), and TestDockerRenderEngineSetupMultiBatch exercises the in-process multi-batch scenario from #96. The create-new-network branch is unchanged and continues to be exercised by the existing integration-style command tests.

Docstrings on both Engine.Setup (interface) and dockerRenderEngine.Setup (impl) are updated to describe the multi-call semantics so callers can rely on them.

Fixes #96

I have:

• Read and followed Crossplane's contribution process.
• Run ./nix.sh flake check to ensure this PR is ready for review.
• Added or updated unit tests.
• Linked a PR or a docs tracking issue to document this change.
• Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4071e1b8-de72-430a-983b-ebc93091bf02

📥 Commits

Reviewing files that changed from the base of the PR and between 5050d31 and ad83077.

📒 Files selected for processing (2)

• cmd/crossplane/render/engine.go
• cmd/crossplane/render/engine_docker_test.go

✅ Files skipped from review due to trivial changes (1)

• cmd/crossplane/render/engine.go

---

📝 Walkthrough

Walkthrough

Updates Engine.Setup to reuse an existing Docker network on repeated calls, while documenting cleanup ownership. The Docker engine annotates functions for the existing network, and tests cover preset-network and repeated-call behavior.

Changes

Docker render setup reuse

Layer / File(s)	Summary
Setup contract and behavior cmd/crossplane/render/engine.go, cmd/crossplane/render/engine_docker.go	Engine.Setup documents multi-call cleanup semantics, and dockerRenderEngine.Setup annotates supplied functions when e.network is already set before returning a no-op cleanup.
Setup tests cmd/crossplane/render/engine_docker_test.go	Tests cover preset-network annotation behavior, preservation of existing annotations, nil and empty function inputs, and repeated pre-seeded calls.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• crossplane/cli#65: Shares the Docker render network annotation path and the existing-network branch in dockerRenderEngine.Setup.

Suggested reviewers

• adamwg

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise, under 72 characters, and matches the network-reuse annotation change.
Description check	✅ Passed	The description clearly describes the same Setup/network reuse behavior and added tests.
Linked Issues check	✅ Passed	The PR implements the linked issue's Option A by reusing the existing network, annotating new functions, preserving user annotations, and keeping the interface unchanged.
Out of Scope Changes check	✅ Passed	The summary shows only the setup logic, docs, and tests needed for the issue, with no unrelated code changes.
Breaking Changes	✅ Passed	Changes only adjust Setup behavior/docs/tests; no public fields/flags were removed/renamed or made required, and no cmd/** behavior was removed.
Feature Gate Requirement	✅ Passed	PASS: This PR only changes cmd/crossplane/render behavior/docs/tests; no apis/** changes or feature-flag plumbing were added, and it fixes existing Setup semantics.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

cmd/crossplane/render/engine_docker_test.go (1)

302-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Could we fold this scenario into the table-driven Setup cases?

This test looks good functionally, but it breaks the _test.go convention used just above. Keeping the multi-batch scenario in the same args/want/reason table would make the coverage easier to extend and keep the file aligned with local test patterns. As per path instructions, **/*_test.go: "Enforce table-driven test structure: PascalCase test names (no underscores), args/want pattern, use cmp.Diff with cmpopts.EquateErrors() for error testing. Check for proper test case naming and reason fields."

🤖 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 302 - 351, The
multi-batch Setup scenario in dockerRenderEngine should be folded into the
existing table-driven test pattern instead of living as a standalone test. Move
this coverage into the Setup cases alongside the other args/want/reason entries,
and keep using the same Setup helper patterns and cmp.Diff-based assertions so
the test structure stays consistent with the surrounding engine_docker_test
coverage.

Source: Path instructions

cmd/crossplane/render/engine.go (1)

44-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Please clarify the preconfigured-network exception in the contract.

Thanks for documenting the multi-call behavior. One edge case still reads a little too strongly here: with the Docker engine, the first Setup() call can still return a no-op cleanup when the network was pre-seeded via --crossplane-docker-network. Could you call that out explicitly so the interface comment stays aligned with cmd/crossplane/render/engine_docker.go and its preset-network tests?

🤖 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.go` around lines 44 - 50, The Setup contract
comment needs to explicitly mention the preconfigured Docker network exception.
Update the interface documentation on Setup in engine.go to state that although
the first call normally creates the environment and returns the real cleanup,
the Docker implementation may return a no-op cleanup on that first call when a
network was already provided via the preset network path. Keep the wording
aligned with the behavior in engine_docker.go and the preset-network tests so
callers understand this special case.

🤖 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.

Inline comments:
In `@cmd/crossplane/render/engine_docker_test.go`:
- Around line 267-274: The current `EmptyFunctionsList` test case only covers a
nil `fns` slice, not an actual empty slice, so it does not match the boundary
case described. Update `TestDockerRenderEngine_Setup` in `dockerRenderEngine`
test coverage by either adding a separate case with `fns: []pkgv1.Function{}` or
renaming this case to clearly indicate it tests nil only, so the distinction
between nil and empty is explicit.

---

Nitpick comments:
In `@cmd/crossplane/render/engine_docker_test.go`:
- Around line 302-351: The multi-batch Setup scenario in dockerRenderEngine
should be folded into the existing table-driven test pattern instead of living
as a standalone test. Move this coverage into the Setup cases alongside the
other args/want/reason entries, and keep using the same Setup helper patterns
and cmp.Diff-based assertions so the test structure stays consistent with the
surrounding engine_docker_test coverage.

In `@cmd/crossplane/render/engine.go`:
- Around line 44-50: The Setup contract comment needs to explicitly mention the
preconfigured Docker network exception. Update the interface documentation on
Setup in engine.go to state that although the first call normally creates the
environment and returns the real cleanup, the Docker implementation may return a
no-op cleanup on that first call when a network was already provided via the
preset network path. Keep the wording aligned with the behavior in
engine_docker.go and the preset-network tests so callers understand this special
case.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 9d864580-e182-43fc-a2bb-6f19140a3c40

📥 Commits

Reviewing files that changed from the base of the PR and between d0fecff and 5050d31.

📒 Files selected for processing (3)

• cmd/crossplane/render/engine.go
• cmd/crossplane/render/engine_docker.go
• cmd/crossplane/render/engine_docker_test.go

[adamwg]

LGTM. Agreed this is the right change - no need to expand the Engine interface.

[jcogilvie]

@coderabbitai addressed your two nitpicks plus the inline finding in ad83077:

1. Inline (engine_docker_test.go line 312) — Split into two table cases: NilFunctionsList (nil slice) and EmptyFunctionsList (non-nil empty slice). Both exercised under the same runner.

2. Multi-batch test folded into table-driven structure — Each case now holds an ordered batches []batch slice and the runner loops Setup once per batch, asserting fn state after each. Single-batch cases supply one batch; the new MultiBatchAnnotatesAdditionalFunctions case supplies two. The standalone TestDockerRenderEngineSetupMultiBatch is gone; coverage is preserved and the file now matches the table-driven path-instruction convention.

3. Interface docstring exception called out — Engine.Setup's contract no longer claims "first call returns a real cleanup." Reworded to: the cleanup is real only on calls that create a new environment. That excludes calls on engines pre-configured to use an externally-managed environment (Docker engine with --crossplane-docker-network) and calls on engines with nothing to clean up (local engine).

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@jcogilvie Thanks — those updates address my feedback. The nil vs empty slice split, the table-driven multi-batch coverage, and the clarified Engine.Setup contract all look aligned with the concerns I raised.