[wrangler] Add automatic queue provisioning to resource provisioning#11788
[wrangler] Add automatic queue provisioning to resource provisioning#11788irvinebroque wants to merge 9 commits intomainfrom
Conversation
…system Extends the existing resource provisioning feature to support Cloudflare Queues. When users deploy with queue bindings configured, missing queues are now automatically created, providing the same seamless experience as KV, D1, and R2. Changes: - Add QueueHandler class following the R2Handler pattern (name-based, no ID) - Add queue provisioning helper functions using ComplianceConfig - Collect and de-duplicate queues from both producers and consumers - Skip ensureQueuesExistByConfig when provisioning is enabled - Include delivery_delay in queue binding transformation - Update deploy tests to use --x-provision=false for legacy behavior tests The feature is controlled by the existing --x-provision flag (enabled by default) and --x-auto-create flag for interactive vs automatic creation.
🦋 Changeset detectedLatest commit: 7192f85 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 |
|
Claude finished @irvinebroque's task —— View job Changeset ReviewReviewing changeset files... Tasks:
The changeset Markdown Headers: The changeset uses code blocks with JSONC format which is good, but there's a comment in the review from petebacondarwin noting "We prefer JSONC config nowadays" on this file. Looking at the changeset, it already uses However, the changeset has one minor formatting issue: Line 11: The comment syntax in the example uses Other validation results: The changeset is well-written overall with good examples and clear explanations. The only suggestion is to clarify the configuration format being shown or use the more standard TOML format. |
irvinebroque
requested review from
MattieTK,
danlapid,
jonesphillip,
lrapoport-cf and
mia303
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: |
|
@irvinebroque - was this PR generated by AI? If so, although you said you have reviewed it, can you mention this for transparency in the PR description? |
irvinebroque
force-pushed
the
bib/provision-queues
branch
from
c34bbe1 to
3ef2e35
Compare
| // Skip queues - they use queue name from config, no ID write-back needed | ||
| if (resourceType === "queues") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Not obvious to me why this ended up being necessary, given that Queues should behave like R2 and I don't see equivalent here for R2
There was a problem hiding this comment.
I think these are noops since the binding will be identical and the patch function below will find no difference.
So I think you can remove this check and the one below to simplify this code.
petebacondarwin
left a comment
petebacondarwin
left a comment
There was a problem hiding this comment.
Exciting to see this implemented but I think we can be a bit cleaner with the approach.
| return mock; | ||
| } | ||
|
|
||
| // Legacy helpers for backwards compatibility |
There was a problem hiding this comment.
I am not sure this comment is accurate. These helpers are not legacy nor related to backward compatibility. I would just delete this comment.
| // Skip queues - they use queue name from config, no ID write-back needed | ||
| if (resourceType === "queues") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I think these are noops since the binding will be identical and the patch function below will find no difference.
So I think you can remove this check and the one below to simplify this code.
| bindings: CfWorkerInit["bindings"], | ||
| requireRemote: boolean | ||
| requireRemote: boolean, | ||
| config?: Config |
There was a problem hiding this comment.
Rather than add a new parameter, just repurpose the complianceConfig parameter above. The actual object that is passed in there is always a Config anyway...
async function collectPendingResources(
config: Config,
accountId: string,
scriptName: string,
bindings: CfWorkerInit["bindings"],
requireRemote: boolean
}
| const resources = | ||
| resourceType === "queues" && config | ||
| ? collectUniqueQueueBindings(config) | ||
| : bindings[resourceType] ?? []; |
There was a problem hiding this comment.
Rather than special casing here, I would rather we use polymorphism by adding a new method to the ProvisionResourceHandler class that returns the binding:
getResourcesFromConfig(config: Config | undefined, bindings: B[]) {
return bindings;
}and then in the QueueHandler class:
getResourcesFromConfig(config: Config | undefined, bindings: B[]) {
assert(config);
return collectUniqueQueueBindings(config);
}and the call site would look like:
const resources = HANDLERS[resourceType].getResourcesFromConfig(config, bindings[resourceType] ?? []);Ensures queue producer bindings include the `remote` field, matching the pattern used by R2 and other bindings that support remote mode.
Rather than special-casing queues in collectPendingResources, add a getResourcesFromConfig method to each handler in HANDLERS that can be overridden per resource type. QueueHandler's method calls collectUniqueQueueBindings to collect both producers and consumers. Also simplifies the function signature by using Config directly instead of ComplianceConfig, since Config is always passed in practice.
Clarify that the queue skipping in config patching is required because: 1. config.queues has a different shape (producers/consumers) than other bindings which are flat arrays 2. Queues use name-based identification so no ID write-back is needed
The mockListQueues and mockCreateQueue helpers are standard test utilities, not legacy helpers for backwards compatibility.
| getResourcesFromConfig: ( | ||
| _config: Config, | ||
| bindings: CfKvNamespace[] | ||
| ): CfKvNamespace[] => bindings, |
There was a problem hiding this comment.
My suggestion was for this to be a method on the handler (e.g. here KVHandler. The "standard" implementation will be on ProvisionResourceHandler and only the QueueHandler needs the method to be overridden.
Moreover, I believe that this method can use a type parameter to avoid having to cast the return values.
|
/bonk can you fix the merge conflicts on this pr? |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The feature looks solid overall — queue provisioning slots in cleanly alongside KV/D1/R2. Tests are good, the de-duplication logic is well-thought-out, and CI is green (the Windows failure is a flaky dev-registry timeout, unrelated).
There is one outstanding piece of feedback from @petebacondarwin that hasn't been fully addressed yet, plus a few smaller items I noticed.
Outstanding: getResourcesFromConfig should be a method on the handler class (petebacondarwin's feedback)
Pete's latest comment clarifies that getResourcesFromConfig should live as a method on ProvisionResourceHandler (with a default implementation returning bindings), not as a function on each entry in the HANDLERS map. The current implementation repeats the same trivial (_config, bindings) => bindings lambda for KV, D1, and R2, which is exactly the boilerplate that a base class default eliminates.
Additionally, using the existing generic type parameter B on the class would give proper typing without the intersection cast as CfR2Bucket[] & CfKvNamespace[] & CfD1Database[] & CfQueue[] at the call site.
Suggested shape:
// On ProvisionResourceHandler (static method or standalone):
static getResourcesFromConfig(config: Config, bindings: B[]): B[] {
return bindings;
}
// Override only in QueueHandler:
static getResourcesFromConfig(config: Config, _bindings: CfQueue[]): CfQueue[] {
return collectUniqueQueueBindings(config);
}Since handlers are instantiated per-binding inside collectPendingResources, this would need to be a static method or a function on the Handler class itself (not an instance method), which is awkward with the current architecture. The simplest way to address Pete's feedback while keeping the current instantiation pattern would be to add it as a static method on each handler class, or keep it on the HANDLERS map but eliminate the repeated lambdas by having a shared default. Worth discussing which approach feels cleanest.
Minor items
-
Synthetic binding name for consumer-only queues (
bindings.ts:558): The name__queue_consumer_${consumer.queue}__is used as a synthetic binding name for consumer-only queues. This binding name flows into the provisioning output messages (e.g., the user would see__queue_consumer_incoming__ provisioned). Consider whether this user-visible string should be more human-friendly, or whether consumer-only queues should show differently (e.g., just the queue name). -
listQueuesForProvisioningdoesn't paginate (queues/client.ts:185): The function defaultspageto 1 but doesn't loop through all pages. ForisConnectedToExistingResourcethis is fine because it passes thenamefilter, but theLOADERS.queuescall atbindings.ts:516fetches the full list for the interactive picker — if a user has more than one page of queues, not all will appear. -
queue_nameis always required onCfQueue(not optional): TheisConnectedToExistingResourceguardif (!this.binding.queue_name)at line 414 can never be true sinceCfQueue.queue_nameis typed asstring(not optional). This is fine as defensive code but the comment should reflect it's a type guard for safety rather than a realistic code path. -
The Windows CI failure is unrelated — it's a timeout in
@fixture/dev-registrytail handler test, a known flaky test.
|
Review posted on PR #11788. Here's a summary of the key findings: Overall: The feature is well-implemented — queue provisioning integrates cleanly with the existing KV/D1/R2 pattern. Tests cover inheritance, auto-creation, de-duplication, and consumer-only queues. CI is green (the Windows failure is an unrelated flaky dev-registry test). Main outstanding issue: petebacondarwin's feedback about Minor items flagged:
|
|
Closing this in favour of #13352, which will support Queues + all other bindings |
4 participants
Extends the existing resource provisioning feature to support Cloudflare Queues. When users deploy with queue bindings configured, missing queues are now automatically created, just like KV, D1, and R2.
Changes:
The feature is controlled by the existing
--x-provision flag(enabled by default) and--x-auto-createflag for interactive vs automatic creation.I've tested this out locally and deployed successfully. And reviewed code, left comments. Written with opencode.
Questions