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

Add aws configuration in 'Shoryuken.configure_server' #252

Merged
merged 3 commits into from
Oct 23, 2016

Conversation

h3poteto
Copy link
Contributor

I add aws configuration in Shoryuken.configure_server, like sidekiq:
https://github.com/mperham/sidekiq/blob/master/lib/sidekiq.rb#L75-L83

Now, I have to call EnvironmentLoader.load in initializer of my Rails application to setup aws configuration. For example:

EnvironmentLoader.load(config_file: "config/shoryuken.yml")

I want to divide EnvironmentLoader.load method, because I try to fix #249.

To fix #249, shoryuken have to read shoryuken.yml before daemonize, and then load Rails.
So, I want to divide EnvironmentLoader.load.

But, now EnvironmentLoader.load is used in initializer of user's Rails application, therefore I propose alternate method to configure aws in initializer. That is Shoryuken.configure_server.

In sidekiq, user can configure redis information in initializer.

Sidekiq.configure_server do |config|
  config.redis = { url: "redis://localhost:6379" }
end

Sidekiq.configure_client do |config|
 config.redis = { url: "redis://localhost:6379" }
end

I aim the same way as sidekiq.

Shoryuken.configure_server do |config|
  config.aws = { :sqs_endpoint => '...', :access_key_id: '...', :secret_access_key: '...', region: '...' }
end

Shoryuken.configure_client do |config|
  config.aws = { :sqs_endpoint => '...', :access_key_id: '...', :secret_access_key: '...', region: '...' }
end

receive_message
).map(&:to_sym)

aws_options = hash.reject do |k, v|

Choose a reason for hiding this comment

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

Unused block argument - v. If it's necessary, use _ or _v as an argument name to indicate that it won't be used.

@options ||= {}
end

def options=(hash)

Choose a reason for hiding this comment

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

Use attr_writer to define trivial writer methods.

@@ -0,0 +1,49 @@
module Shoryuken
class AwsConfig

Choose a reason for hiding this comment

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

Extra empty line detected at class body beginning.

@phstc
Copy link
Collaborator

phstc commented Oct 9, 2016

Hi @h3poteto

Not sure if I could follow the change completely. Is it basically extracting the Aws.config setup? How will it fix #249?

@h3poteto
Copy link
Contributor Author

@phstc
Thank you for your reply.

I create sample pull request in my repository: h3poteto#2
I will divide EnvironmentLoader.load to fix #249, and intercept current EnvironmentLoader.load.

But, now developers use EnvironmentLoader.load, so I prepare alternate load method which is this pull request.

@@ -118,6 +131,10 @@ def on_stop(&block)
@stop_callback = block
end

def aws=(hash)
@aws = Shoryuken::AwsConfig.setup(hash)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

@h3poteto is @aws being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, @aws is not used!

@agustin
Copy link
Contributor

agustin commented Oct 23, 2016

Hi there! what do you think of this @phstc? looks like something that you would like to merge eventually? just wondering because I was looking for a good way to decouple the general AWS config of my application from the one that Shoryuken should use.

I have 3 different users with different permissions in my app, one is for general purposes like store stuff in S3. Other is handling keys with the KMS, and was expecting to be able to have the last one to be in charge of the queues and jobs/workers, but the way is right now seems that if I passed the [:aws] key in the shoryuken.yml config file this will override the Aws.config right? or I'm maybe missed something here 😟 .

And also on the Shoryuken::Client Class there is no way to pass over a different configuration to the SQS or SNS clients. I feel that, as I said before, there is something that I'm missing here.

Let me know if this is not clear at all, and also if you rather have this conversation on an issue or in another place, I wrote it in here because this PR looks like something that will help on this same topic. Maybe I'm wrong, don't hesitate to let me know!

Thanks a lot for all the hard work on all this! 🍺

@phstc
Copy link
Collaborator

phstc commented Oct 23, 2016

hi @agustin

The change in this PR is still using Aws.config which configures the Aws globally. If I understood correct, you would like to configure the SQS client in Shoryuken using a specific creds and region and for other parts of your app to use a different creds, right?

For example, using this:

sqs = Aws::SQS::Client.new(
  region: region_name,
  credentials: credentials,
  # ...
)

Instead of globally setting it Aws.config, is that right?

@agustin
Copy link
Contributor

agustin commented Oct 23, 2016

exactly @phstc! maybe if the SNS and the SQS method on the Shoryuken::Client accepts to pass the client directly as an optional argument and if not fallback to the actual way of instance it, maybe that could work. WDYT?

If you are ok with that maybe I can open a PR with that feature, let me know!
Thankss

@phstc
Copy link
Collaborator

phstc commented Oct 23, 2016

@agustin actually, I think we can continue what @h3poteto started in this PR and add AwsConfig.sqsand AwsConfig.sns instead of configuring Aws.config directly. WDYT?

If you are ok with that maybe I can open a PR with that feature, let me know!

Please do!

@phstc phstc merged commit ab2c3b0 into ruby-shoryuken:master Oct 23, 2016
@phstc
Copy link
Collaborator

phstc commented Oct 23, 2016

@h3poteto great work on this, thanks! Added to master 🍻

@agustin ^

@h3poteto
Copy link
Contributor Author

thank you!

agustin added a commit to agustin/shoryuken that referenced this pull request Oct 24, 2016
…I added to the AwsConfig Class the sqs and sns methods to properly instanciate a client with the credentials and avoid the override of Aws.config ones
agustin added a commit to agustin/shoryuken that referenced this pull request Nov 8, 2016
…I added to the AwsConfig Class the sqs and sns methods to properly instanciate a client with the credentials and avoid the override of Aws.config ones

- Remove a redundant private method, following @phstc comment
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.

Can't read logfile option in config file
4 participants