Added support for aws-sdk-s3 gem which is now preferred way to intera…

[morgoth]

…ct with s3.

Reference: https://github.com/aws/aws-sdk-ruby/blob/master/V3_UPGRADING_GUIDE.md#library-maintainer

It still should work with aws-sdk fine

[houndci-bot]

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

[houndci-bot]

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

[houndci-bot]

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

[virgoproz]

+1 on this Pull Request to be merged!

[olivierbuffon]

+1

[morgoth]

@tute Can we merge it and have a new version release, please?

[tute]

We could also open up the spec version without changing the gem name, as it says in the linked doc: spec.add_dependency('aws-sdk', '>= 2.0', '< 4').

I don't work on paperclip now. Maybe @jyurek can point us in the right direction.

[morgoth]

Version 3.x depends almost on the whole internet ;-) as you can see in https://rubygems.org/gems/aws-sdk-resources
The proper solution, that allows moving forward is to use standalone gem.
There is still fallback to aws-sdk so it won't break existing apps.

[bitsapien]

+1

[virgoproz]

any updates?

[leogiertz]

@jyurek Would be fantastic if you could take a look at this. 🎩

[geoffharcourt]

Hi, I had some test failures while I was reviewing, so I'm looking into what might be wrong:

Testing against version 5.1.4

An error occurred while loading ./spec/paperclip/storage/s3_spec.rb.
Failure/Error: (Gem::Version.new(Aws::VERSION) >= Gem::Version.new("2.3.0"))

NameError:
  uninitialized constant Aws::VERSION
# ./spec/support/conditional_filter_helper.rb:3:in `aws_accelerate_available?'
# ./spec/paperclip/storage/s3_spec.rb:285:in `block in <top (required)>'
# ./spec/paperclip/storage/s3_spec.rb:4:in `<top (required)>'

Randomized with seed 785

[geoffharcourt]

This looks like it's a little more complicated than it appears at first glance. With the new aws-sdk-s3 gem we can drop a bunch of conditional checking for accelerated S3 transport capability (it's now always available with the new gem), but the old gem dropping the Aws::VERSION constant means that we can't reliably check that version number to confirm compatibility or the presence of that capability.

I think we should drop the fallback to the aws-sdk gem and remove the conditional.

[geoffharcourt]

Related: #2484

[geoffharcourt]

Hi @morgoth we're going to nip this in the bud and drop support for the aws-sdk gem and cut a new major release, so please do remove the fallback code.

[morgoth]

@geoffharcourt I removed it. Not sure why tests are now failing (they pass locally).
Also not sure why they were passing before...

[geoffharcourt]

@morgoth I think before the reason was that the aws-sdk gem had a breaking change from the missing constant that was done in a tiny version update (I am guessing they didn't think anyone would depend on that constant). Not sure why the tests are failing, I had a bunch of trouble getting the suite to pass today.

I'll take a look at the Travis failures.

[morgoth]

@geoffharcourt CI failures are not related to this changes and are fixed in https://github.com/thoughtbot/paperclip/pull/2488/files
Can we move it forward, please?

[pjg]

Could we move this forward, please?

[pjg]

@morgoth after bundling the version from this PR, I'm presented with the following message:

##################################################
#  NOTE FOR UPGRADING FROM 4.3.0 OR EARLIER       #
##################################################

Paperclip is now compatible with aws-sdk >= 2.0.0.

If you are using S3 storage, aws-sdk >= 2.0.0 requires you to make a few small
changes:

* You must set the `s3_region`
* If you are explicitly setting permissions anywhere, such as in an initializer,
  note that the format of the permissions changed from using an underscore to
  using a hyphen. For example, `:public_read` needs to be changed to
  `public-read`.

For a walkthrough of upgrading from 4 to 5 and aws-sdk >= 2.0 you can watch
http://rubythursday.com/episodes/ruby-snack-27-upgrade-paperclip-and-aws-sdk-in-prep-for-rails-5

I think this needs to be updated too, to mention aws-sdk-s3 v3.

[morgoth]

@pjg yes, but I'm not sure if I should do this here.
@geoffharcourt said that he wants to release a major version, so he will have a better overview what will be included there and might adjust it before release himself.

[morgoth]

I rebased with master, but looks like there is new build error

/home/travis/.rvm/rubies/ruby-2.3.4/bin/ruby -S bundle exec cucumber --format progress
F----------------No such file or directory - getcwd (Errno::ENOENT)

😞

[geoffharcourt]

@morgoth I'm not sure what happened, your prior PR (#2488) passed on Travis and then the same commit failed after it was merged.

[morgoth]

@geoffharcourt Maybe some gem updated in the meantime, thus having a different environment to run the tests as Gemfile.lock is git ignored.

[morgoth]

Rebased with master, so it's green now

[morgoth]

Any news on this one? :-)

[Empact]

@morgoth Would you be up for adjusting the PR to make it backwards compatible with aws-sdk < 3 as well as aws-sdk-s3? That would make maintenance and upgrading more forgiving by easing discontinuities.

[morgoth]

@Empact It was like this at the beginning but then I was asked to remove backward compatibility in #2481 (comment)

[Empact]

@morgoth Fair enough if the maintainers want it that way. I generally like to leave backwards compatibility for at least one release so that users can upgrade the dependency in isolation - i.e. they can upgrade Paperclip without needing to also upgrade aws-sdk; but since the dependency is a different library altogether that doesn't really apply here.

[kawikathomas]

Any idea when this will get merged?

[collimarco]

Yes, it would be really useful if you could merge this now

[nateemerson]

@morgoth THANK YOU for your work on this. Looks fantastic!
@geoffharcourt THANK YOU for your work on reviewing and getting this moved through. I greatly appreciate your efforts.

That being said...

Can we please move forward on this? I have a project that is currently stuck in a vulnerable state due to https://nvd.nist.gov/vuln/detail/CVE-2017-0889. I can't upgrade Paperclip due to a newly discovered type of dependency hell enabled by Amazon's bizarre architectural changes from aws-sdk 1.x to 3.x :|

[bmulholland]

@nateemerson Similar issue here. We upgraded to aws-sdk 2.x which enabled a Paperclip upgrade. Does that work for you in the interim?

[nateemerson]

@bmulholland No because my project has a feature dependent on a service gem from aws-sdk v3...v1 and v3 could surprisingly coexist due to the architecture overhaul (paperclip depending on v1 of aws-sdk and this other component depending on a specific service lib from v3) which is why the project is in the current state. Unfortunately, v2 and v3 cannot coexist because the architecture is more similar so there are dependency clashes between the current Paperclip version and this other service gem.

[nateemerson]

If anyone else is experiencing the issue I am having, I forked the repo and cut a tag here that you can feel free to use in the short term. @morgoth's code merged cleanly with the v5.2.1 branch. Thanks again for your work @morgoth!

[quentindemetz]

please merge this

[michal-samluk]

Is there any chance that this will be merged soon? :)

[zhitongLIU]

Please merge this, we don't need aws-iot in our projet....

[woodhull]

@geoffharcourt 👋 old friend, any chance you could either accept or reject with a reason so I can fix it?

[geoffharcourt]

Hi @woodhull! I'm no longer at thoughtbot, so I'm not in a position to accept or reject.

[bmulholland]

Looks like it's @sidraval that's maintaining this repo now. Sid, could this be merged?

[sidraval]

Thanks so much for the PR @morgoth 😄

[xtagon]

@sidraval Thanks for merging! 👍