fix(render): do not overwrite function docker network if set, start crossplane-container in same network#65
fix(render): do not overwrite function docker network if set, start crossplane-container in same network#65nkzk wants to merge 18 commits into
Conversation
|
Thanks for the PR, @nkzk! Would you mind creating an issue for this as well, for discoverability and tracking? I haven't reviewed in detail yet, but the described fixes sound reasonable. |
nkzk
changed the title
nkzk
force-pushed
the
fix-render-docker-network
branch
2 times, most recently
from
cad5894 to
abb26bd
Compare
|
I see that there are already tests for passing function annotations to the engine. I had copilot help me create unit tests for injectNetworkAnnotations. Also ran flake check. |
|
Hmm, i got it to work in a devcontainer with this fix, but the current implemention has some issues in CI. But i think this can be solved on the user-side. One of our earliest approaches was to start up functions as service-containers before running multiple/different renders, and it worked because gitlab/github connects the job-container to the bridge-network used by service-containers. But since crossplane render will start up crossplane in another temporary bridge network, it doesnt seem that this will continue to work. However, my theory is that the user can specify the docker-network in their CI-provider (gitlab/github), and then specify the the docker-network flag in the crossplane render command with the fix in this branch to solve this. We have another workflow which uses rootless DinD/PinP, but kind of the same issue there. I'll do some more testing soon. But let me know if something i say sounds off :D |
nkzk
force-pushed
the
fix-render-docker-network
branch
from
669f038 to
396a2d1
Compare
|
Think this PR is ready for review, i did some more testing in CI and did not completely figure it out yet, but i think its just an issue of configuring the docker-network in CI and setting that value as the function-docker-network flag in the render-command. A quality of life improvement for us would be if we can spin up the crossplane container ourselves and make render use it. If we could configure the crossplane-containerthe same way as functions, with the development annotation to manage the container lifecycle ourselves, it would just simplify this alot for us. But maybe its out of scope for this PR, i'm not sure whats the best way to implement this would be. But open to work on it if someone has some ideas. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an optional CrossplaneDockerNetwork flag, threads it into the docker render engine, makes dockerRenderEngine.Setup skip temporary network creation when a network is preconfigured, preserves existing runtime network annotations, provides an annotation parser, and wires network defaulting into both render commands. ChangesDocker network preconfiguration support
Sequence DiagramsequenceDiagram
participant RenderCmd
participant FunctionSet
participant EngineFlags
participant dockerEngine
participant DockerNetwork
RenderCmd->>FunctionSet: load functions with annotations
RenderCmd->>RenderCmd: SetDefaultCrossplaneDockerNetwork(fns)
activate RenderCmd
RenderCmd->>FunctionSet: find first function with AnnotationKeyRuntimeDockerNetwork
RenderCmd->>EngineFlags: set CrossplaneDockerNetwork if flag empty
deactivate RenderCmd
RenderCmd->>dockerEngine: NewEngineFromFlags(network: CrossplaneDockerNetwork)
dockerEngine->>dockerEngine: Setup(functions)
alt CrossplaneDockerNetwork is preconfigured
dockerEngine->>FunctionSet: skip annotation injection
dockerEngine-->>RenderCmd: return no-op cleanup
else CrossplaneDockerNetwork is empty
dockerEngine->>DockerNetwork: create temporary render network
dockerEngine->>FunctionSet: inject network annotation
dockerEngine-->>RenderCmd: return cleanup to remove network
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 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 |
@nkzk Good thought - I can see how this would be useful. It's a little tricky, since the crossplane container in For your use-case, would it be easier to download a crossplane binary and use the |
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
Thanks for filing an issue for this, and for the fix. A few comments inline, but the overall approach looks good to me.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/crossplane/render/engine.go (1)
67-71: ⚡ Quick winUpdate stale constructor docs after signature change.
Could you update this comment? On Line 69 it still mentions a
network parameter, butNewEngineFromFlagsnow derives this fromEngineFlags.CrossplaneDockerNetwork.🤖 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 67 - 71, Update the doc comment for NewEngineFromFlags to remove the outdated reference to a `network parameter` and instead state that the Docker network is derived from EngineFlags.CrossplaneDockerNetwork; specifically edit the comment block above the NewEngineFromFlags function to reflect that when no binary path is set it returns a Docker engine using the resolved image reference and that the Docker network is taken from EngineFlags.CrossplaneDockerNetwork (not supplied by the caller).
🤖 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/op/cmd.go`:
- Around line 170-185: The override parsing for
render.AnnotationKeyRuntimeDockerNetwork is nested inside the if
c.EngineFlags.CrossplaneDockerNetwork == "" block so the --function-annotations
override never applies when a network is already set; move the block that parses
c.FunctionAnnotations (using render.NewAnnotationsFromStrings and checking
render.AnnotationKeyRuntimeDockerNetwork) out of that conditional and always run
it so that when an annotation value exists you set
c.EngineFlags.CrossplaneDockerNetwork (and/or c.CrossplaneDockerNetwork if used
elsewhere) to that value, ensuring the function-annotations override takes
precedence.
---
Nitpick comments:
In `@cmd/crossplane/render/engine.go`:
- Around line 67-71: Update the doc comment for NewEngineFromFlags to remove the
outdated reference to a `network parameter` and instead state that the Docker
network is derived from EngineFlags.CrossplaneDockerNetwork; specifically edit
the comment block above the NewEngineFromFlags function to reflect that when no
binary path is set it returns a Docker engine using the resolved image reference
and that the Docker network is taken from EngineFlags.CrossplaneDockerNetwork
(not supplied by the caller).
🪄 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: 14a09b90-8615-4a74-831b-924cd8db6271
📒 Files selected for processing (6)
cmd/crossplane/render/annotation.gocmd/crossplane/render/engine.gocmd/crossplane/render/engine_docker.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/render.gocmd/crossplane/render/xr/cmd.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/crossplane/render/engine_docker.go
- cmd/crossplane/render/render.go
nkzk
force-pushed
the
fix-render-docker-network
branch
from
5ecd13a to
2c2f39b
Compare
|
I like this change and I want to see it land, but you've checked off the PR checklist box about adding tests and I don't see any. Have I missed something? |
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
LGTM. Per @jcogilvie's comment, this could probably use a bit of test coverage (may be easiest to factor out the network defaulting logic from both render commands and test it separately).
|
yeah my bad, had some tests but dissapeared in a revert+refactor. i can look into it tomorrow! |
Signed-off-by: Nikita Z <nkzk95@gmail.com>
…ntainer in it Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
…tions in comment Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
…lags if empty, default to the first docker-network annotation in the provided functions. If provided, the docker-network annotation in the FunctionAnnotations cli flag takes presedence Signed-off-by: Nikita Z <nkzk95@gmail.com>
…aneDockerNetwork Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Move Crossplane Docker network defaulting into a shared render helper used by both XR and operation render commands. Add focused coverage for function annotation overrides and network defaulting precedence. Also test both render commands to make sure --function-annotations sets the render engine Docker network before the engine is created. Signed-off-by: Nikita Z <nkzk95@gmail.com>
nkzk
force-pushed
the
fix-render-docker-network
branch
from
2974e59 to
591793c
Compare
Signed-off-by: Nikita Z <nkzk95@gmail.com>
Signed-off-by: Nikita Z <54776184+nkzk@users.noreply.github.com>
|
Added tests with alot of help from copilot, but the tests look reasonable to me. Let me know if you have any more feedback, thanks! :) |
Signed-off-by: Nikita Z <nkzk95@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/network.go`:
- Around line 42-44: In the condition checking for
AnnotationKeyRuntimeDockerNetwork, add an additional check to verify that the
annotation value is not empty before assigning it to f.CrossplaneDockerNetwork
and returning. If the value is empty, the code should skip the assignment and
return, allowing the loop to continue checking for other valid annotations
instead of short-circuiting on an empty annotation value.
🪄 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: 053e7f3a-2568-4b83-a7b9-3686bac7c660
📒 Files selected for processing (11)
cmd/crossplane/render/annotation.gocmd/crossplane/render/engine.gocmd/crossplane/render/engine_docker.gocmd/crossplane/render/network.gocmd/crossplane/render/network_test.gocmd/crossplane/render/op/cmd.gocmd/crossplane/render/op/cmd_test.gocmd/crossplane/render/render.gocmd/crossplane/render/render_test.gocmd/crossplane/render/xr/cmd.gocmd/crossplane/render/xr/cmd_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/crossplane/render/annotation.go
- cmd/crossplane/render/engine_docker.go
- cmd/crossplane/render/render.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Nikita Z <54776184+nkzk@users.noreply.github.com>
3 participants
Description of your changes
Closes #75
Fixes:
FunctionAnnotationsflag, run crossplane-container in it.I have:
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.