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

BF: Allow to explicitly specify host to aws-s3 resource #4239

Merged
merged 2 commits into from Mar 6, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Mar 3, 2020

See first commit (cc7dbd5 ATM) for details

@yarikoptic yarikoptic added this to the 0.12.3 milestone Mar 3, 2020
@codecov
Copy link

@codecov codecov bot commented Mar 3, 2020

Codecov Report

Merging #4239 into maint will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4239      +/-   ##
==========================================
+ Coverage   89.28%   89.31%   +0.03%     
==========================================
  Files         275      275              
  Lines       36853    36859       +6     
==========================================
+ Hits        32905    32922      +17     
+ Misses       3948     3937      -11     
Impacted Files Coverage Δ
datalad/downloaders/tests/test_http.py 60.58% <0.00%> (+2.18%) ⬆️
datalad/downloaders/http.py 73.70% <0.00%> (+2.78%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43c671d...8e340e0. Read the comment docs.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Mar 3, 2020

poor windowses

@yarikoptic yarikoptic added the merge-if-ok label Mar 3, 2020
datalad/downloaders/s3.py Outdated Show resolved Hide resolved

@with_tempfile
def test_boto_host_specification(tempfile):
# This test relies on a yoh-specific set of credentials to access
Copy link
Contributor

@kyleam kyleam Mar 3, 2020

Choose a reason for hiding this comment

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

While this of course is useful on your end, my preference would be to not bring this into the main tree.

Copy link
Member Author

@yarikoptic yarikoptic Mar 3, 2020

Choose a reason for hiding this comment

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

where/how to keep it so I don't forget and test whenever I do change anything in related code?

Copy link
Contributor

@kyleam kyleam Mar 3, 2020

Choose a reason for hiding this comment

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

If having an appropriately named local branch isn't enough, you could leave a one-line comment in this file as a note for yourself. Anyway, just my two cents. Feel free to disregard.

yarikoptic added 2 commits Mar 3, 2020
While trying to crawl  dandiarchive  bucket with authentication, to fetch also
files which are not publicly available, I have ran into

  <Error><Code>InvalidRequest</Code><Message>The authorization mechanism you have provided is not supported. Please use AWS4-HMAC-SHA256.<

for which discussion was ongoing in 2017: jschneier/django-storages#28 .
A workaround which worked for me was to specify host option to boto.connect_s3 to
point to the specific region.  So with this fix now it would be possible to use it
in the provider configuration, e.g.

	[provider:dandi-s3]
	url_re = s3://dandiarchive($|/.*)
	credential = dandi-s3-backup
	authentication_type = aws-s3
	aws-s3_host = s3.us-east-2.amazonaws.com

There might be other options we might want to add later on, so I did not
store host in the attribute, but right within the dictionary of optional
kwargs for connect_s3.
@kyleam kyleam merged commit 3e70bd0 into datalad:maint Mar 6, 2020
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants