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

Fix aws deprecation warning message #279

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Fix aws deprecation warning message #279

merged 1 commit into from
Dec 5, 2016

Conversation

phstc
Copy link
Collaborator

@phstc phstc commented Dec 5, 2016

Actualy aws is always set here https://github.com/phstc/shoryuken/blob/master/lib/shoryuken.rb#L29
so it's never nil, consequently always showing the warning message no
matter if aws is set or not

Actualy `aws` is always set here https://github.com/phstc/shoryuken/blob/master/lib/shoryuken.rb#L29
so it's never nil, consequently always showing the warning message no
matter if `aws` is set or not
Shoryuken.logger.warn { "[DEPRECATION] aws in shoryuken.yml is deprecated. Please use configure_server and configure_client in your initializer"} unless Shoryuken.options[:aws].nil?
unless Shoryuken.options[:aws].to_h.empty?
Shoryuken.logger.warn { '[DEPRECATION] aws in shoryuken.yml is deprecated. Please use configure_server and configure_client in your initializer' }
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@h3poteto I should have tested #269 more, it's always showing the warning message, because of this https://github.com/phstc/shoryuken/blob/master/lib/shoryuken.rb#L29.

But TBH, I'm strongly considering removing this DEPRECATION warning. I don't see the issue with setting it in the shoryuken.yml, otherwise I would need to write a class only to initialize it in a configure_server block, also some people have a shoryuken.yml in a server folder that they move during the deploy process to the app/config, WDYT?

Choose a reason for hiding this comment

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

@phstc @h3poteto
I think shoryuken 's aws related config should be defined in the initializer. Because if the configuration of aws is in shoryuken.yml, the initialize process of shoryuken becomes too complicated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed this https://github.com/phstc/shoryuken/blob/master/lib/shoryuken.rb#L29 .
Thank you for your fix.

I recommend use configure_server and configure_client in initializer. So, I add this DEPRECATION warning.

Now, Shoryuken has multipe configure methods of aws.
First, worker side has two ways:

  • Write aws information in shoryuken.yml
  • Use configure_client block in initializer

Second, producer side has a few ways:

  • Set environment variables, AWS_ACCESS_KEY and AWS_SECRET_ACCESS_KEY
  • Use ~/.aws/credentials
  • Use Instance profile
  • Use configure_server block in initializer

I thought this situation was complicated.
And I thought that setting aws in separate place is not good.

But, It may not be bad for multiple methods to exist, if we can control.
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@h3poteto @suzan2go I have a project which I don't use Rails, I use a standalone version of Shoryuken. So for this project, I would need to create a Ruby class, and make sure it's the first file required in the -r to set the AWS config. Also, there are still some people that during the deploy process (capistrano), copy a shoryuken.yml to the project's dir, for this people they would need to copy a class instead, which feels bad or change the way they initialize AWS.

Because if the configuration of aws is in shoryuken.yml, the initialize process of shoryuken becomes too complicated.

Unless I'm missing something I think the only thing it requires is this:
https://github.com/phstc/shoryuken/blob/master/lib/shoryuken/environment_loader.rb#L55-L58

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems that Sidekiq does not allow configuring Redis in the sidekiq.yml. Alright, maybe we don't need to then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I answer this:

I have a project which I don't use Rails, I use a standalone version of Shoryuken. So for this project, I would need to create a Ruby class, and make sure it's the first file required in the -r to set the AWS config.

I thought that if people use pure Ruby class to Shoryuken worker (not Rails), please write configure_client in top of the Ruby file.

Shoryuken.configure_client do |config|
  config.aws = {
    sqs_endpoint: ENV["SQS_ENDPOINT"],
    secret_access_key: ENV["AWS_SECRET_ACCESS_KEY"],
    access_key_id: ENV["AWS_ACCESS_KEY_ID"],
    region: ENV["AWS_REGION"]
  }
end

class MyWorker
  include Shoryuken::Worker

  shoryuken_options queue: 'default', auto_delete: true
  def perform(sqs_msg, body)
    puts body
  end
end

This way is same as Sidekiq, it presented here: https://www.youtube.com/watch?v=bfPb1zD91Rg&index=1&list=PLjeHh2LSCFrWGT5uVjUuFKAcrcj5kSai1

@phstc phstc merged commit 085b1a5 into master Dec 5, 2016
@phstc phstc deleted the fix-269 branch December 5, 2016 15:30
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.

3 participants