-
Notifications
You must be signed in to change notification settings - Fork 79
Add support for direct upload to r2 buckets #705
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
Conversation
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.
Good start. I would be good to add some tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #705 +/- ##
====================================
Coverage 84% 84%
====================================
Files 52 52
Lines 7078 7200 +122
====================================
+ Hits 5957 6076 +119
- Misses 1121 1124 +3 🚀 New features to boost your workflow:
|
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
for more information, see https://pre-commit.ci
…hat. R2FsProvider inherits from S3FsProvider now (duplicate functions removed)
for more information, see https://pre-commit.ci
…ient class type mismatch
for more information, see https://pre-commit.ci
d708a18 to
b9c7a7c
Compare
for more information, see https://pre-commit.ci
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.
Found some testing code that needs to be removed.
Also need to clean up the "magic numbers" scattered in code
And probably base class for the Filesystem would be best
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 great ! Could you add some benchmarks in a follow up.
What does this PR do?
Allows files to be directly uploaded to R2 buckets when the
output_dirpoints to a lightning_storage folder.https://www.loom.com/share/8ef98a2adc0c4f7b86543bab9679882d?sid=387f8cbd-48a2-417e-8058-2346d14736d3
Questions for @tchaton and/or @bhimrazy:
data_connection_idin order to fetch the temporary credentials for the r2 bucket. I wasn't able to find a graceful way of threading that through from theoutput_dirto theget_bucket_and_pathfunction without adding a ton of code, so I just setting an env var and reading from that. This feels very hacky so let me know if you have any recommendations.get_r2_bucket_credentialscall an SDK method instead of making a direct API call. I can do this in a followup PR unless you think it's necessary now.Fixes # (issue).
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in GitHub issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃