-
Notifications
You must be signed in to change notification settings - Fork 53
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 possibility to parameterize S3 service URL #138
Conversation
* Add possibility to parameterize S3 service URL * add usage description into the docs * add rig for custom S3 servers * address PR review: add cods to rig, allow mock * forgot to commit tests/conftest.py :)
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
======================================
Coverage 93.5% 93.5%
======================================
Files 21 21
Lines 1119 1119
======================================
Hits 1047 1047
Misses 72 72
|
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.
Tests pass and I think this looks good overall, but I think there are some changes we can make. @pjbull do you want to fix this or would you like me to?
@jayqi FYI, the tests don't actually pass. (The rig wasn't added to the list and so they weren't run). There are some failing tests that may actually be MinIO/S3 incompatibility, but I haven't gotten to the bottom of them yet. |
Also, changes you suggest make sense, feel free to make those and look at the failing tests. I'll check with you next time I have a chance to dig in to see if you uncovered anything |
@pjbull I think this looks ready now if you have time to take a look. |
if not getattr(rig, "is_custom_s3", False): | ||
# Skip resetting the default client for custom S3 endpoint, but keep the other tests, | ||
# since they're still useful. | ||
rig.client_class._default_client = None |
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.
👏 I hadn't figured out why this one was failing!
#137 added ability to specify the endpoint url for
S3Path
That work is accepted, and we need to now test if the configuration of the live tests for that scenario work from a non-fork branch.