feat: add NODE_VERSION build arg to sandbox Dockerfile (#610)#659
feat: add NODE_VERSION build arg to sandbox Dockerfile (#610)#659mvanhorn wants to merge 1 commit intocloudflare:mainfrom
Conversation
Mirrors the existing BUN_VERSION pattern. Operators who want to test
their sandbox image against Node 22 (or pin to a specific patch
version) can now do:
docker build --build-arg NODE_VERSION=22 ...
Default stays at Node 20 LTS, so the existing build behavior is
unchanged.
Changes:
- Top of file: declared `ARG NODE_VERSION=20` next to BUN_VERSION,
with the same override-via-build-arg comment.
- The two `FROM node:20-slim` stages (pruner, builder) now use
`FROM node:${NODE_VERSION}-slim`.
- The runtime-base stage re-declares `ARG NODE_VERSION=20` so the
later `COPY --from=node:${NODE_VERSION}-slim` calls can resolve
it (Docker requires re-declaration after each FROM).
- Updated the inline comment from "Install Node.js 20 LTS" to
reflect the override path.
Closes cloudflare#610
mvanhorn
requested review from
aron-cf,
ghostwriternr,
scuffi and
whoiskatrin
as code owners
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
/bonk fix the build and code if needed |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
The Dockerfile change itself is clean and correct — the ARG declaration, re-declaration inside runtime-base, and all four interpolation sites look good. Docker's stage-scoped ARG semantics are handled properly.
However, there are two issues that should be addressed before merging:
1. docker-bake.hcl needs updating
The CI builds use docker-bake.hcl, which passes build args to the Dockerfile via the _common target. Currently it only passes BUN_VERSION:
args = { SANDBOX_VERSION = SANDBOX_VERSION, BUN_VERSION = BUN_VERSION }Without adding NODE_VERSION here, the bake variable/env var won't reach the Dockerfile — users would need to set it via --build-arg directly, bypassing the bake workflow entirely. For consistency with BUN_VERSION, docker-bake.hcl should declare a NODE_VERSION variable and pass it through args.
2. Missing changeset
Per this repo's contribution guidelines, Dockerfile changes that affect the published @cloudflare/sandbox package need a changeset file in .changeset/. The changeset bot already flagged this. Since this is a Dockerfile-only change with a backward-compatible default, a patch changeset referencing @cloudflare/sandbox is appropriate.
| @@ -1,5 +1,7 @@ | |||
| # Bun version — override via --build-arg BUN_VERSION=$(cat .bun-version) | |||
| ARG BUN_VERSION=1 | |||
| # Node version — override via --build-arg NODE_VERSION=22 | |||
There was a problem hiding this comment.
Nit: the comment shows NODE_VERSION=22 as the example override, while BUN_VERSION on line 1 shows the $(cat .bun-version) pattern. This is fine as-is (there's no .node-version file in the repo), just noting the slight inconsistency in comment style.
| # Bun version — override via --build-arg BUN_VERSION=$(cat .bun-version) | ||
| ARG BUN_VERSION=1 | ||
| # Node version — override via --build-arg NODE_VERSION=22 | ||
| ARG NODE_VERSION=20 |
There was a problem hiding this comment.
The docker-bake.hcl file (used by CI) needs a corresponding NODE_VERSION variable and it needs to be added to the _common target's args map — otherwise this build arg can't be controlled through the bake workflow that CI and the publish pipeline use. Without that, the arg only works with raw docker build --build-arg invocations.
In docker-bake.hcl, something like:
variable "NODE_VERSION" { default = "20" }and update the _common target:
args = { SANDBOX_VERSION = SANDBOX_VERSION, BUN_VERSION = BUN_VERSION, NODE_VERSION = NODE_VERSION }
|
Review posted. Here's a summary of the findings: The Dockerfile changes are correct — the Two issues flagged:
|
|
/bonk fix all the issues you've found |
There was a problem hiding this comment.
Follow-up review
The Dockerfile changes are correct — the ARG declarations, re-declaration inside runtime-base, and all interpolation sites are handled properly.
Two issues from the previous review remain unresolved:
1. docker-bake.hcl must be updated
The CI pipeline builds all Docker images via docker-bake.hcl (see reusable-build.yml:244). The _common target explicitly passes build args to the Dockerfile, and currently only includes SANDBOX_VERSION and BUN_VERSION. Without updating it, NODE_VERSION will silently fall back to the Dockerfile default no matter what env var is set in CI.
Two changes needed in docker-bake.hcl:
Line 11 — add a variable declaration:
variable "NODE_VERSION" { default = "20" }Line 28 — pass it through to the Dockerfile:
args = { SANDBOX_VERSION = SANDBOX_VERSION, BUN_VERSION = BUN_VERSION, NODE_VERSION = NODE_VERSION }2. Missing changeset
The Dockerfile is part of the @cloudflare/sandbox package. A changeset file is required per repo conventions. Create e.g. .changeset/node-version-build-arg.md:
---
'@cloudflare/sandbox': patch
---
Add `NODE_VERSION` build arg to the Dockerfile, allowing operators to customize the Node.js version used in sandbox container images.Per repo guidelines, only reference @cloudflare/sandbox — never the internal packages.
| # Bun version — override via --build-arg BUN_VERSION=$(cat .bun-version) | ||
| ARG BUN_VERSION=1 | ||
| # Node version — override via --build-arg NODE_VERSION=22 | ||
| ARG NODE_VERSION=20 |
There was a problem hiding this comment.
The ARG itself is fine. Just a reminder: docker-bake.hcl (which drives CI builds) also needs a corresponding NODE_VERSION variable and args entry — otherwise this build arg is only usable via raw docker build --build-arg and won't be controllable through the bake workflow.
|
Review posted successfully. Here's a summary: Request Changes review submitted on PR #659 with two required fixes:
The Dockerfile changes themselves are correct — no issues there. |
2 participants
Closes #610.
What changed
Added a `NODE_VERSION` build arg to `packages/sandbox/Dockerfile`, mirroring the existing `BUN_VERSION` pattern from line 1. Default stays at `20` so existing builds are unchanged; operators who want to test against Node 22 (or pin to a specific patch) can now run:
```bash
docker build --build-arg NODE_VERSION=22 ...
```
Touched four spots in one file:
Why this matters
The issue (vmsp) noted that BUN_VERSION is already a build arg and asked for the same treatment for Node. The Bun pattern was a simple one-line ARG plus interpolation; the Node side is a little more involved because the version is referenced in two FROM stages plus two COPY --from lines inside a third stage. With one global ARG plus one re-declaration, all four call sites now share a single source of truth.
Verification