add containers build to build output - #15609
emily-shen wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: d4ad323 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| builtImages | ||
| ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
BaseContainerSchema accepts any non-empty application name, so valid standard configs such as name: "Session Container" produce the tag session container:..., which Docker rejects. Durable Object images already sanitize this same input.
| } | |
| sanitizeRepositoryName(config.name), |
|
I'm Bonk, and I've done a quick review of your PR. Adds Container image builds to experimental Build Output.
|
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
befd9e5 to
80cd82e
Compare
80cd82e to
61c24d0
Compare
c8cdb48 to
ef1ca3c
Compare
ef1ca3c to
cf98b7b
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| builtImages.push({ localTag }); | ||
| return { localReference: localTag }; |
There was a problem hiding this comment.
🔴 Repeated builds leak Docker images
Each successful buildAndWriteContainerOutput creates fresh local tags without removing tags referenced by the previous output. Rebuilding overwrites those references, leaving unreachable images that consume Docker disk indefinitely.
Learn more
A local reference is the Docker tag written into the build output for a later deployment. Each Dockerfile build creates a UUID-tagged image, and successful calls intentionally retain it. A later build writes new references into the same Container config paths, but no code reads and removes the superseded tags. Those old images can no longer be reached through the current build output and accumulate on every rebuild.
Example: Build a project containing a 2 GB Dockerfile-backed Container ten times before deployment. The output references only the tenth tag, while Docker retains all ten images and their unique tags.
Recommended fix: Before replacing Container output, read its existing localReference values and remove only tags superseded by a completed build. Preserve the previous output and tags until the new Container configs are written successfully, so a failed rebuild does not destroy the last usable output.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is not ideal, but not as big of a deal as devin is making it out to be, since docker is clever about how it stores images. also, the happy path involves a deployment which will remove the tag. but yes ideally i guess we should use docker to remove the image tag from the last build at the start, just like how we delete the built worker files in the build output directory
| const rawConfig: RawConfig = convertToWranglerConfig( | ||
| worker, | ||
| settings, | ||
| Object.values(getContainerConfigExports(result.data)) |
There was a problem hiding this comment.
🔴 Durable Object Containers abort Vite
A Durable Object-managed Container makes convertToWranglerConfig throw during plugin resolution. Vite never reaches the supported container build-output phase.
Learn more
The validated project config accepts Durable Object-managed Containers, and buildAndWriteContainerOutput explicitly builds their named images. Passing every Container export into convertToWranglerConfig() also passes this valid variant, but convertContainer unconditionally throws for it. This happens while resolving plugin configuration, before the Build Output hook runs.
Example: A default Worker plus defineContainer({ name: "app", schedulingPolicy: "durable-object", images: { web: { dockerfile: "./Dockerfile" } } }) fails during resolvePluginConfig(). The expected build would emit the named web image under the Container's Build Output directory.
Recommended fix: Keep Durable Object-managed Containers out of the Wrangler config conversion, or extend convertToWranglerConfig() to produce their supported Wrangler shape, including the linked Durable Object class and named images. Preserve all Container exports in parsedNewConfig so buildOutputContainers() still emits them.
Was this helpful? React with 👍 or 👎 to provide feedback.
3 participants
This adds container building to the container to
vite buildandwrangler buildwhen using the build output spec.Note that this does not wire in any consumers of the build output - the only consumer i'm going to try to wire in is
cf deploy(and versions upload), which will be happening in the cf repo. So for example this does not wire in the new config into local dev/vite preview.We discussed adding a skip container build option as well but i haven't implemented that
@jamesopstad i wasn't sure how you wanted the container stuff to be structured in the parsed output so feel free to change it
NOTE: this doesn't fully work e2e yet!
Currently this PR does not get convertToWranglerConfig() to populate the containers field from the programmatic container config, but we need it to for validation to pass.
A picture of a cute animal (not mandatory, but encouraged)