-
Notifications
You must be signed in to change notification settings - Fork 1.1k
support using containers with ctx.exports #11512
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 3a55730 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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this code is in the cloudchamber folder but isn't actually used by any cloudchamber paths, so i've moved these functions into the containers folder. (under build.ts and deploy.ts)
| versionId, | ||
| accountId, | ||
| scriptName, | ||
| dryRun: props.dryRun ?? false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not strictly related to this PR but dry run will have already exited by the point this is called so this was unnecessary
350145c to
52ead14
Compare
38d4bd9 to
52cacc5
Compare
petebacondarwin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a few questions
Fixes CC-6386.
The containers control plane needs a DO namespace id (sort of*) when creating a container app in order to link the container app and DO. This is helpfully returned by EWC when the script has a binding to a DO, but not when the DO is simply exported from the script and unbound. For now, we have to list and filter all DO namespace ids, which is not ideal, but there is a PR in flight that should enable search params on the DO list endpoint.
*The container create API can actually accept a script and class name instead, and this is what (future) direct API users would be using.
However, wrangler is in an awkward place because of how we automatically provision containers. Wrangler deploy needs to know whether a particular container + durable object combination already exists, and thus whether we should create a new container app, modify an existing one, or error because you've accidentally tried to reuse the name of an existing container app.
So wrangler will have to get the container's DO's namespace id to make sure it hasn't changed, and at that point we might as well just pass that to the container create API.
(possibly this is something that should be solved EWC side but i think we could follow up with that if we want?)