Adds option to skip container rollout on deploy#12656
Adds option to skip container rollout on deploy#12656mikenomitch wants to merge 1 commit intomainfrom
Conversation
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
mikenomitch
force-pushed
the
nomitch/container-rollout-none
branch
from
3465234 to
8e633b3
Compare
There was a problem hiding this comment.
🟡 Dry-run path missing containersRollout !== "none" guard still attempts Docker build
When --dry-run --containers-rollout=none is used with a Dockerfile-based container, the Docker verification is correctly skipped at packages/wrangler/src/deploy/deploy.ts:884-895, but the dry-run code path at lines 897-909 still attempts to build containers via Docker without checking props.containersRollout !== "none".
Root Cause
The non-dry-run deploy path at packages/wrangler/src/deploy/deploy.ts:1178 correctly guards with props.containersRollout !== "none", but the dry-run path at line 898 only checks normalisedContainerConfig.length before invoking buildContainer(). Since the Docker verification was skipped (line 884-895 has the guard), Docker may not be installed, and the buildContainer call at line 901 will fail when it tries to invoke Docker commands.
This defeats the purpose of --containers-rollout=none, which per the CLI description should deploy "without building or updating any Containers" (packages/wrangler/src/deploy/index.ts:230).
Impact: Users running --dry-run --containers-rollout=none with a Dockerfile-based container and no Docker installed will get a confusing Docker error instead of a clean dry-run output.
(Refers to lines 898-909)
Was this helpful? React with 👍 or 👎 to provide feedback.
| let thrownError: Error | undefined; | ||
| try { | ||
| await runWrangler("deploy index.js --containers-rollout=none"); | ||
| } catch (e) { | ||
| thrownError = e as Error; | ||
| } | ||
|
|
||
| expect(thrownError?.message ?? "").not.toContain( | ||
| "The Docker CLI could not be launched" | ||
| ); |
There was a problem hiding this comment.
can we get rid of this try catch and just add the appropriate mocks (ref the test above), this feels a bit imprecise
2 participants
This allows you to skip deploying a container. This is useful if you know that your container is not going to be updated or you don't have Docker locally, but still want to make changes to your Worker.