Bypass container image validation when rollout_kind: none is set#13757
Bypass container image validation when rollout_kind: none is set#13757nikitassharma wants to merge 13 commits into
rollout_kind: none is set#13757Conversation
🦋 Changeset detectedLatest commit: 69d7d56 The changes in this PR will be included in the next version bump. 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
|
OK, so for a new container with The overall logic is sound. The changes properly handle all the edge cases:
The tests cover all these cases. The type changes are consistent across the codebase. The validation properly allows the missing image only when LGTM |
![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.
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
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: |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Vite plugin getContainerOptions doesn't handle undefined image, producing invalid image_uri: undefined (packages/vite-plugin-cloudflare/src/containers.ts:47-56)
The PR made ContainerApp.image optional (packages/workers-utils/src/config/environment.ts:115), but packages/vite-plugin-cloudflare/src/containers.ts was not updated to handle this new case. When container.image is undefined (e.g. rollout_kind: none without an image), isDockerfile(undefined, configPath) returns false (correctly), but the else branch at line 47 sets image_uri: container.image which is undefined. This produces an object with an invalid image_uri: undefined instead of throwing an error like the equivalent wrangler code does in packages/wrangler/src/api/startDevWorker/LocalRuntimeController.ts:494-497. Downstream consumers (e.g. prepareContainerImagesForDev) would then attempt to pull a container with an undefined image URI, causing confusing runtime failures.
View 9 additional findings in Devin Review.
# Conflicts: # packages/wrangler/src/containers/deploy.ts
emily-shen
left a comment
emily-shen
left a comment
There was a problem hiding this comment.
i am a bit confused - why would we want to let people remove the image field when they are intentionally deploying something with a container, just not updating the container?
Since setting This change is intended to allow a minimal wrangler config that reflects reality without having to update a config file when a container change is rolled out elsewhere. |
There was a problem hiding this comment.
oh also i realised i never merged #12656
should we get that in first? i can do that today
probably best to do that one after
|
ooh missed that one, yeah that would be great! |
204cf2b to
52be9d8
Compare
52be9d8 to
634c6d3
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
5 participants
Fixes CC-7586
Setting
rollout_kind: nonefor an existing container skips making any changesto, or rolling out that container. It behaves as a no-op for new containers.
Update the output to more explicitly describe this behavior, and make the image
field optional for existing containers that skip rollouts.
rollout_kind: nonecan be used when trying to deploy worker changes withouttriggering a rollout for any containers.
rollout_kindis not a documented optionA picture of a cute animal (not mandatory, but encouraged)
