Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Support prefix path for R2 buckets #211

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

jakelazaroff
Copy link
Contributor

Finally played around with Y-Sweet and it's really slick! Local testing was a breeze and it was super easy to deploy on Cloudflare Workers.

One area in which the R2 support doesn't seem to have parity with S3 (afaict) is in using a bucket prefix. This PR adds that — slightly awkwardly, as it reuses the S3_BUCKET_PREFIX environment variable even though it's not an S3 bucket. Happy to change the PR if you have a better way to do this!

Copy link

vercel bot commented Feb 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
y-sweet-debugger ⬜️ Ignored (Inspect) Visit Preview Feb 20, 2024 2:27pm
y-sweet-demos ⬜️ Ignored (Inspect) Visit Preview Feb 20, 2024 2:27pm
y-sweet-gendocs ⬜️ Ignored (Inspect) Visit Preview Feb 20, 2024 2:27pm

Copy link

vercel bot commented Feb 17, 2024

@jakelazaroff is attempting to deploy a commit to the drifting-corp Team on Vercel.

A member of the Team first needs to authorize it.

@paulgb
Copy link
Member

paulgb commented Feb 17, 2024

Cool, thanks Jake! Will take a look soon.

@paulgb
Copy link
Member

paulgb commented Feb 20, 2024

Ok, this looks good! I'm going to LGTM it as is, but I took some notes from my code dive for my future self:

  • The schema of the Configuration object pre-dates support for S3 on the Cloudflare worker implementation of Y-Sweet.
  • When we added support for S3, it was by dropping in an optional S3Config object.
  • Both the Configuration and S3Config objects support an optional bucket_prefix. Currently, Y-Sweet never sets the bucket_prefix of Configuration, but it can be used by code that calls Y-Sweet as a library (including the Y-Sweet Cloud platform).
  • If S3Config is Some(_), the bucket_prefix of Configuration is set

This PR is a correct approach, and I think it's the best we can do without a breaking API change. But it does make me realize that for library users of y-sweet-worker, our current API is confusing, because it potentially allows bucket_prefix to be set in two places and only one of them is used.

A potential follow-up here would be to have a BucketConfig enum with S3(S3Config) as one option an R2 {bucket_prefix: Option<String>} as another. This change would break the API for library consumers.

@paulgb paulgb merged commit 8510220 into jamsocket:main Feb 20, 2024
8 checks passed
@jakelazaroff jakelazaroff deleted the r2-bucket-prefix branch February 20, 2024 22:14
@jakelazaroff
Copy link
Contributor Author

(Y-)Sweet, thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants