feat(cli): copy python-executor into gateway-runtime image#1926
feat(cli): copy python-executor into gateway-runtime image#1926renuka-fernando wants to merge 6 commits into
Conversation
Gateway-builder now emits a python-executor/ directory alongside the gateway-runtime build context. Stage it into the runtime context and add a COPY step in the runtime Dockerfile template so the custom python-executor replaces the one baked into the base image, matching the existing policy-engine binary pattern.
📝 WalkthroughOverviewThis change updates the CLI and gateway-builder templates to allow a custom Changes
Impact
WalkthroughThis pull request stages a custom python-executor directory from gateway-builder output into the gateway-runtime Docker build context when present. BuildGatewayImages invokes a new helper to copy output/python-executor into output/gateway-runtime/python-executor before image build. The runtime Dockerfile removes the default /app/python-executor, copies the staged executor, installs Python packaging and requirements into /app/python-libs, then purges build-time packages. The generator's copyDir now skips only .venv, pycache, and .git and ignores non-regular filesystem entries. Sequence DiagramsequenceDiagram
participant BuildGatewayImages
participant ensurePythonExecutorInRuntimeContext
participant FileSystem
participant DockerBuilder
BuildGatewayImages->>ensurePythonExecutorInRuntimeContext: stage python-executor (tempDir)
ensurePythonExecutorInRuntimeContext->>FileSystem: stat output/python-executor
alt executor exists
ensurePythonExecutorInRuntimeContext->>FileSystem: remove dest gateway-runtime/python-executor
ensurePythonExecutorInRuntimeContext->>FileSystem: copy output/python-executor -> gateway-runtime/python-executor
end
ensurePythonExecutorInRuntimeContext-->>BuildGatewayImages: return result
BuildGatewayImages->>DockerBuilder: build gateway-runtime image
DockerBuilder->>DockerBuilder: remove /app/python-executor
DockerBuilder->>DockerBuilder: copy staged python-executor -> /app/python-executor
DockerBuilder->>DockerBuilder: install requirements -> /app/python-libs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cli/src/internal/gateway/docker_build.go (1)
131-150:⚠️ Potential issue | 🔴 CriticalFunction behavior conflicts with Dockerfile expectations.
This function returns
nilwhenpython-executordoesn't exist (line 135), making the staging optional. However,gateway/gateway-builder/templates/Dockerfile.gateway-runtime.tmplline 29 unconditionally executesCOPY python-executor/ /app/python-executor/, which will fail if the directory is absent from the build context.This mismatch will cause Docker build failures when gateway-builder doesn't produce the python-executor directory. See the comment on the Dockerfile for resolution options.
🤖 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 `@cli/src/internal/gateway/docker_build.go` around lines 131 - 150, ensurePythonExecutorInRuntimeContext currently returns nil when pythonExecutorSrc is missing which conflicts with the Dockerfile that always COPYs python-executor; instead, detect the missing source and create an empty pythonExecutorDst so the build context always contains the directory. In the ensurePythonExecutorInRuntimeContext function, when errors.Is(err, os.ErrNotExist) is true, call os.MkdirAll(pythonExecutorDst, 0o755) (ensuring parent dirs exist) and optionally touch a placeholder file, then return nil; keep the subsequent RemoveAll and utils.CopyDir logic for the case where pythonExecutorSrc exists.
🧹 Nitpick comments (1)
cli/src/internal/gateway/docker_build.go (1)
145-147: 💤 Low valueConsider adding user feedback for the staging operation.
The function silently stages the python-executor directory. Consider adding a success message similar to line 69's output copy confirmation, or at minimum logging when the directory is staged vs. skipped for debugging purposes.
🤖 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 `@cli/src/internal/gateway/docker_build.go` around lines 145 - 147, The staging of the python-executor directory is silent; after calling utils.CopyDir(pythonExecutorSrc, pythonExecutorDst) add a user-facing success/info log (using the same logger/format used for the existing output copy confirmation) indicating the python-executor was staged, and also log a message when the source directory is missing or the staging is intentionally skipped; reference the utils.CopyDir call and the pythonExecutorSrc/pythonExecutorDst variables to locate where to add a processLogger.Infof or fmt.Printf success message and a conditional log for the skipped 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.
Duplicate comments:
In `@cli/src/internal/gateway/docker_build.go`:
- Around line 131-150: ensurePythonExecutorInRuntimeContext currently returns
nil when pythonExecutorSrc is missing which conflicts with the Dockerfile that
always COPYs python-executor; instead, detect the missing source and create an
empty pythonExecutorDst so the build context always contains the directory. In
the ensurePythonExecutorInRuntimeContext function, when errors.Is(err,
os.ErrNotExist) is true, call os.MkdirAll(pythonExecutorDst, 0o755) (ensuring
parent dirs exist) and optionally touch a placeholder file, then return nil;
keep the subsequent RemoveAll and utils.CopyDir logic for the case where
pythonExecutorSrc exists.
---
Nitpick comments:
In `@cli/src/internal/gateway/docker_build.go`:
- Around line 145-147: The staging of the python-executor directory is silent;
after calling utils.CopyDir(pythonExecutorSrc, pythonExecutorDst) add a
user-facing success/info log (using the same logger/format used for the existing
output copy confirmation) indicating the python-executor was staged, and also
log a message when the source directory is missing or the staging is
intentionally skipped; reference the utils.CopyDir call and the
pythonExecutorSrc/pythonExecutorDst variables to locate where to add a
processLogger.Infof or fmt.Printf success message and a conditional log for the
skipped case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01631d8a-3434-4b07-a3f7-8bbc2bd014c0
📒 Files selected for processing (2)
cli/src/internal/gateway/docker_build.gogateway/gateway-builder/templates/Dockerfile.gateway-runtime.tmpl
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gateway/gateway-builder/internal/policyengine/generator.go (1)
366-380:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRedundant and inconsistent directory skip checks.
The directory skip logic has two separate checks:
- Lines 368-370 skip
.venv,venv,env,__pycache__, and.git- Lines 376-378 skip only
.venv,__pycache__, and.gitThe second check is redundant because directories matching its conditions are already filtered by the first check. Additionally, per the AI summary, the intent is to narrow skipping to only
.venv,__pycache__, and.git, but line 368 still checks forvenvandenv(without dots), creating inconsistency.🔧 Proposed fix to remove redundancy and align with narrowing intent
// Skip common virtual environment and cache directories if info.IsDir() && path != src { name := info.Name() - if name == ".venv" || name == "venv" || name == "env" || name == "__pycache__" || name == ".git" { + if name == ".venv" || name == "__pycache__" || name == ".git" { return filepath.SkipDir } } dstPath := filepath.Join(dst, relPath) if info.IsDir() { - if info.Name() == ".venv" || info.Name() == "__pycache__" || info.Name() == ".git" { - return filepath.SkipDir - } return os.MkdirAll(dstPath, info.Mode()) }🤖 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 `@gateway/gateway-builder/internal/policyengine/generator.go` around lines 366 - 380, The directory-skip logic is duplicated and inconsistent between the two checks around info.IsDir() (the block using path != src with name := info.Name()) and the later if info.IsDir() that returns filepath.SkipDir; consolidate them by removing the earlier redundant check (the one that tests for ".venv", "venv", "env", "__pycache__", ".git") and keep a single directory-skip branch that checks only the intended names (".venv", "__pycache__", ".git") using info.Name(), then proceed to create dstPath with os.MkdirAll(dstPath, info.Mode()) in the non-skipped directory branch.
🤖 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.
Outside diff comments:
In `@gateway/gateway-builder/internal/policyengine/generator.go`:
- Around line 366-380: The directory-skip logic is duplicated and inconsistent
between the two checks around info.IsDir() (the block using path != src with
name := info.Name()) and the later if info.IsDir() that returns
filepath.SkipDir; consolidate them by removing the earlier redundant check (the
one that tests for ".venv", "venv", "env", "__pycache__", ".git") and keep a
single directory-skip branch that checks only the intended names (".venv",
"__pycache__", ".git") using info.Name(), then proceed to create dstPath with
os.MkdirAll(dstPath, info.Mode()) in the non-skipped directory branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b96c417a-52d9-4906-b2ee-332e15529f0e
📒 Files selected for processing (2)
gateway/gateway-builder/internal/policyengine/generator.gogateway/gateway-builder/templates/Dockerfile.gateway-runtime.tmpl
2 participants
Purpose
Gateway-builder now emits a python-executor/ directory alongside the gateway-runtime build context. Stage it into the runtime context and add a COPY step in the runtime Dockerfile template so the custom python-executor replaces the one baked into the base image, matching the existing policy-engine binary pattern.