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

[MRG+1] A different S3 Endpoint URL is now possible when uploading images #3625

Merged
merged 2 commits into from Feb 20, 2019

Conversation

@Ordepsousa
Copy link
Contributor

@Ordepsousa Ordepsousa commented Feb 13, 2019

AWS_ENDPOINT_URL in Settings was read by files.py but not by images.py. I don't know if it was intentionally but i couldn't connect to my local minio S3 Server when uploading images with ImagesPipeline

@codecov
Copy link

@codecov codecov bot commented Feb 13, 2019

Codecov Report

Merging #3625 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3625      +/-   ##
==========================================
+ Coverage    84.5%   84.52%   +0.01%     
==========================================
  Files         167      167              
  Lines        9406     9410       +4     
  Branches     1397     1397              
==========================================
+ Hits         7949     7954       +5     
  Misses       1199     1199              
+ Partials      258      257       -1
Impacted Files Coverage Δ
scrapy/pipelines/images.py 90.71% <100%> (+0.27%) ⬆️
scrapy/utils/trackref.py 86.48% <0%> (+2.7%) ⬆️

@kmike
Copy link
Member

@kmike kmike commented Feb 13, 2019

A great catch, thanks @Ordepsousa!

I wonder if you can add other missing AWS options here as well, as you're on it - it looks like AWS_REGION_NAME, AWS_USE_SSL and AWS_VERIFY are also missing.

By the way, the existing code doesn't look good at all: it mutates a global object, so one can't start two spiders via CrawlerRunner and have different AWS options for them. Also, the code is duplicated between files and images pipelines. But that're separate issues.

@kmike kmike changed the title A different S3 Endpoint URL is now possible when uploading images [MRG+1] A different S3 Endpoint URL is now possible when uploading images Feb 13, 2019
@Ordepsousa
Copy link
Contributor Author

@Ordepsousa Ordepsousa commented Feb 13, 2019

No problem @kmike :)

Added the missing AWS settings as you asked for
Yeah, it doesn't look so good, probably will need some cleaning in the future

@kmike kmike added this to the v1.7 milestone Feb 15, 2019
@kmike kmike removed this from the v1.7 milestone Feb 15, 2019
@dangra dangra merged commit c72ab1d into scrapy:master Feb 20, 2019
3 checks passed
@kmike kmike added this to the v1.7 milestone Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants