-
Notifications
You must be signed in to change notification settings - Fork 782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add provisioning to versions upload and reprovision on bucket jurisdiction changes #7889
Conversation
🦋 Changeset detectedLatest commit: 0e311e4 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 |
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/13032244342/npm-package-wrangler-7889 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7889/npm-package-wrangler-7889 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-wrangler-7889 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-workers-bindings-extension-7889 -O ./cloudflare-workers-bindings-extension.0.0.0-vd11e670fb.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-vd11e670fb.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-create-cloudflare-7889 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-kv-asset-handler-7889 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-miniflare-7889 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-pages-shared-7889 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-unenv-preset-7889 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-vite-plugin-7889 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-vitest-pool-workers-7889 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-workers-editor-shared-7889 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-workers-shared-7889 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13032244342/npm-package-cloudflare-workflows-shared-7889 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
21a58b5
to
774721e
Compare
packages/wrangler/src/d1/utils.ts
Outdated
@@ -46,7 +46,7 @@ export const getDatabaseByNameOrBinding = async ( | |||
return dbFromConfig; | |||
} | |||
|
|||
const allDBs = await listDatabases(accountId); | |||
const allDBs = await listDatabases(accountId, true); |
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.
Do we care that limitCalls = true
changes technically breaks existing behaviour a bit? Previously this would (slowly) get all databases, now this only gets 30.
can we pass in a larger page size (it's 10 by default) - i tested it at 100 and it didn't seem to be noticably slower.
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 is a bit of a hack for now. Once the new API is ready we can switch over to that, which should be today or tomorrow
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.
Is the new API ready yet? Do we wanna wait for it or merge this as is?
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.
It's ready now, so I'll update this PR to use it now
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.
Updated
}, | ||
{ once: true } | ||
} | ||
// { once: true } |
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.
?
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.
Removed
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.
Looks good to me. Just two minor comments.
packages/wrangler/src/d1/utils.ts
Outdated
@@ -46,7 +46,7 @@ export const getDatabaseByNameOrBinding = async ( | |||
return dbFromConfig; | |||
} | |||
|
|||
const allDBs = await listDatabases(accountId); | |||
const allDBs = await listDatabases(accountId, true); |
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.
Is the new API ready yet? Do we wanna wait for it or merge this as is?
getFlag("RESOURCES_PROVISION") | ||
? await provisionBindings( | ||
bindings, | ||
accountId, | ||
scriptName, | ||
props.experimentalAutoCreate, | ||
props.config | ||
) | ||
: null; |
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.
Nit: Just find the syntax a little bit strange 😅
getFlag("RESOURCES_PROVISION") | |
? await provisionBindings( | |
bindings, | |
accountId, | |
scriptName, | |
props.experimentalAutoCreate, | |
props.config | |
) | |
: null; | |
if (getFlag("RESOURCES_PROVISION")) { | |
await provisionBindings( | |
bindings, | |
accountId, | |
scriptName, | |
props.experimentalAutoCreate, | |
props.config | |
); | |
} |
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.
Fixed
87ed74f
to
04f1a47
Compare
.changeset/neat-sloths-reply.md
Outdated
"wrangler": patch | ||
--- | ||
|
||
feat: add resource auto-provisioning to versions upload |
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.
Should we make it clear that this is still experimental and requires an experimental flag
Allows resource provisioning in versions upload.
Also allow switching jurisdictions (since apparently buckets with the same name can exist in different jurisdictions).