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

allow aws sdk v3 #381

Merged
merged 1 commit into from
Jun 24, 2017
Merged

allow aws sdk v3 #381

merged 1 commit into from
Jun 24, 2017

Conversation

rsharrott
Copy link
Contributor

Probably should go and define the specific gems in use once sdk v3 is released.

@phstc
Copy link
Collaborator

phstc commented Jun 12, 2017

Hi @rsharrott

Is there an ETA for v3?

@rsharrott
Copy link
Contributor Author

SDK V3 release candidates are already available. The basic idea is to split the SDK to multiple gems so we can select only the aws services we actually use and reduce the number of dependencies. More details here: https://aws.amazon.com/blogs/developer/aws-sdk-for-ruby-modularization-version-3-2/

This PR simply allows people who are using SDK V3 to use recent versions of shoryuken. The current gemfile restricts to SDK V2 so I changed it to allow V2 or V3.

@phstc
Copy link
Collaborator

phstc commented Jun 12, 2017

@rsharrott cool. I will test it out. Have you tested it with Shoryuken? I'm wondering if there's any breaking change.

Migrating Code From Version 2 to Version 3

Based on what they say, it should be just a bump.

@rsharrott
Copy link
Contributor Author

I'm running it in production now and have not seen any issues.

@oyeanuj
Copy link

oyeanuj commented Jun 24, 2017

@phstc Do you think this can be merged?

I was trying to use aws-sdk-s3 and it is conflicting with Shoryuken (which wants v2 and aws-sdk-s3 wants v3).

Thanks @rsharrott for the PR!

@phstc phstc merged commit 43ceef2 into ruby-shoryuken:master Jun 24, 2017
@phstc
Copy link
Collaborator

phstc commented Jun 24, 2017

@rsharrott @oyeanuj merged. We will need to pay attention in case aws-sdk gets a new version > 2 that breaks the compatibility with 2.

@phstc
Copy link
Collaborator

phstc commented Sep 1, 2017

@rsharrott @oyeanuj I had to revert the aws-sdk change, since v3 does not work with Shoryuken, as @ota42y pointed out here.

With v3:

uninitialized constant Aws::SQS

For making it compatible with v3, we would need something like this:

spec.add_dependency 'aws-sdk-core', '>= 2'

if Aws::VERSION >= 3 # not sure how to do this if
  spec.add_dependency 'aws-sdk-sqs', '~> 1'
end

# shoryuken.rb
require 'aws-sdk-core'

if Aws::VERSION >= 3 # not sure how to do this if
  require 'aws-sdk-sqs'
end

Any thoughts?

cc/ @pyromaniac

@pyromaniac
Copy link

pyromaniac commented Sep 1, 2017

Yeah, that's exactly what we need, but unfortunately, I don't see a way to do it. Instead, we can try to do it like this:

if Aws::VERSION >= 3 # not sure how to do this if
  begin
    require 'aws-sdk-sqs'
  rescue LoadError
    puts "Aws 3 requires `aws-sdk-sqs` to be installed separately. Please add `gem 'aws-sdk-sqs'` 
to your Gemfile".
  end
end

And probably, they didn't update the version lol https://github.com/aws/aws-sdk-ruby/blob/master/gems/aws-sdk-core/lib/aws-sdk-core/version.rb

@oyeanuj
Copy link

oyeanuj commented Sep 1, 2017

@phstc Just so I am understanding correctly, shoryuken needs to load aws-sdk v2 or just aws-sdk-sqs for v3? So shoryuken could just make a major version bump? I am sure I am missing something in my understanding here...

@phstc
Copy link
Collaborator

phstc commented Sep 1, 2017

@oyeanuj Shoryuken runs fine with aws-sdk-core 2, but for 3 it needs aws-sdk-sqs, requiring it in the gemspec and also via require. Ideally, I would like to make it compatible with both, without having a major release.

@pyromaniac I think it's a great idea, I will play with it.

@phstc
Copy link
Collaborator

phstc commented Sep 2, 2017

@pyromaniac I think I got a fix based on your snippet (great idea 🍻) WDYT #433?

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.

4 participants