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

use variable for client schemes, allowing override #467

Merged

Conversation

kujenga
Copy link
Contributor

@kujenga kujenga commented Aug 31, 2024

This change is intended to make the default client implementations more flexible so that their scheme can be customized. This can be useful in scenarios where a subclass wants to implement a custom scheme on e.g. a S3 compatible API [1] but with a custom scheme so that the default S3 access is still also available.

[1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores

The tests have been updated to include a new s3-like rig which uses the
new scheme override functionality.

(If there is a better strategy for tests or this seems like overkill let me know and happy to adjust! It seems a bit more work is needed to get them passing)

Closes #466


Contributor checklist:

  • I have read and understood CONTRIBUTING.md
  • Confirmed an issue exists for the PR, and the text Closes #issue appears in the PR summary (e.g., Closes #123).
  • Confirmed PR is rebased onto the latest base
  • Confirmed failure before change and success after change
  • Any generic new functionality is replicated across cloud providers if necessary
  • Tested manually against live server backend for at least one provider
  • Added tests for any new functionality
  • Linting passes locally
  • Tests pass locally
  • Updated HISTORY.md with the issue that is addressed and the PR you are submitting. If the top section is not `## UNRELEASED``, then you need to add a new section to the top of the document for your change.

This change is intended to make the default client implementations
more flexible so that their scheme can be customized. This can be
useful in scenarios where a subclass wants to implement a custom
scheme on e.g. a S3 compatible API [1] but with a custom scheme
so that the default S3 access is still also available.

[1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores

The tests have been updated to include a new s3-like rig which uses the
new scheme override functionality.
@kujenga kujenga force-pushed the at/client-prefix-flexibility branch from b458e3c to 54c37b2 Compare August 31, 2024 06:04
Copy link

codecov bot commented Aug 31, 2024

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.2%. Comparing base (b776bee) to head (147db5b).
Report is 1 commits behind head on 467-live-tests.

Files with missing lines Patch % Lines
cloudpathlib/azure/azblobclient.py 75.0% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           467-live-tests    #467     +/-   ##
================================================
- Coverage            94.2%   93.2%   -1.0%     
================================================
  Files                  23      23             
  Lines                1745    1745             
================================================
- Hits                 1645    1628     -17     
- Misses                100     117     +17     
Files with missing lines Coverage Δ
cloudpathlib/gs/gsclient.py 90.0% <100.0%> (-1.5%) ⬇️
cloudpathlib/s3/s3client.py 92.9% <ø> (-1.5%) ⬇️
cloudpathlib/azure/azblobclient.py 89.9% <75.0%> (-6.9%) ⬇️

Copy link
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the hard-coding is a bug. Thanks for the fix. Agreed we want to enable scenarios like you lay out.

Do we need to add cloud_prefix to the client though? I'd rather keep it in one place just on the path.

For all the instances you replaced, you should be able to get the cloud_prefix from the instance of the cloud_path passed to the method, no?

Would that work for your use cases? If not, we could potentially add cloud_prefix as a property that reaches through _cloud_meta to get the associated cloud_prefix from the path_class:
https://github.com/drivendataorg/cloudpathlib/blob/master/cloudpathlib/cloudpath.py#L96-L100

@pjbull
Copy link
Member

pjbull commented Sep 1, 2024

Also, one other thought: I think adding a rig for this is overkill. That will duplicate all the tests, which isn't necessary. I'd just do a single test with the custom scheme to show that piece works.

@kujenga
Copy link
Contributor Author

kujenga commented Sep 3, 2024

For all the instances you replaced, you should be able to get the cloud_prefix from the instance of the cloud_path passed to the method, no?

Good call, done!

Also, one other thought: I think adding a rig for this is overkill. That will duplicate all the tests, which isn't necessary. I'd just do a single test with the custom scheme to show that piece works.

Fair enough, done!

If there's anything else just let me know! I left them as separate commits to keep what I changed clear but happy to squash as well if you prefer.

Copy link
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last tweak to the tests and could you add a HISTORY.md comment? Then I think it's good to go. Thanks!

tests/test_client.py Outdated Show resolved Hide resolved
kujenga added a commit to kujenga/cloudpathlib that referenced this pull request Sep 4, 2024
This isolates the implementation in response to PR feedback:
drivendataorg#467 (comment)
This isolates the implementation in response to PR feedback:
drivendataorg#467 (comment)
@kujenga
Copy link
Contributor Author

kujenga commented Sep 4, 2024

great, let me know if there's anything else!

@pjbull pjbull changed the base branch from master to 467-live-tests September 9, 2024 14:46
@pjbull pjbull merged commit 16038bb into drivendataorg:467-live-tests Sep 9, 2024
23 of 24 checks passed
pjbull added a commit that referenced this pull request Sep 9, 2024
* use variable for client schemes, allowing override

This change is intended to make the default client implementations
more flexible so that their scheme can be customized. This can be
useful in scenarios where a subclass wants to implement a custom
scheme on e.g. a S3 compatible API [1] but with a custom scheme
so that the default S3 access is still also available.

[1] https://cloudpathlib.drivendata.org/stable/authentication/#accessing-custom-s3-compatible-object-stores

The tests have been updated to include a new s3-like rig which uses the
new scheme override functionality.

* use single cloud_prefix

* tests: switch to lighter-weight custom scheme tests

* update HISTORY file for custom scheme support

* update custom scheme tests to utilize pytest fixture

This isolates the implementation in response to PR feedback:
#467 (comment)

Co-authored-by: Aaron Taylor <aaron@additive.ai>
@kujenga
Copy link
Contributor Author

kujenga commented Oct 16, 2024

Hi @pjbull , any chance you'd be able to tag a release with this new functionality? Thank you!

@pjbull
Copy link
Member

pjbull commented Oct 16, 2024

@kujenga Yep! Need to get a release out for 3.13 anyway in the next few days.

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.

Allow subclasses of S3Client, etc to set a custom scheme
2 participants