Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add support for publishing workers with r2 bucket bindings #452

Merged
merged 2 commits into from
Feb 13, 2022
Merged

feat: add support for publishing workers with r2 bucket bindings #452

merged 2 commits into from
Feb 13, 2022

Conversation

petebacondarwin
Copy link
Contributor

This change adds the ability to define bindings in your wrangler.toml file
for R2 buckets. These buckets will then be available in the environment
passed to the worker at runtime.

Closes #365

@changeset-bot
Copy link

changeset-bot bot commented Feb 12, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 69639d1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
wrangler Patch

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

@petebacondarwin petebacondarwin mentioned this pull request Feb 12, 2022
Closed
5 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Feb 12, 2022 •
edited
Loading

wrangler prerelease is available for testing:

npm install --save-dev https://prerelease-registry.developers.workers.dev/runs/1836269986/wrangler

threepointone
threepointone requested changes Feb 12, 2022
View reviewed changes
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Could you add it to wrangler dev as well? That would be needed to close the issue.

packages/wrangler/src/config.ts
@@ -203,6 +203,15 @@ export type Config = {
preview_id?: string;
}[];

r2_buckets?: {
/** The binding name used to refer to the R2 bucket in the worker. */
binding: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really dislike "binding" bah. I'll make some noise about it later.

@threepointone
Copy link
Contributor

You could also add wrangler dev support in another PR, but then remove the "Closes" directive from the PR description, and I'll stamp this.

@petebacondarwin
Copy link
Contributor Author

That is so weird. I actually ran wrangler dev on my test project and I saw the R2 binding in the worker....
But looking through the code, again, I can see that it is not being added at the <Dev> component...
Is there any reason why dev extracts these bindings differently to how publish? Perhaps there is an opportunity for more de-duplication there?

@petebacondarwin
Copy link
Contributor Author

Looking through the code where toFormData() is used, it seems that we also need to update when submitting a secret. (Not quite sure why... is that related to the issue before of setting secrets where wiping out other environment variables?)

@petebacondarwin
feat: add support for publishing workers with r2 bucket bindings
ab824ad 
This change adds the ability to define bindings in your `wrangler.toml` file
for R2 buckets. These buckets will then be available in the environment
passed to the worker at runtime.

Closes #365
@petebacondarwin
fixup! feat: add support for publishing workers with r2 bucket bindings
69639d1
@petebacondarwin
Copy link
Contributor Author

Added the bindings to wrangler dev I'll check it manually later today.
I also changed the CfWorkerInit so that each of the bindings properties were required (but could still be undefined) so as to ensure we don't miss this again.
@threepointone - PTAL.

threepointone
threepointone approved these changes Feb 13, 2022
View reviewed changes
Copy link
Contributor

@threepointone threepointone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@threepointone
Copy link
Contributor

That is so weird. I actually ran wrangler dev on my test project and I saw the R2 binding in the worker....

But looking through the code, again, I can see that it is not being added at the <Dev> component...

Is there any reason why dev extracts these bindings differently to how publish? Perhaps there is an opportunity for more de-duplication there?

This is scary. I hope R2 bindings aren't being exposed if they're simply published. We should verify this next week.

@petebacondarwin petebacondarwin merged commit 1cf6701 into main Feb 13, 2022
@petebacondarwin petebacondarwin deleted the r2-publish branch February 13, 2022 08:39
@threepointone
Copy link
Contributor

Is there any reason why dev extracts these bindings differently to how publish? Perhaps there is an opportunity for more de-duplication there?

Probably! It'll also force people to ship support for both whenever they make a new type of binding.

@github-actions github-actions bot mentioned this pull request Feb 13, 2022
Merged
This was referenced Sep 1, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@threepointone threepointone threepointone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

r2 bucket support
2 participants
@petebacondarwin @threepointone