fix: respect config.account_id in wrangler dev --remote #6963

RamIdeas · 2024-10-14T14:11:38Z

What this PR solves / how to test

Fixes #6947

Author has addressed the following

Tests
- TODO (before merge)
- Tests included
- Tests not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Changeset included
- Changeset not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Documentation not necessary because: fix

changeset-bot · 2024-10-14T14:11:43Z

🦋 Changeset detected

Latest commit: c08ea41

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

RamIdeas · 2024-10-14T14:13:20Z

packages/wrangler/src/dev.tsx

-								accountId = await requireAuth({});
+								accountId = await requireAuth(config);


main fix for wrangler dev --remote

RamIdeas · 2024-10-14T14:13:55Z

packages/wrangler/src/api/startDevWorker/ConfigController.ts

+		}
+
+		return {
+			accountId: config.account_id ?? (await getAccountId()),


fix default for startWorker

Does this mean config has a precedent over the env variable?

This should probably use the requireAuth() helper

This should probably use the requireAuth() helper

5396ab5

Does this mean config has a precedent over the env variable?

Yes. Which matches previous behaviour unless I'm mistaken?

Yes. Which matches previous behaviour unless I'm mistaken

So it does, that behaviour is surprising

I don't think it's surprising. IMO order of precedence: cli args > config file > env vars, because env vars can be set across projects, config file per project, cli args per command – the specificity is clear.

I can understand setting an env var inline and expecting that to take precedence (eg CLOUDFLARE_ACCOUNT_ID=1234 npx wrangler deploy) but instead the cli arg should be used for that use-case (eg. npx wrangler deploy --account-id 1234) because that env var could've come from the eg ~/.zshrc file

I've never seen --account-id before - it doesn't seem to be documented 🙃

Huh you're right! We should add one to support per command overrides with higher precedence than the config file

RamIdeas · 2024-10-14T14:14:34Z

packages/wrangler/src/api/startDevWorker/ConfigController.ts

+		if (input.dev?.auth) {
+			return unwrapHook(input.dev.auth, config);
+		}


provide (wrangler.toml) config to the hook fn as param

github-actions · 2024-10-14T14:15:13Z

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/11363964648/npm-package-wrangler-6963

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-wrangler-6963 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-create-cloudflare-6963 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-cloudflare-kv-asset-handler-6963

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-miniflare-6963

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-cloudflare-pages-shared-6963

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-cloudflare-vitest-pool-workers-6963

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-cloudflare-workers-editor-shared-6963

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11363964648/npm-package-cloudflare-workers-shared-6963

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

wrangler@3.80.4 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20241004.0
`workerd`	1.20241011.1	1.20241011.1
`workerd --version`	1.20241011.1	2024-10-11

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

RamIdeas · 2024-10-14T14:15:34Z

packages/wrangler/src/api/startDevWorker/types.ts

@@ -129,7 +129,7 @@ export interface StartDevWorkerInput {
 		/** Whether the worker runs on the edge or locally. */
 		remote?: boolean;
 		/** Cloudflare Account credentials. Can be provided upfront or as a function which will be called only when required. */
-		auth?: AsyncHook<CfAccount>;
+		auth?: AsyncHook<CfAccount, [Pick<Config, "account_id">]>; // provide config.account_id as a hook param


only the ConfigController has access to the wrangler.toml config so enable it as a hook param for StartDevWorkerInput.dev.auth

RamIdeas · 2024-10-14T14:16:25Z

packages/wrangler/src/api/startDevWorker/types.ts

@@ -187,6 +187,7 @@ export type StartDevWorkerOptions = Omit<StartDevWorkerInput, "assets"> & {
 	};
 	dev: StartDevWorkerInput["dev"] & {
 		persist: string;
+		auth?: AsyncHook<CfAccount>; // redefine without config.account_id hook param (can only be provided by ConfigController with access to wrangler.toml, not by other controllers eg RemoteRuntimeContoller)


Other controllers won't have access to the wrangler.toml config file so redefine this without a hook param on StartDevWorkerOptions.dev.auth

How will this work if e.g. the remote runtime controller unwraps the hook? Will that not fail?

Only the ConfigController unwraps this hook. The ConfigController overwrites this property with a new hook that does not expect a param – this new hook is then unwrapped by the RemoteRuntimeController

penalosa · 2024-10-14T14:24:34Z

packages/wrangler/src/api/startDevWorker/ConfigController.ts

-		});
+	const auth = async () => {
+		if (input.dev?.auth) {
+			return unwrapHook(input.dev.auth, config);


I'm not sure this is the same behaviour. This seems to unconditionally unwrap the hook if provided, even if not in remote mode?

It doesn't unconditionally unwrap the hook. It unconditionally sets the auth property with its own hook which will unwrap the original property when itself is unwrapped. Therefore it is still conditionally unwrapped as before

(This diff answers your other question. https://github.com/cloudflare/workers-sdk/pull/6963/files#r1799613869)

penalosa · 2024-10-14T14:25:08Z

packages/wrangler/src/api/startDevWorker/types.ts

@@ -187,6 +187,7 @@ export type StartDevWorkerOptions = Omit<StartDevWorkerInput, "assets"> & {
 	};
 	dev: StartDevWorkerInput["dev"] & {
 		persist: string;
+		auth?: AsyncHook<CfAccount>; // redefine without config.account_id hook param (can only be provided by ConfigController with access to wrangler.toml, not by other controllers eg RemoteRuntimeContoller)


How will this work if e.g. the remote runtime controller unwraps the hook? Will that not fail?

penalosa

Approved pending https://github.com/cloudflare/workers-sdk/pull/6963/files#r1800710064 and the TODO test

RamIdeas requested a review from a team as a code owner October 14, 2024 14:11

fix: respect config.account_id in wrangler dev --remote

a473b1c

RamIdeas force-pushed the fix-config-accountid-dev-remote branch from 6fd39ba to a473b1c Compare October 14, 2024 14:12

RamIdeas commented Oct 14, 2024

View reviewed changes

penalosa reviewed Oct 14, 2024

View reviewed changes

penalosa approved these changes Oct 15, 2024

View reviewed changes

RamIdeas added 3 commits October 15, 2024 17:44

use requireAuth instead of getAccountId

5396ab5

add tests

5bd123a

add changeset

ad85fed

RamIdeas added the e2e Run e2e tests on a PR label Oct 15, 2024

fix lint

c08ea41

RamIdeas merged commit a5ac45d into main Oct 16, 2024
22 checks passed

RamIdeas deleted the fix-config-accountid-dev-remote branch October 16, 2024 15:12

workers-devprod mentioned this pull request Oct 16, 2024

Version Packages #6997

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: respect config.account_id in wrangler dev --remote #6963

fix: respect config.account_id in wrangler dev --remote #6963

RamIdeas commented Oct 14, 2024 •

edited

Loading

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

RamIdeas Oct 14, 2024

RamIdeas Oct 14, 2024

Skye-31 Oct 14, 2024

penalosa Oct 15, 2024

RamIdeas Oct 15, 2024

Skye-31 Oct 15, 2024 •

edited

Loading

RamIdeas Oct 16, 2024

emily-shen Oct 16, 2024

RamIdeas Oct 16, 2024

RamIdeas Oct 14, 2024

github-actions bot commented Oct 14, 2024 •

edited

Loading

RamIdeas Oct 14, 2024

RamIdeas Oct 14, 2024 •

edited

Loading

penalosa Oct 14, 2024

RamIdeas Oct 14, 2024 •

edited

Loading

penalosa Oct 14, 2024

RamIdeas Oct 14, 2024 •

edited

Loading

penalosa Oct 14, 2024

penalosa left a comment

		accountId = await requireAuth({});
		accountId = await requireAuth(config);

fix: respect config.account_id in wrangler dev --remote #6963

fix: respect config.account_id in wrangler dev --remote #6963

Conversation

RamIdeas commented Oct 14, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Oct 14, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skye-31 Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

RamIdeas Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RamIdeas Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

penalosa left a comment

Choose a reason for hiding this comment

RamIdeas commented Oct 14, 2024 •

edited

Loading

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

Skye-31 Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 14, 2024 •

edited

Loading

RamIdeas Oct 14, 2024 •

edited

Loading

RamIdeas Oct 14, 2024 •

edited

Loading

RamIdeas Oct 14, 2024 •

edited

Loading