Skip to content
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

Merged
merged 11 commits into from
Apr 3, 2021
Merged

Conversation

pjbull
Copy link
Member

@pjbull pjbull commented Mar 23, 2021

#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.

* 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 :)
@pjbull pjbull changed the title Add possibility to parameterize S3 service URL (#137) Add possibility to parameterize S3 service URL Mar 23, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Mar 23, 2021

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #138 (b1adaca) into master (80b596f) will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##           master    #138   +/-   ##
======================================
  Coverage    93.5%   93.5%           
======================================
  Files          21      21           
  Lines        1119    1119           
======================================
  Hits         1047    1047           
  Misses         72      72           
Impacted Files Coverage Δ
cloudpathlib/s3/s3client.py 92.8% <100.0%> (ø)
cloudpathlib/anypath.py 100.0% <0.0%> (ø)

@jayqi jayqi linked an issue Mar 26, 2021 that may be closed by this pull request
Copy link
Member

@jayqi jayqi left a 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?

docs/docs/authentication.md Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@pjbull
Copy link
Member Author

pjbull commented Mar 26, 2021

@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.

@pjbull
Copy link
Member Author

pjbull commented Mar 26, 2021

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

@jayqi jayqi self-requested a review April 2, 2021 20:08
@jayqi
Copy link
Member

jayqi commented Apr 2, 2021

@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
Copy link
Member Author

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!

@pjbull pjbull merged commit de6b547 into master Apr 3, 2021
@pjbull pjbull deleted the custom-s3-endpoint branch April 3, 2021 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use cloudpathlib for accessing custom S3 installations?
3 participants