Native S3 transfer driver #1010
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add all the methods I think it needs - Re-order them alphabetically - Work on how to configure it
Worked with Reid and Peter to get the S3 transfer class connecting to S3, and recusively LS-ing objects in a bucket. It also inherently gets `__credentials`, and `cd` working too.
It does send, the paths are weird, and I don't know what they're supposed to do. I suspect there's some changes to make.
Also cleaned up some test code, and did some re-formatting.
Also fix statically assigned bucket and key names in object size fetch on puts
Just the basics for now, but more to come.
They're not 100% complete, but overall coverage of the class is at 90%, which seems like it's good enough.
- Add all the methods I think it needs - Re-order them alphabetically - Work on how to configure it
Worked with Reid and Peter to get the S3 transfer class connecting to S3, and recusively LS-ing objects in a bucket. It also inherently gets `__credentials`, and `cd` working too.
It does send, the paths are weird, and I don't know what they're supposed to do. I suspect there's some changes to make.
Also cleaned up some test code, and did some re-formatting.
Also fix statically assigned bucket and key names in object size fetch on puts
Just the basics for now, but more to come.
They're not 100% complete, but overall coverage of the class is at 90%, which seems like it's good enough.
…enia into Issue939_S3Transfer
|
The PR looks really good. I read over the commits, and they seem fine. I'm a bit puzzled about what to do about the unit tests. |
petersilva
approved these changes
Apr 5, 2024
|
OK... well I tried to get the unit tests to pass. I'm pretty sure they will AFTER this gets merged... but I can't figure out why it isn't passing already. |
|
sigh... I merged it, but the unit tests still have a failure on ubuntu 20.04 as @gcglinton had mentioned. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per many discussions, voila, a native S3-compatible transfer driver.
Built on the backs of the good work done by @reidsunderland, and @tysonkaufmann, and a collaborative programming session with @petersilva, @reidsunderland, and @andreleblanc11.
It does all the things it's supposed to do, at least as far as I can tell.
I ran a very simple set of configs, and there was no errors:
pollto find files in an S3 bucketsubscribeto download and save them, and then post a new message for..senderto pick up those local files and re-upload them to a different bucketIt's only been tried against AWS S3 proper, so it may or may not work with MinIO, or others, but theoretically, everything is in place to do that.
More testing will likely be required to ensure it's working exactly as intended, but this might be a good start.
There's also some unit tests, because they're tasty, and fun, and I like writing them (what does that say about me?). It also meant figuring how to mock
boto3in order to keep from having to spin up a dependent service, which turned out to be super easy using moto.The only issue left is that these new tests fail on Ubuntu 20.04, but they work fine on 22.04. It seems like the Ubuntu provided
python3-opensslis broken, or incompatible with boto3. Following a combination of answers from this SO question led me to fixing it by forcing pip to upgrade the pyOpenSSL package:sudo pip3 install pyopenssl --upgradeFixing the packages in the runner before doing the unit tests is the best way to go about it, but that would also mean documenting that fix somewhere so users can actually make it work on Ubuntu 20.04, and potentially other distributions/versions as well.
It might be also be possible to conditionally skip the s3 tests, based on Python version, or possibly even OS, but from what I've been able to find, that's not very reliable, and very much doesn't solve the issue.