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 ability to set endpoint_url from AWS_ENDPOINT_URL env variable #193

Merged
merged 4 commits into from
Jan 26, 2022

Conversation

kabirkhan
Copy link
Contributor

Hey guys! Long time :)

Loving this package so far. One minor convenience that might be easier to merge here than this ridiculous upstream PR for the boto3 library.

Allows setting the endpoint_url from the AWS_ENDPOINT_URL env variable while waiting for the upstream PR from boto3: boto/boto3#2746

I'll add a test if the idea works for you guys

…ery useful for localstack while waiting for upstream PR from boto3: boto/boto3#2746
@pjbull
Copy link
Member

pjbull commented Jan 25, 2022

Looks good to me! Test would be great, and then an issue to track removing if/when the upstream issue is resolved.

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #193 (ee99b4a) into master (3827cd8) will decrease coverage by 0.4%.
The diff coverage is 100.0%.

@@           Coverage Diff            @@
##           master    #193     +/-   ##
========================================
- Coverage    94.3%   93.9%   -0.5%     
========================================
  Files          21      21             
  Lines        1197    1198      +1     
========================================
- Hits         1129    1125      -4     
- Misses         68      73      +5     
Impacted Files Coverage Δ
cloudpathlib/s3/s3client.py 92.2% <100.0%> (-2.9%) ⬇️
cloudpathlib/gs/gsclient.py 92.3% <0.0%> (-2.0%) ⬇️

@kabirkhan
Copy link
Contributor Author

Here's that test

@kabirkhan
Copy link
Contributor Author

Added the issue as well

@pjbull
Copy link
Member

pjbull commented Jan 25, 2022

Test looks good. Can you make sure linting passes? (make lint should do that for you locally)

@kabirkhan
Copy link
Contributor Author

Done, not sure why the live tests would be failing though. Doesn't seem to be related to this change.

@pjbull
Copy link
Member

pjbull commented Jan 26, 2022

@kabirkhan yeah, i think that is just that PRs from forks can't use the secrets so live tests can't authenticate. i'll do some digging to see how we enable that.

@pjbull pjbull changed the base branch from master to aws-endpoint-url January 26, 2022 01:13
@pjbull pjbull merged commit 2a4e221 into drivendataorg:aws-endpoint-url Jan 26, 2022
@pjbull
Copy link
Member

pjbull commented Jan 26, 2022

Dropped these changes on their own branch so we can run the live tests there to confirm

pjbull added a commit that referenced this pull request Jan 26, 2022
) (#195)

* Add ability to set endpoint_url from AWS_ENDPOINT_URL env variable. Very useful for localstack while waiting for upstream PR from boto3: boto/boto3#2746

* add test

* rm print

* lint

Co-authored-by: Kabir Khan <[email protected]>
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.

2 participants