-
Notifications
You must be signed in to change notification settings - Fork 337
feat: authenticate calls to preview service when possible #429
Conversation
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.
Instead of splitting the auth and no_auth previews up, keeping one preview function with a function that performs the upload and returns the script Id (and encapsulates the auth_client, checking for user config, etc stuff) will make this much easier to merge with the livereload branch
I am glad that is the case, because that's approximately the refactor i was headed toward! |
b787286
to
f2abfec
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.
This looks like great progress so far! Excited to see the necessary token/email logic (headers on the POST request for preview... unless I'm misunderstanding preview auth works) :)
f2abfec
to
6e3ea47
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.
Just realized after testing on my machine that this currently doesn't account for the case where there is a global user config, but account_id isn't configured. This also breaks tests locally if one has a global user config, since the tests for preview don't configure account_id
oo good point. mine probably pass because i have account id as an environment variable as well. I'll try to update the tests, but also, our tests should maybe set/unset env rather than taking it from execution env? dunno. gonna have to think on that. |
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.
🔥 lgtm
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.
LGTM!
src/commands/publish/preview/mod.rs
Outdated
// so we omit them and provide the user with a little guidance. We don't error out, though, | ||
// because there are valid workarounds for this for testing purposes. | ||
if project.kv_namespaces.is_some() { | ||
message::warn("KV Namespaces are not supported without setting API credentials"); |
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.
Nice!
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.
lgtm!
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.
Breaking changes for newly generated projects. You shouldn't need to use authenticated preview w/o KV
❯ wrangler generate
🌀 🐑 Generating a new webpack worker project with name 'worker'...
🔧 Creating project called `worker`...
✨ Done! New project created /Users/averyharnish/Documents/work/wrangler/worker
❯ cd worker
❯ wrangler preview
Error: Your wrangler.toml is missing the field ["account_id"] which is required to publish to your subdomain!
The quick fix to this (and the general complaint that one has to populate their account id) would be to tell folks to set the appropriate env var. But i'm open to other thoughts, we could potentially fallback to unauthenticated and warn about it the way we do with kv. |
This reverts commit d9c1460.
* use custom validate fn for preview
This might just be a problem with $ wrangler preview
✨ Built successfully, built project size is 517 bytes.
Error: https://api.cloudflare.com/client/v4/accounts/e04d3d27a6829775c8ef937145d7f3ec/workers/scripts/worker/preview: Client Error: 400 Bad Request yuck EDIT: I think this is a separate issue, so I opened one here at #439 |
I also don't get a warning about empty config + falling back to unauthenticated if I don't have a global config set up ❯ rm /Users/averyharnish/.wrangler/config/default.toml
❯ wrangler whoami
Error: ⚠️ Your global config has an error, run `wrangler config`: missing field `email`
❯ wrangler preview
✨ Built successfully, built project size is 517 bytes.
👷 GET https://00000000000000000000000000000000.cloudflareworkers.com
👷 Your worker responded with: Hello worker! This one I think we should probably address here |
I'll check this out today. |
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.
LGTM!
Today Wrangler uses the basic service on cloudflareworkers.com to run its preview command. This PR utilizes the authenticated endpoints, which unlocks desirable features such as kv binding support for wrangler preview specifically.
This PR also includes a refactor to Project, which moves the
validate_project
function out of publish and makes it a public method on Project. In hindsight, preview could probably have used it as a function? It could go either way.Another refactor makes the preview_address a static &str const, as it is always the same, hard coded value.
Finally, I unwrapped the method passed as an arg and passed it unwrapped.
Closes #423 and #431