-
-
Notifications
You must be signed in to change notification settings - Fork 280
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
Conversation
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
andAWS_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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Actualy
aws
is always set here https://github.com/phstc/shoryuken/blob/master/lib/shoryuken.rb#L29so it's never nil, consequently always showing the warning message no
matter if
aws
is set or not