-
Notifications
You must be signed in to change notification settings - Fork 1k
Improve resource provisioning #10521
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
Conversation
🦋 Changeset detectedLatest commit: 040c1e9 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
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.
this is a really nice command, i had no idea it existed until now
(adding it to the docs here: cloudflare/cloudflare-docs#24833)
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.
Honestly, me neither until writing this PR!
d880601 to
3845580
Compare
8ab18a0 to
c4f361d
Compare
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
.changeset/rotten-glasses-smile.md
Outdated
| Improves the Wrangler auto-provisioning feature by: | ||
| - Writing back changes to the user's config file (not necessary, but can make it resilient to binding name changes) | ||
| - Fixing --dry-run | ||
| - Fixing bindings view for specific versions to not display TOML |
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.
Nit: Should we mention --x-provision just so people know this is behind an experimental flag and the way to test it out?
Also, it would be nice if you can add some details on what the fix for --dry-run is about.
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.
The PR description also mentions "some improvements to R2", would be nice to details what they are
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.
| bindings.send_email.push({ ...binding, name: name }); | ||
| } else if (binding.type === "wasm_module") { | ||
| if (!("source" in binding)) { | ||
| continue; |
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.
should this not be an error?
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.
Or at least a warning to the user?
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.
This function is a bit lossy when used with WorkerMetadataBinding (there's a warning in the comment above, but maybe that should be louder?). Essentially we can't really reconstruct legacy bindings like WASM/Text/Data from just the metadata because they also require access to the Worker's modules
| } | ||
|
|
||
| /** | ||
| * Convert either StartDevWorkerOptions["bindings"] or WorkerMetadataBinding[] to CfWorkerInit["bindings"] |
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.
nit: I like when comments start with a one sentence high level summary of what the function does
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.
Is that not kinda what this line is?
| if (binding.type === "plain_text") { | ||
| bindings.vars ??= {}; | ||
| bindings.vars[name] = binding.value; | ||
| bindings.vars[name] = "value" in binding ? binding.value : binding.text; |
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.
would binding.value ?? binding.text work here and below?
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.
No, because the types don't overlap
packages/wrangler/src/deployment-bundle/create-worker-upload-form.ts
Outdated
Show resolved
Hide resolved
| } else if (previewIsDefined) { | ||
| if (getFlag("RESOURCES_PROVISION")) { | ||
| assert(binding); | ||
| return getIdFromSettings(config, binding, isLocal); |
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.
| return getIdFromSettings(config, binding, isLocal); | |
| return await getIdFromSettings(config, binding, isLocal); |
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.
we should probably have a lint check for that
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.
Is this a problem? It's an async function so returning a promise is perfectly valid
| !namespace.preview_id | ||
| ) { | ||
| assert(binding); | ||
| return getIdFromSettings(config, binding, isLocal); |
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.
| return getIdFromSettings(config, binding, isLocal); | |
| return await getIdFromSettings(config, binding, isLocal); |
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.
As above
| } | ||
|
|
||
| export function getKVNamespaceId( | ||
| async function getIdFromSettings( |
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.
nit: add JSDoc
| bindings.send_email.push({ ...binding, name: name }); | ||
| } else if (binding.type === "wasm_module") { | ||
| if (!("source" in binding)) { | ||
| continue; |
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.
Or at least a warning to the user?
| override inherit(): void { | ||
| if (!this.binding.bucket_name) { | ||
| this.binding.bucket_name = INHERIT_SYMBOL; | ||
| } | ||
| } |
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.
I am struggling to get my head around the logic here and in canInherit(). Could you add some comments about how we decide whether we can inherit an R2 binding?
Also, it might help to move this function next to canInherit() since the logic appears to be tightly coupled between the two.
|
|
||
| /** | ||
| * Inheriting an R2 binding replaces the id property (bucket_name for R2) with the inheritance symbol. | ||
| * This works when deploying (and is appropriate for all other binding types), but it means that the |
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.
Ah! This is the motivation I was failing to understand.
Thank you!
| } | ||
|
|
||
| function getSettings( | ||
| export function getSettings( |
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.
Should this function be moved to a shared location (e.g. a file in the utils dir)? 🤔 (possibly as a followup)
02a064a to
7ed9ee1
Compare
Co-authored-by: Dario Piotrowicz <[email protected]>
Co-authored-by: Dario Piotrowicz <[email protected]>
Co-authored-by: Dario Piotrowicz <[email protected]>
Co-authored-by: James Opstad <[email protected]>
7ed9ee1 to
4a4fad4
Compare
Fixes DEVX-1737, DEVX-1738, DEVX-2171, and DEVX-1740
Improves the Wrangler auto-provisioning feature by:
--dry-run