Don't require an account_id in wrangler.toml #1966

jyn514 · 2021-06-23T19:21:25Z

Helps with #331. I recommend reviewing this commit-by-commit with whitespace changes hidden.

Note that unlike the suggestion in the original issue, this actually never writes to the wrangler.toml at all, which will hopefully make it easier to share Worker projects while still being able to publish them. Instead it will load the account ID from the internet if it's not present, which required a fair bit of refactoring to make it lazily loaded.

If account_id is present in wrangler.toml, use that.
If there is only one ID for the current authentication token, use that.
If there are multiple IDs, give an error and prompt the user to add the id to wrangler.toml.

Here's an example error:

👀  You have multiple accounts.
🕵️  You can copy your account_id below
+--------------------------+----------------------------------+
| Account Name             | Account ID                       |
+--------------------------+----------------------------------+
| Cloudflare Community Jam | 6174b0f52802f24a9f52e7015282898c |
+--------------------------+----------------------------------+
| Edge Workers             | 615f1f0479e7014f0bebcd10d379f10e |
+--------------------------+----------------------------------+
Error: field `account_id` is required

I'm going to save adding support for auto-editing wrangler.toml for a follow-up, this PR is already a little big.

src/settings/toml/manifest.rs

jyn514 · 2021-06-23T19:34:03Z

Find a way to get rid of the Error: text at the end. I think I can do that by adding a custom error type with its own Display impl, and returning that.

No, this hits the same issue as before - display_account_id_maybe prints directly to stdout. I guess I can change that too? It feels pretty intrusive, I'd have to copy the emojis from StdOut.

Another alternative is to call process::exit, but that feels like a heavy hammer.

src/settings/toml/manifest.rs

nataliescottdavidson

Thanks @jyn514 ! closes #331

nataliescottdavidson · 2021-06-28T13:16:18Z

Cargo.toml

 log = "0.4.11"
 notify = "4.0.15"
 number_prefix = "0.4.0"
+once_cell = "1"


Thanks for this change. Reducing dependencies that we don't need is always appreciated.

src/settings/toml/manifest.rs

jyn514 · 2021-06-28T14:41:29Z

@nataliescottdavidson this doesn't affect zone_id, I wasn't sure what the long-term plan for that is. #1941 (comment) mentions it should go away but I haven't looked into it.

nataliescottdavidson · 2021-06-28T16:31:14Z

@nataliescottdavidson this doesn't affect zone_id, I wasn't sure what the long-term plan for that is. #1941 (comment) mentions it should go away but I haven't looked into it.

Yeah, that's on our TODO list, doesn't need to be in this change though.

jyn514 · 2021-06-28T20:41:07Z

Some more things I need to fix:

wrangler generate shouldn't tell you to add an account ID unless you have multiple:

> wrangler generate
 Creating project called `worker`...
 Done! New project created /home/jnelson/src/test-project/worker/worker
🕵️  You can find your zone_id in the right sidebar of a zone's overview tab at https://dash.cloudflare.com
🕵️  You can copy your account_id below
+----------------------------+----------------------------------+
| Account Name               | Account ID                       |
+----------------------------+----------------------------------+
| Jyn514@gmail.com's Account | e1706d218241c1230155b4f91b3218af |
+----------------------------+----------------------------------+
🕵️  You will need to update the following fields in the created wrangler.toml file before continuing:
- account_id

wrangler preview should require an account ID instead of defaulting to an empty string (this is a pre-existing bug):

> wrangler preview
Error: Something went wrong with the request to Cloudflare...
Could not route to /accounts/workers/scripts/worker/preview, perhaps your object identifier is invalid? [API code: 7003]
No route for that URI [API code: 7000]

jyn514 · 2021-06-28T20:50:40Z

wrangler preview should require an account ID instead of defaulting to an empty string (this is a pre-existing bug):

Hmm, I couldn't replicate this error. Maybe it was a transient error on the server side?

jyn514 · 2021-06-28T20:53:31Z

I managed to replicate it - the issue is that this mishandles account_id = "" and assumes it's a valid ID. That's very unfortunate, because everyone with a fresh wrangler generate project has account_id = "". I can change generate to stop doing that, but it won't fix it for the projects that have already been created.

jyn514 · 2021-06-28T21:07:31Z

Hmm, with that in mind I wonder if it still makes sense to use Option<String>. It might make sense to keep using "" as the "null" value that means not loaded and change all the None values to "".

nataliescottdavidson · 2021-06-29T13:39:10Z

I managed to replicate it - the issue is that this mishandles account_id = "" and assumes it's a valid ID. That's very unfortunate, because everyone with a fresh wrangler generate project has account_id = "". I can change generate to stop doing that, but it won't fix it for the projects that have already been created.

Hmm, with that in mind I wonder if it still makes sense to use Option. It might make sense to keep using "" as the "null" value that means not loaded and change all the None values to "".

I don't think we want to go back to "" for null, your implementation is much nicer. Could we add a check in here that errors / prompts account id if the account id in wrangler.toml is ""?

jyn514 · 2021-06-29T13:41:27Z

@nataliescottdavidson I changed it to treat the empty string just as if it were unset, which should hopefully prompt the user less: https://github.com/cloudflare/wrangler/pull/1966/files#diff-c82f6d0441c7f8ad03e619c809138bbb7e0ca631652e6ad8c3c2edf43c5fbe74R527-R529. I can change it to give a hard error if you like but I'm not sure how useful it would be.

caass

Find a way to get rid of the Error: text at the end.

Can you tell where this is coming from?

Should I add support for auto-editing wrangler.toml? (I would slightly prefer to have it go in a follow-up, this PR is already a little big.)

I agree it should be a follow-up -- a good idea, though.

Can the serde impls be simplified? (see below)

They look good to me -- see below

Should the error be different when environment files are loaded? (see below)

I'm not sure what you mean by this :/

Another alternative is to call process::exit, but that feels like a heavy hammer.

I kind of agree, but I also think it's not that bad if there's a comment above explaining why process::exit. If we run into problems later, it's a one-liner to revert.

I think this looks good!

caass · 2021-06-30T23:52:29Z

src/deploy/zoneless.rs

-            // TODO: Deserialize empty strings to None; cannot do this for account id
-            // yet without a large refactor.


squidward_future.gif

src/settings/toml/manifest.rs

jyn514 · 2021-07-01T14:13:05Z

@caass and I discussed this on a call last night.

Error: without anything following is now Error: field `account_id` is required, which looks reasonably intentional. This also makes the process::exit thing moot.
@nataliescottdavidson already looked at the messages around environment files, I just forgot to remove it from the PR description.

nilslice · 2021-07-01T14:23:08Z

@jyn514, thanks for the work put into this. Can you check that in places where you're printing errors, that:

they go to stderr
we exit with a non-zero exit code (so in cases like CI pipelines, the build breaks intentionally after a wrangler related issue)

jyn514 · 2021-07-01T14:56:17Z

@nilslice when you say check, do you mean with an automated test? or just locally?

nilslice · 2021-07-01T14:58:03Z

@jyn514 it would be nice to have some automation here, but probably out of scope for your PR. Just a manual pass to ensure this is the case.

I haven't reviewed any code yet, but a manual spot check would be great.

jyn514 · 2021-07-01T15:06:29Z

Hmm, so I'm printing errors to stderr, but the account info to stdout, because that's how wrangler whoami behaves. Is that ok?

It does have a non-zero exit code.

> wrangler publish 2>/dev/null
👀  You have multiple accounts.
🕵️  You can copy your account_id below
+--------------------------+----------------------------------+
| Account Name             | Account ID                       |
+--------------------------+----------------------------------+
| Cloudflare Community Jam | 6174b0f52802f24a9f52e7015282898c |
+--------------------------+----------------------------------+
| Edge Workers             | 615f1f0479e7014f0bebcd10d379f10e |
+--------------------------+----------------------------------+
> wrangler publish >/dev/null
Error: field `account_id` is required
> echo $?
1

`OnceCell` is needed later, and this avoids having two dependencies that do the same thing. Wrangler has dependencies that use both, so this doesn't actually reduce compile times, but it will make it clear which library to use.

Previously, it was always `Some` and the empty string was used to see if it wasn't filled out. This removes unnecessary checking for the empty string.

This makes it explicit where the account ID is required and where it's treated as the empty string.

This collects all handling of the account_id in one place, so that errors no longer have to be handled at each call-site. Making this a new type, rather than a function on `Manifest`, allows it to be used for other types as well, such as `Target`. Using a `OnceCell` rather than an `Option` has two benefits: 1. It does not allow setting the account twice, which would be a bug. 2. It allows setting the account with only an immutable reference, which reduces the amount of churn in calling functions. Note that this still deserializes the empty string as not having an `account_id` for backwards compatibility.

This avoid the following test failure: ``` it_builds_with_webpack_target_webworker_with_custom_file stdout ---- Created fixture at /tmp/.tmpSzLIJb thread 'it_builds_with_webpack_target_webworker_with_custom_file' panicked at 'Build failed: Output { status: ExitStatus(ExitStatus(256)), stdout: "👀 You have multiple accounts.\nPlease choose one from below and add it to `wrangler.toml` under `account_id`.\n🕵\u{fe0f} You can copy your account_id below\n+--------------------------+----------------------------------+\n| Account Name | Account ID |\n+--------------------------+----------------------------------+\n| Cloudflare Community Jam | 6174b0f52802f24a9f52e7015282898c |\n+--------------------------+----------------------------------+\n| Edge Workers | 615f1f0479e7014f0bebcd10d379f10e |\n+--------------------------+----------------------------------+\n", stderr: "Error: \n" }', tests/build.rs:377:5 ``` It also prevents any similar issues that aren't tested, by forcing each command that wants an account ID to load it explicitly.

Previews have different behavior when an account is available than when it isn't. This restores that previous behavior: if an account is available, use authenticated previews, otherwise unauthenticated previews. Note that "an account is available" has become a slightly tricky question, since it's no longer guaranteed that it will be in the wrangler.toml. Instead, this checks if it is either in the .toml or if there is only a single account available to the authentication token.

This only affects `wrangler report`. The old behavior preserved the behavior from before, but was less helpful because it wouldn't include the ID if there was only a single ID for the account token.

It no longer does anything now that the account ID is loaded on demand.

…ate` Before, wrangler would give this message: ``` > wrangler generate Creating project called `worker`... Done! New project created /home/jnelson/src/test-project/worker/worker 🕵️ You can find your zone_id in the right sidebar of a zone's overview tab at https://dash.cloudflare.com 🕵️ You can copy your account_id below +----------------------------+----------------------------------+ | Account Name | Account ID | +----------------------------+----------------------------------+ | Jyn514@gmail.com's Account | e1706d218241c1230155b4f91b3218af | +----------------------------+----------------------------------+ 🕵️ You will need to update the following fields in the created wrangler.toml file before continuing: - account_id ``` Adding the account ID is no longer necessary, so don't suggest adding it.

jyn514 requested a review from a team as a code owner June 23, 2021 19:21

jyn514 commented Jun 23, 2021

View reviewed changes

src/settings/toml/manifest.rs Outdated Show resolved Hide resolved

src/settings/toml/manifest.rs Outdated Show resolved Hide resolved

jyn514 force-pushed the jnelson/EW-5071-no-account-id branch from b8b9718 to 134e788 Compare June 23, 2021 20:00

nilslice reviewed Jun 23, 2021

View reviewed changes

src/settings/toml/manifest.rs Outdated Show resolved Hide resolved

jyn514 force-pushed the jnelson/EW-5071-no-account-id branch 2 times, most recently from ce5d170 to 4db52c4 Compare June 23, 2021 20:29

nataliescottdavidson approved these changes Jun 28, 2021

View reviewed changes

nataliescottdavidson linked an issue Jun 28, 2021 that may be closed by this pull request

WIP RFC: Automatically configure account_id and zone_id #331

Closed

jyn514 force-pushed the jnelson/EW-5071-no-account-id branch from 4db52c4 to c532063 Compare June 28, 2021 16:25

nataliescottdavidson requested a review from caass June 28, 2021 16:31

jyn514 force-pushed the jnelson/EW-5071-no-account-id branch 2 times, most recently from db569ab to 3958957 Compare June 28, 2021 21:29

jyn514 changed the title ~~[WIP] Don't require an account_id in wrangler.toml~~ Don't require an account_id in wrangler.toml Jun 28, 2021

caass approved these changes Jul 1, 2021

View reviewed changes

jyn514 added the feature Feature requests and suggestions label Jul 1, 2021

Switch from lazy_static to once_cell

64bcd16

`OnceCell` is needed later, and this avoids having two dependencies that do the same thing. Wrangler has dependencies that use both, so this doesn't actually reduce compile times, but it will make it clear which library to use.

jyn514 and others added 9 commits July 1, 2021 15:40

Use the None variant of RouteId::account_id

89e030e

Previously, it was always `Some` and the empty string was used to see if it wasn't filled out. This removes unnecessary checking for the empty string.

Use an Option for account_id

d278ea7

This makes it explicit where the account ID is required and where it's treated as the empty string.

Handle multiple accounts

a86b7a6

Use maybe_load instead of if_present for load_project_info

1ee1f42

This only affects `wrangler report`. The old behavior preserved the behavior from before, but was less helpful because it wouldn't include the ID if there was only a single ID for the account token.

Remove unneeded validate function

dafdc29

It no longer does anything now that the account ID is loaded on demand.

jyn514 force-pushed the jnelson/EW-5071-no-account-id branch from 3958957 to 47eecb3 Compare July 1, 2021 19:40

jyn514 merged commit 3e8d9b2 into master Jul 1, 2021

delete-merged-branch bot deleted the jnelson/EW-5071-no-account-id branch July 1, 2021 19:56

jyn514 mentioned this pull request Jul 9, 2021

Temporary workaround to avoid dropping a tokio::runtime while another is running #1991

Merged

jyn514 mentioned this pull request Jul 23, 2021

Don't panic when the user isn't logged in #2012

Merged

jyn514 mentioned this pull request Aug 9, 2021

Error message is unhelpful when multiple KV stores are bound to the same variable #2020

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't require an account_id in wrangler.toml #1966

Don't require an account_id in wrangler.toml #1966

jyn514 commented Jun 23, 2021 •

edited

jyn514 commented Jun 23, 2021

nataliescottdavidson left a comment •

edited

nataliescottdavidson Jun 28, 2021

jyn514 commented Jun 28, 2021

nataliescottdavidson commented Jun 28, 2021

jyn514 commented Jun 28, 2021 •

edited

jyn514 commented Jun 28, 2021

jyn514 commented Jun 28, 2021

jyn514 commented Jun 28, 2021

nataliescottdavidson commented Jun 29, 2021

jyn514 commented Jun 29, 2021 •

edited

caass left a comment •

edited

caass Jun 30, 2021

jyn514 commented Jul 1, 2021 •

edited

nilslice commented Jul 1, 2021

jyn514 commented Jul 1, 2021

nilslice commented Jul 1, 2021

jyn514 commented Jul 1, 2021 •

edited

		// TODO: Deserialize empty strings to None; cannot do this for account id
		// yet without a large refactor.

Don't require an account_id in wrangler.toml #1966

Don't require an account_id in wrangler.toml #1966

Conversation

jyn514 commented Jun 23, 2021 • edited

jyn514 commented Jun 23, 2021

nataliescottdavidson left a comment • edited

Choose a reason for hiding this comment

nataliescottdavidson Jun 28, 2021

Choose a reason for hiding this comment

jyn514 commented Jun 28, 2021

nataliescottdavidson commented Jun 28, 2021

jyn514 commented Jun 28, 2021 • edited

jyn514 commented Jun 28, 2021

jyn514 commented Jun 28, 2021

jyn514 commented Jun 28, 2021

nataliescottdavidson commented Jun 29, 2021

jyn514 commented Jun 29, 2021 • edited

caass left a comment • edited

Choose a reason for hiding this comment

caass Jun 30, 2021

Choose a reason for hiding this comment

jyn514 commented Jul 1, 2021 • edited

nilslice commented Jul 1, 2021

jyn514 commented Jul 1, 2021

nilslice commented Jul 1, 2021

jyn514 commented Jul 1, 2021 • edited

jyn514 commented Jun 23, 2021 •

edited

nataliescottdavidson left a comment •

edited

jyn514 commented Jun 28, 2021 •

edited

jyn514 commented Jun 29, 2021 •

edited

caass left a comment •

edited

jyn514 commented Jul 1, 2021 •

edited

jyn514 commented Jul 1, 2021 •

edited