Skip to content
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

wrangler support Queues http-pull consumer config and cli #5231

Conversation

w-kuhn
Copy link
Contributor

@w-kuhn w-kuhn commented Mar 12, 2024

What this PR solves / how to test

Fixes MQ-307

Cloudflare Queues currently only supports worker consumers. Support for http-pull consumers is coming soon and part of that support is wranglerization.

This change extends the current worker-only consumer config (wrangler queues consumer add <queue-name> <script-name> ...options) to include wrangler queues consumer http add|remove <queue-name> ...options and wrangler queues consumer worker add|remove <queue-name> <script-name> ...options.

wrangler queues consumer

Configure Queue Consumers

Commands:
  wrangler queues consumer add <queue-name> <script-name>     Add a Queue Worker Consumer
  wrangler queues consumer remove <queue-name> <script-name>  Remove a Queue Worker Consumer
  wrangler queues consumer http                               Configure Queue HTTP Pull Consumers
  wrangler queues consumer worker                             Configure Queue Worker Consumers

and:

wrangler queues consumer http

Configure Queue HTTP Pull Consumers

Commands:
  wrangler queues consumer http add <queue-name>     Add a Queue HTTP Pull Consumer
  wrangler queues consumer http remove <queue-name>  Remove a Queue HTTP Pull Consumer

Author has addressed the following

@w-kuhn w-kuhn requested a review from a team as a code owner March 12, 2024 18:19
Copy link

changeset-bot bot commented Mar 12, 2024

🦋 Changeset detected

Latest commit: c7322a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers Patch

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

Copy link
Contributor

github-actions bot commented Mar 12, 2024

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-wrangler-5231

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5231/npm-package-wrangler-5231

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-wrangler-5231 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-create-cloudflare-5231 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-cloudflare-kv-asset-handler-5231
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-miniflare-5231
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-cloudflare-pages-shared-5231
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8366124306/npm-package-cloudflare-vitest-pool-workers-5231

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.36.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240314.0
workerd 1.20240314.0 1.20240314.0
workerd --version 1.20240314.0 2024-03-14

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

Copy link
Contributor

@elithrar elithrar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from a help text / CLI sub-command surface perspective!

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 92.07921% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 72.14%. Comparing base (9fd7eba) to head (c7322a3).
Report is 10 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5231      +/-   ##
==========================================
+ Coverage   71.96%   72.14%   +0.17%     
==========================================
  Files         313      317       +4     
  Lines       16239    16318      +79     
  Branches     4157     4164       +7     
==========================================
+ Hits        11687    11773      +86     
+ Misses       4552     4545       -7     
Files Coverage Δ
packages/wrangler/src/config/validation.ts 90.12% <ø> (ø)
.../src/queues/cli/commands/consumer/http-pull/add.ts 100.00% <100.00%> (ø)
...rc/queues/cli/commands/consumer/http-pull/index.ts 100.00% <100.00%> (ø)
...c/queues/cli/commands/consumer/http-pull/remove.ts 100.00% <100.00%> (ø)
...ler/src/queues/cli/commands/consumer/worker/add.ts 100.00% <100.00%> (ø)
.../src/queues/cli/commands/consumer/worker/remove.ts 100.00% <100.00%> (ø)
packages/wrangler/src/queues/cli/commands/index.ts 100.00% <100.00%> (ø)
packages/wrangler/src/queues/client.ts 96.42% <94.73%> (-0.87%) ⬇️
packages/wrangler/src/deploy/deploy.ts 90.30% <93.10%> (+0.34%) ⬆️
...wrangler/src/queues/cli/commands/consumer/index.ts 84.61% <80.00%> (-15.39%) ⬇️
... and 1 more

... and 7 files with indirect coverage changes

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions. But looking pretty good.

Oh and I think this should have a changeset, right?
(Edit: I see that you have marked this, and docs, as "TODO before merge" - so we should hold you to that!)

packages/wrangler/src/deploy/deploy.ts Outdated Show resolved Hide resolved
packages/wrangler/src/deploy/deploy.ts Show resolved Hide resolved
packages/wrangler/src/queues/cli/commands/index.ts Outdated Show resolved Hide resolved
packages/wrangler/src/queues/client.ts Show resolved Hide resolved
@jbwcloudflare
Copy link
Contributor

I think we'll want a new test that runs wrangler deploy with an HTTP consumer configured in wrangler.toml, like this one for "script" consumers:

it("should update queue consumers on deploy", async () => {
writeWranglerToml({
queues: {
consumers: [
{
queue: "queue1",
dead_letter_queue: "myDLQ",
max_batch_size: 5,
max_batch_timeout: 3,
max_retries: 10,
},
],
},
});
await fs.promises.writeFile("index.js", `export default {};`);
mockSubDomainRequest();
mockUploadWorkerRequest();
mockGetQueue("queue1");
mockPutQueueConsumer("queue1", "test-name", {
dead_letter_queue: "myDLQ",
settings: {
batch_size: 5,
max_retries: 10,
max_wait_time_ms: 3000,
},
});
await runWrangler("deploy index.js");
expect(std.out).toMatchInlineSnapshot(`
"Total Upload: xx KiB / gzip: xx KiB
Uploaded test-name (TIMINGS)
Published test-name (TIMINGS)
https://test-name.test-sub-domain.workers.dev
Consumer for queue1
Current Deployment ID: Galaxy-Class"
`);
});

@w-kuhn

This comment was marked as outdated.

@w-kuhn

This comment was marked as outdated.

@petebacondarwin petebacondarwin added blocked Blocked on other work internal Requires support from the Cloudflare Platform labels Mar 19, 2024
@petebacondarwin petebacondarwin self-requested a review March 19, 2024 16:11
@w-kuhn

This comment was marked as outdated.

@w-kuhn
Copy link
Contributor Author

w-kuhn commented Mar 20, 2024

addressed feedback and it's all green - I think this is ready for final review.

ETA: We're going to need to wait to ship this until the api for updating pull consumers is available. Will need to utilize that behavior in toml config.

@jbwcloudflare I updated wrangler deploy to check for a pull consumer first then update if it exists or create if not. I can't fully test it until that config api lands.

tested deploy multiple times on pre-release build against staging api, all good 👍

@@ -96,6 +97,33 @@ export async function postConsumer(
);
}

export async function postTypedConsumer(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's more confusing than helpful to try to offer a generic "typed consumer" function here.

I suggest just having a separate postWorkerConsumer and postHttpConsumer. Each one can have request/response types that apply to that consumer type.

packages/wrangler/src/queues/cli/commands/index.ts Outdated Show resolved Hide resolved
packages/wrangler/src/deploy/deploy.ts Outdated Show resolved Hide resolved
packages/wrangler/src/deploy/deploy.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, from a Wrangler team perspective. Nice work @w-kuhn

@petebacondarwin
Copy link
Contributor

Oh actually the test failures are legit: https://github.com/cloudflare/workers-sdk/actions/runs/8364727430/job/22900763340?pr=5231#step:8:2073

Need to update some snapshots.

@w-kuhn
Copy link
Contributor Author

w-kuhn commented Mar 20, 2024

Oh actually the test failures are legit: https://github.com/cloudflare/workers-sdk/actions/runs/8364727430/job/22900763340?pr=5231#step:8:2073

Need to update some snapshots.

Thanks! Fixed that up. Just need to open up some gates before merging tomorrow. 🥳

@RamIdeas RamIdeas merged commit e88ad44 into cloudflare:main Mar 21, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on other work internal Requires support from the Cloudflare Platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants