-
Notifications
You must be signed in to change notification settings - Fork 516
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
Add GCS Fs implementation #296
Conversation
96ed2b2
to
0bb9f71
Compare
996337c
to
74ef818
Compare
@Kargakis Would you maybe interested to take a look into it? |
A notification bump, since it would actually be nice to have it at least looked at 😢 |
Any news on this one? |
Bump. |
I would love to get this merged but I am hesitant to do without e2e tests so I'll defer to @spf13 on the potential to get infra for tests. |
@Kargakis Hey, nice to see some activity here :) What tests are you exactly talking about? New code is actually covered by comprehensive mocked e2e tests, with an ability to switch over to a real bucket for testing, as per this comment. if you have some bucket to use permanently for the sake of testing (with proper timeouts configured, since tests bombard a bucket), then I’d gladly make a real bucket the main testing target. Meanwhile the mocks have been adjusted to mimick the real thing as close as it’s necessary for the sake of testing. |
I meant that the existing tests should be using real GCS buckets and not mocks as part of CI. |
Got it - there's going to be a bit of an issue with that, since some slowing down would have to be introduced to not trigger GCS timeouts with the tests doing so much in so little time. That said, I'm happy to switch to using the real buckets as soon as there is one - I cannot organize it for your project, unfortunately. |
If @spf13 thinks we can merge w/o using GCS I am fine with merging as is, but I would prefer him to sign this off. |
I think we should merge it now, but flag it as experimental until we can establish a more complete end2end test. |
@spf13 thanks for the response! @VOvchinnikov can you please update the README here to document how to configure the use of a GCS bucket? Thanks! |
@spf13 can you be a bit more specific on how to mark this as experimental? Would creating an "experimental" subpackage where we can have gcs suffice? |
No, I'd just add it to the documentation. It's not really usable without
reading the documentation anyway as it requires unique configuration.
…On Mon, Nov 8, 2021 at 9:41 AM Michalis Kargakis ***@***.***> wrote:
@spf13 <https://github.com/spf13> can you be a bit more specific on how
to mark this as experimental? Would creating an "experimental" subpackage
where we can have gcs suffice?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#296 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABKKZCLB76SYRZ7XWSNHCDUK7OSJANCNFSM42ERP5UA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Fixed the merge conflicts and added a README section in #331 |
An implementation of GCS Fs, based on #161
Thanks @Zatte for the inital implementation!
Changes vs the original PR:
master
client
as its base abstraction, notbucket
=> bucket is thus a part of path, the first component in it (!)0755
Bonus:
gs://<bucket>/<path>
URLs too, even though it's explicitly excluded from tests, sinceafero
in general does not work with URLs.The original PR notes, which still apply to this version of the implementation + extra for this version:
Limitations:
Performance implications