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.

Sign up for GitHub

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

Jump to bottom

fix: use account id for listing zones #6164

Merged
merged 1 commit into from
Jul 2, 2024
Merged

fix: use account id for listing zones #6164

merged 1 commit into from
Jul 2, 2024

Conversation

threepointone
Copy link
Contributor

@threepointone threepointone commented Jun 28, 2024 •
edited
Loading

Fixes #4944

Trying to fetch /zones fails when it spans more than 500 zones. The fix to use an account id when doing so. This patch passes the account id to the zones call, threading it through all the functions that require it.

Author has addressed the following

  • Tests
    • TODO (before merge)
    • Included - updated
    • Not necessary because: already covered
  • E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
    • I don't know
    • Required / Maybe required
    • Not required because: already covered
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation
    • TODO (before merge)
    • Cloudflare docs PR(s):
    • Not necessary because: internal change
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jun 28, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: cdcb94e

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

This PR includes changesets to release 2 packages
Name Type
wrangler Patch
@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
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 28, 2024 •
edited
Loading

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/9730503953/npm-package-wrangler-6164

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

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

Or you can use npx with this latest build directly:

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

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

wrangler@3.62.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240620.0
workerd 1.20240620.1 1.20240620.1
workerd --version 1.20240620.1 2024-06-20

Please ensure constraints are pinned, and miniflare/workerd minor versions match.
@threepointone threepointone force-pushed the zones-for-account-id branch 2 times, most recently from 3dafbf4 to 8c5b227 Compare June 28, 2024 07:19
@threepointone threepointone added the e2e Run e2e tests on a PR label Jun 28, 2024
@threepointone threepointone force-pushed the zones-for-account-id branch 2 times, most recently from c73dc47 to 1840b9d Compare June 28, 2024 07:38
threepointone
threepointone commented Jun 28, 2024
View reviewed changes
packages/wrangler/src/versions/rollback/index.ts
@@ -85,7 +85,7 @@ export async function versionsRollbackHandler(args: VersionsRollbackArgs) {
}));

const message = await prompt(
"Please provide an optional message for this rollback (120 characters max)?",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the trailing ? because this isn't phrased as a question
packages/wrangler/src/__tests__/rollback.test.ts
@@ -119,7 +119,7 @@ describe("rollback", () => {
mockPostDeployment();

mockPrompt({
text: "Please provide a message for this rollback (120 characters max, optional)?",
text: "Please provide an optional message for this rollback (120 characters max)",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm concerned that this test wasn't previously failing.
@threepointone threepointone force-pushed the zones-for-account-id branch 3 times, most recently from 61d6424 to a1d81c1 Compare June 28, 2024 09:38
@threepointone threepointone marked this pull request as ready for review June 28, 2024 09:54
@threepointone threepointone requested a review from a team as a code owner June 28, 2024 09:54
threepointone
threepointone commented Jun 28, 2024
View reviewed changes
packages/wrangler/src/dev.tsx
@@ -912,7 +916,10 @@ export async function validateDevServerSettings(
// we want it to exit the Wrangler process early to allow the user to fix it. Calling it here forces
// the error to be thrown where it will correctly exit the Wrangler process
if (args.remote) {
await getZoneIdForPreview(host, routes);
const accountId =
args.accountId ?? config.account_id ?? (await getAccountId());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RamIdeas does this look right to you?
Copy link
Contributor

@RamIdeas RamIdeas Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks about right. I just checked on the --x-dev-env code path and it passes accountId to getWorkerAccountAndContext so this will continue to work when we flip that flag on by default
@threepointone threepointone force-pushed the zones-for-account-id branch 2 times, most recently from 6066a3d to 7836571 Compare June 29, 2024 08:34
@threepointone
fix: use account id for listing zones
cdcb94e 
Fixes #4944

Trying. to fetch `/zones` fails when it spans more than 500 zones. The fix to use an account id when doing so. This patch passes the account id to the zones call, threading it through all the functions that require it.
threepointone
threepointone commented Jun 30, 2024
View reviewed changes
packages/wrangler/src/dev.tsx
@@ -537,6 +537,9 @@ export async function startDev(args: StartDevOptions) {
args.config ||
(args.script && findWranglerToml(path.dirname(args.script)));

const projectRoot = configPath && path.dirname(configPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this up so I can reference config at line 653
@threepointone threepointone merged commit 4cdad9b into main Jul 2, 2024
21 checks passed
@threepointone threepointone deleted the zones-for-account-id branch July 2, 2024 10:06
@workers-devprod workers-devprod mentioned this pull request Jul 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e Run e2e tests on a PR
2 participants
@threepointone @RamIdeas