Load account ID before uploading cron triggers

[jyn514]

Fixes #2021.

[jyn514]

The error happens because wrangler is loading the global account ID when it isn't set in config.toml, and the test expects it to error instead:

---- settings::toml::tests::deployments::it_errors_on_get_deployments_missing_account_id stdout ----
thread 'settings::toml::tests::deployments::it_errors_on_get_deployments_missing_account_id' panicked at 'assertion failed: manifest.get_deployments(environment).is_err()', src/settings/toml/tests/deployments.rs:99:5

I'm not sure how to fix this. I can't just change the test to expect this to pass, or it will fail for people running cargo test without logging in first. I also can't just let it be since that fails it if you have run wrangler login. My only idea is to sandbox this somehow, so it uses a different config file when running tests than when running normally? This is in an integration test so we'd have to set it externally somehow, either through an API meant only for tests or through an environment variable.

[jyn514]

Something is wrong here. This test was passing before: https://github.com/cloudflare/wrangler/blob/99473b72b72f6251155f089f32fa8eeccbd1875f/src/settings/toml/tests/deployments.rs#L1072
But that test doesn't set the account_id in the toml file anywhere. So it must have been possible for this to work without an account ID before.

[jyn514]

... but it doesn't :/

$ wrangler --version
wrangler 1.17.1-rc.1
$ wrangler publish
🕵️  You can copy your account_id below
+----------------------------+----------------------------------+
| Account Name               | Account ID                       |
+----------------------------+----------------------------------+
| [email protected]'s Account | e1706d218241c1230155b4f91b3218af |
+----------------------------+----------------------------------+
Error: field `account_id` is required to deploy to workers.dev

[jspspike]

I think I know the issue you're having. These tests don't run the entire publish step, they just test get_deployments which just reads the wrangler.toml and spits out things that need to deployed. This doesn't actually deploy them so normally wouldn't need to make an api call. Now for the cron step it will try to load the account_id and since it's not present in the wrangler.toml with your new thing it'll try to make an api call and then fail since it has no credentials

[jyn514]

@jspspike thanks, that was helpful! Here's what I've tried after that:

1. Never load the api_token in tests: 30281d4. That doesn't work because LazyAccountId::load will still try to fetch the account_id, which gives an auth error from Cloudflare's API.
2. Never load the account_id in tests: c3ad80e. That doesn't work because we test that get_deployments fails if there's no account ID :/ I don't know how to keep that test and still have the correct behavior: https://github.com/cloudflare/wrangler/blob/c3ad80efc0f938b40a221dbd93fe448c5cbaf454/src/settings/toml/tests/deployments.rs#L99

[jyn514]

In particular there's a lot of global state around GlobalUser I don't know how to manage. Really I want the account_id to be configurable per-test, but that seems like a really big refactor I'm not sure how to do.

@nilslice @nataliescottdavidson @jspspike do one of you have suggestions?

[jyn514]

A really awful hack I thought of is to add a new #[cfg(test)] fn with_default_account_id(val: Option<String>, f: impl FnOnce) function that uses a Mutex or thread-local to decide what the default should be. But that seems really ugly.

[jyn514]

A really awful hack I thought of is to add a new #[cfg(test)] fn with_default_account_id(val: Option, f: impl FnOnce) function that uses a Mutex or thread-local to decide what the default should be. But that seems really ugly.

This turned out to be both broken and racy 😢

[jspspike]

Never load the account_id in tests: c3ad80e. That doesn't work because we test that get_deployments fails if there's no account ID :/ I don't know how to keep that test and still have the correct behavior:

If this is the only test that is failing, then this change should make it so that you don't need an account_id in you wrangler.toml to get deployments. So it might be safe to remove the test? Ideally replace the test with something that mocks out the api call and still works but we don't have that kind of mocking for tests at the moment. I think someone else on @cloudflare/wrangler should probably decide if that's okay to do

[jyn514]

Ok, I got this working without too many hacks. The way it works is by forcing tests to explicitly write out the account_id in wrangler.toml when they want it to be present; otherwise, get_deployments() will fall back to loading it, which will always error out when cfg(test) is set. That makes sure that:

• The test is reproducible between different users
• The test doesn't depend on what the default value of the account_id is (it can only depend on there being an error if it isn't present)