-
Notifications
You must be signed in to change notification settings - Fork 337
Don't require an account_id in wrangler.toml #1966
Conversation
No, this hits the same issue as before - Another alternative is to call |
b8b9718
to
134e788
Compare
ce5d170
to
4db52c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log = "0.4.11" | ||
notify = "4.0.15" | ||
number_prefix = "0.4.0" | ||
once_cell = "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this change. Reducing dependencies that we don't need is always appreciated.
@nataliescottdavidson this doesn't affect |
4db52c4
to
c532063
Compare
Yeah, that's on our TODO list, doesn't need to be in this change though. |
Some more things I need to fix:
|
Hmm, I couldn't replicate this error. Maybe it was a transient error on the server side? |
I managed to replicate it - the issue is that this mishandles |
Hmm, with that in mind I wonder if it still makes sense to use |
db569ab
to
3958957
Compare
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 |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
// TODO: Deserialize empty strings to None; cannot do this for account id | ||
// yet without a large refactor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
squidward_future.gif
@caass and I discussed this on a call last night.
|
@jyn514, thanks for the work put into this. Can you check that in places where you're printing errors, that:
|
@nilslice when you say check, do you mean with an automated test? or just locally? |
@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. |
Hmm, so I'm printing errors to stderr, but the account info to stdout, because that's how It does have a non-zero exit code.
|
`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 | +----------------------------+----------------------------------+ | [email protected]'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.
3958957
to
47eecb3
Compare
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.
account_id
is present inwrangler.toml
, use that.Here's an example error:
I'm going to save adding support for auto-editing wrangler.toml for a follow-up, this PR is already a little big.