Update non-versioned script settings on deploy#6605
Conversation
🦋 Changeset detectedLatest commit: 6a82bcc The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
c5da324 to
5ed75a8
Compare
|
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/10635013839/npm-package-wrangler-6605You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/6605/npm-package-wrangler-6605Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-wrangler-6605 dev path/to/script.jsAdditional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-create-cloudflare-6605 --no-auto-updatenpm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-cloudflare-kv-asset-handler-6605npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-miniflare-6605npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-cloudflare-pages-shared-6605npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-cloudflare-vitest-pool-workers-6605npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-cloudflare-workers-editor-shared-6605npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/10635013839/npm-package-cloudflare-workers-shared-6605Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
5ed75a8 to
19f630c
Compare
19f630c to
6a82bcc
Compare
| if (props.dryRun) { | ||
| printBindings({ ...withoutStaticAssets, vars: maskedVars }); | ||
| } else { | ||
| assert(accountId, "Missing accountId"); |
There was a problem hiding this comment.
This shouldn't be an assertion. If a user can get themselves in this situation, throw a UserError. An AssertionError is a mistake in our logic and will be sent to Sentry whereas a UserError won't.
There was a problem hiding this comment.
The assertion was just moved from the bottom to fix the type - it probably is something we want in Sentry tbf. Users shouldn't be able to get to the deploy path without an account ID -- we should be prompting or accepting the config value
It is an us issue if neither of those happen
There was a problem hiding this comment.
I see, sorry I thought you introduced it. It should've been an UserError already. The config is already considered. It can only be undefined if not specified in args OR config.
| ); | ||
|
|
||
| // Update tail consumers and logpush settings | ||
| await patchNonVersionedScriptSettings(accountId, scriptName, { |
There was a problem hiding this comment.
Can/should we run the deploy and patch requests in parallel?
There was a problem hiding this comment.
Since this does a deploy we'd have concurrency issues sadly where they'd try to block on eachother and probably more often than not timeout
What this PR solves / how to test
Fixes tail consumer + logpush not updating in new deploy path
Author has addressed the following