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.

Sign up for GitHub

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

Jump to bottom

Fix aws deprecation warning message #279

Merged
merged 1 commit into from
Dec 5, 2016
Merged

Fix aws deprecation warning message #279

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/shoryuken/environment_loader.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ def config_file_options
# https://github.com/phstc/shoryuken/blob/a81637d577b36c5cf245882733ea91a335b6602f/lib/shoryuken.rb#L82
# Please delete this method afert next release (v2.0.12 or later)
def initialize_aws
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?

Copy link

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

Shoryuken::AwsConfig.setup(Shoryuken.options[:aws])
end

Expand Down