-
Notifications
You must be signed in to change notification settings - Fork 829
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
Adding wrangler commands for R2 bucket lock rule configuration #7977
Conversation
🦋 Changeset detectedLatest commit: f53b59d 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 |
219cdf7
to
f348a78
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/13549688131/npm-package-wrangler-7977 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7977/npm-package-wrangler-7977 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-wrangler-7977 dev path/to/script.js Additional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-workers-bindings-extension-7977 -O ./cloudflare-workers-bindings-extension.0.0.0-v00f0bc929.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v00f0bc929.vsix create-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-create-cloudflare-7977 --no-auto-update @cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-kv-asset-handler-7977 miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-miniflare-7977 @cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-pages-shared-7977 @cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-unenv-preset-7977 @cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-vite-plugin-7977 @cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-vitest-pool-workers-7977 @cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-workers-editor-shared-7977 @cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-workers-shared-7977 @cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/13549688131/npm-package-cloudflare-workflows-shared-7977 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
8baf320
to
911107e
Compare
743f60e
to
3b406c5
Compare
would also be good to get some tests for invalid input 🧡 |
7e55bd3
to
5671a91
Compare
c52c71b
to
bbc53c1
Compare
3205495
to
2f4dbe0
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.
Also, its best practice to refrain from squashing your commits/force pushing after a review, so the reviewer can see what has changed since 🧡
If you want to tidy up your commit history you can do that after approval, but also we squash merge to main in this repo anyway so it doesn't really matter a whole lot here 🧡
c6ff6e0
to
135b677
Compare
135b677
to
dffc992
Compare
packages/wrangler/src/r2/lock.ts
Outdated
} | ||
|
||
if (lockDays !== undefined) { | ||
if (!isNaN(lockDays)) { |
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: this is not necessary, because if the input isn't a number yargs will have failed already when its passing the cli args
packages/wrangler/src/r2/lock.ts
Outdated
if (lockDays !== undefined) { | ||
if (!isNaN(lockDays)) { | ||
if (lockDays > 0) { | ||
const conditionDaysValue = Number(lockDays) * 86400; // Convert days to seconds |
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 - yargs will have set the type of lockDays as a number, so there's no need to cast this to a number again. typescript would have let you know if the type was wrong :)
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.
Not sure if it's something with the test setup vs. how wrangler works in practice but it looks like it still reaches there if the type is wrong so I might leave the NaN logic as-is if that's ok
`[Error: Days must be a number.]` |
99db3e4
to
ebaf008
Compare
ebaf008
to
eb2fe2c
Compare
…y making retention required instead of indefinite being implied
eb2fe2c
to
f53b59d
Compare
What this PR solves / how to test
Allows user to configure lock configuration on an R2 bucket