-
Notifications
You must be signed in to change notification settings - Fork 78
feat(cloudflare): add optional R2 batch uploads via rclone for cache population #925
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(cloudflare): add optional R2 batch uploads via rclone for cache population #925
Conversation
🦋 Changeset detectedLatest commit: 3ec1364 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@krzysztof-palka-monogo thank you so much for this PR, it will please many of our users! I only took a quick look and I'll do a full review on Monday. I have some preliminary comments/thoughts:
|
|
Good morning @vicb Here’s the justification for keeping an explicit
|
We definitely don't want "rclone" in the flag. We could use another mechanism, i.e. remote bindings.
IMO we should drop the need for an external config in favor of using env vars - the CLI could dynamically create a temp config file for rclone.js as it doesn't seem to support env vars.
We can use rclone when configured and fallback to the current mechanism - but log that there is faster way and link some docs.
I disagree here. |
|
Hi @vicb I've updated the PR to align with your suggestions:
Documentation now focuses on "batch upload" as an optional performance feature, not implementation details. Please let me know if thats will meet the requirements and if so could we run "Publish prereleases" action to properly test the changes in some real applications 😄 |
commit: |
|
Hi @vicb I will be glad for code review. |
vicb
left a comment
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.
Note sure I'll have time for a full review today, sorry.
A couple comments:
-
R2_ACCOUNT_IDshould rather beCLOUDFLARE_ACCOUNT_IDas it is not specific to R2 -
Thanks for implementing the env vars based solution for CI. I'm wondering if we can improve the story for local dev. It would be nice to get the vars from
.env/.dev.varsfiles but it would mean adding vars that are only used in local dev and that might confuse users. Maybe we could look for a local<pjt>/rclone.confand fallback to env vars when it's not here. What do you think?
|
Hello @vicb I've addressed both points:
This keeps the implementation simple while making local dev more convenient. WDYT? |
I don't think there is an ideal solution for That being said, Next.js uses .env files with this loading order so we should make sure we are consistent. I'll take a deeper look today and comment on how we could do that |
|
I’ll be away for a week starting tomorrow, so I need to wrap up this functionality today 😅 |
…D to CF_ACCOUNT_ID
…sequential uploads
vicb
left a comment
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.
Added a few nits.
The main point is that I think we should use the npm package if it supports copy.
Thanks for all your work on this!
|
Hello @vicb I addressed all the comments. |
I think there is one opened comment about rclone.promises |
|
@vicb could you please clarify what changes exactly you are requesting? |
|
I’m honestly starting to feel a bit sabotaged 😞 |
Either it's abandonned and we should not use it or we use it. The alternative is to use any of the other more recent package on npm.
Sorry you feel that way. |
vicb
left a comment
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.
Many thanks @krzysztof-palka-monogo for the PR and enduring my reviews :)
|
Huge relief :) Thanks a lot for the approval and merge! |
Batch upload R2 cache using rclone
This PR proposes adding support for optional batch uploading R2 cache using rclone.
Based on the solution discussed in Issue #866
Details
Automatic selection between batch vs standard upload
The CLI now detects when all of
R2_ACCESS_KEY_ID,R2_SECRET_ACCESS_KEY, andCLOUDFLARE_ACCOUNT_IDare set in the environment and switches to batch upload mode. Otherwise, it falls back to the standard (Wrangler-based) upload method.Graceful fallback on errors
If batch upload fails (e.g. due to rclone errors), the flow will log a warning and transparently revert to the default upload approach.
Staging and parallel transfers
Assets are copied into a temporary staging directory, then uploaded using rclone copy with concurrency settings (
--transfers=32,--checkers=16) for performance.Related issues