Initialize Rails before parsing Config File

[csebryam]

Part 1

Description

When you specify to load rails in the shoryuken.yml file ( i.e rails: true), Shoryuken::EnvironmentLoader#setup_options will read the config_file before rails has had a chance to be initialized. Any declared rails environment variables will not be present at this time.

In the example below:

# config/shoryuken.yml

------------
delay: 25
queues:
  - <%= ENV['AWS_SQS_QUEUE'] %>

AWS_SQS_QUEUE will not be present in the rails environment variables as it is currently. As a result a log output will show this message: WARN: No queues supplied

Fixes:

Initialize rails as the first step in the #setup_options

Part 2

Description

Currently if you set the :rails flag in the CLI with the -R option, the Shoryuken::Runner will override options in the config files with the CLI options

# lib/shoryuken/runner.rb

loader = EnvironmentLoader.setup_options(options)

# When cli args exist, override options in config file
Shoryuken.options.merge!(options)

loader.load

When loader.load gets called, we run into the same issue as Part 1. The config_file has already been read-in and any rails environment variables were not present at that time. Again, this is because loading rails happens too late in the process, in the #load method.

Fixes

Shoryuken::EnvironmentLoader will initialize rails looking for flags from either Shoryuken.options or CLI options hash. See load_rails? method.

Other

Fixes issues similar to these #670

Because even though I use the dotenv gem, the environment variables are not loaded properly by Shoryuken.
It does work when they exist on the OS level

Similar Issues #511

[cjlarose]

Hey @csebryam, thanks for opening this PR and sorry I've been slow to give feedback.

In general, I'm open to this kind of change, but I'm hesitant because I think it might be backwards-incompatible. In some of the applications I maintain, we reference, for example, the Shoryuken logger instance in a Rails initializer like so

Rails.logger = Shoryuken::Logging.logger

I think this change might actually not break my applications in this case, but it's difficult to say that this change won't break anyone's current configuration.

What would help me as a maintainer would be to know a little bit more about your use case. What are you trying to accomplish with this change? Do you expect to have to export environment variables, for example, in a Rails initializer, in order to read them from shoryuken.yml?

If so, one workaround is to use Shoryuken's configuration API instead of the YAML file. An example is in https://github.com/ruby-shoryuken/shoryuken/wiki/Rails-Integration-Active-Job

# config/initializers/shoryuken.rb

Shoryuken.configure_server do |config|
  # Replace Rails logger so messages are logged wherever Shoryuken is logging
  # Note: this entire block is only run by the processor, so we don't overwrite
  #       the logger when the app is running as usual.

  Rails.logger = Shoryuken::Logging.logger
  Rails.logger.level = Rails.application.config.log_level

  # config.server_middleware do |chain|
  #  chain.add Shoryuken::MyMiddleware
  # end

  # For dynamically adding queues prefixed by Rails.env 
  # Shoryuken.add_group('default', 25)
  # %w(queue1 queue2).each do |name|
  #   Shoryuken.add_queue("#{Rails.env}_#{name}", 1, 'default')
  # end
end

[csebryam]

@cjlarose Thanks for your response and for being open to adding these changes. Also, thanks for sharing your shoryuken.rb initializer file as it was helpful in understanding how you're initializing the Rails logger.

To avoid any logger issues, we could move the initialize_logger as the first setup step.

    def setup_options
      initialize_logger
      initialize_rails if load_rails?
      initialize_options
    end

Use case:

1. To help new Shoryuken gem users avoid Rails specific environment loading issues. I've seen many Issues and StackOverflow questions regarding this (some of which I looked at to get additional help). It would definitely streamline the integration.
2. Without this change, it makes it difficult to use Dotenv or Figaro to setup environment variables as stated in the Wiki:

You can use dotenv to setup your environment variables.

3. This is basic functionality that should just work without having to find workarounds.

In the EnvironmentLoader file, there is an inline comment that states it was adapted from Sidekiq cli . Just going off of that, Sidekiq loads the environment as the first step in the setup_options method, and then loads the config_file as seen here: https://github.com/mperham/sidekiq/blob/main/lib/sidekiq/cli.rb#L225-L241. We would essentially be doing the same thing in this PR.

Hope to hear your thoughts.

[cjlarose]

Ok this definitely feels like the right change to make, though it does seem like it'd warrant major version bump. I don't think it's necessary to move the initialize_logger step up. We can start with initialize_rails.

I just want to clarify what the expected behavior is in a bunch of cases so that we can clearly communicate this change in the changelog.

If a user, for example, was using Shoryuken.configure_server in a Rails initializer to, for example, set delay and timeout values and has a config/shoryuken.yml file and set some options on the CLI, what order is everything interpreted? Does that order change with this PR?

[csebryam]

@cjlarose Sorry for my delay in responding, this took some digging.

Bumping to a major version makes sense to me that way it doesn't break anyones expected behavior.

what order is everything interpreted? Does that order change with this PR?

After some investigation, this PR does change the order things get interpreted.

The order things get loaded in this PR:

1. Because rails is initialized as the first step, Shoryuken.configure_server options will be loaded first.

   • It's the first time the method add_group gets called, setting any default options mentioned in code snipped (A) below.

2. Other options get merged in when Shoryuken::Runner runs:

loader = EnvironmentLoader.setup_options(options)
loader.load

Code snippet (A)
options found in Shoryuken.configure_server will take precedence because of this code here:

    def add_group(group, concurrency = nil, delay: nil)
      concurrency ||= options[:concurrency]
      delay ||= options[:delay]
      ...
    end

Again, keeping any defaults configured in the initializer.

Previous Version Below

Whats more, loading rails before or after doesn't change much in terms of how defaults get interpreted.

    def load
      load_rails
      prefix_active_job_queue_names
      parse_queues
      ...
    end

This is because in both cases rails gets loaded/initialized before parse_queues. This results in loading options from the rails initialized file options found in Shoryuken.configure_server first.

In your example above:

Shoryuken.configure_server do |config|
  Shoryuken.add_group('default', 25)
    %w(queue1 queue2).each do |name|
      Shoryuken.add_queue("#{Rails.env}_#{name}", 1, 'default')
  end
end

This code is essentially what parse_queues runs (i.e Shoryuken.add_group) and it happens first in both cases, keeping the code interpretation order basically the same.

[csebryam]

@cjlarose Not sure if I was able to answer your questions fully but I'm happy to look into anything else. Do you think this PR is ready to merge?

[cjlarose]

Hey @csebryam! I appreciate you figuring out the details on how everything gets initialized. Sorry I haven't been responsive--I took some vacation time. This is still on my radar, though. I'll take a deeper look at it and get you some feedback!

[cjlarose]

Hey @csebryam ! Sorry it took so long to get you some feedback. I was able to take a deeper look and left some comments, but this change looks overall pretty great! Thanks again for taking this on.

[cjlarose]

Since EnvironmentLoader#intialize_options will read in the config file first, then the CLI args, it seems like we can safely remove the call to merge in the CLI args from Shoryuken::Runner#run. What do you think?

shoryuken/lib/shoryuken/runner.rb

Lines 33 to 34 in b0dc0c5

	# When cli args exist, override options in config file
	Shoryuken.options.merge!(options)

[csebryam]

That makes sense. Also, removing this line will clear things up as to when/how the options get loaded and avoids further confusion.

[cjlarose]

Is it possible to rely solely on the CLI arg options[:rails] here? It's not clear to me in what situation options[:rails] would be falsey here, but Shoryuken.options[:rails] would be truthy here, especially since by the time this method is invoked, initialize_options has not yet run.

[csebryam]

I would be ok with just having options[:rails], that is how I'm using it. Though, I wanted to keep the existing functionality in case someone was enabling it through the config/shoryuken.yml file.

[cjlarose]

Correct me if I'm wrong, but Shoryuken.options[:rails] won't be populated by the value in config/shoryuken.yml until initialize_options runs. So if the intent is to preserve functionality of setting rails: true in the config file, I don't think this code accomplishes that.

Either way, I don't think it's necessary to preserve the behavior of allowing rails: true to be set in the config file because it's a bit circular: if we have to read the config file to know whether or not to load rails, but we also don't want to read the config file until after initializing Rails, that seems messy.

[csebryam]

That's right, by the time initialize_options runs, it has already passed the option to initialize_rails.

if we have to read the config file to know whether or not to load rails, but we also don't want to read the config file until after initializing Rails, that seems messy.

That is a very good point!

Moving forward with this change to only allowing CLI arg to load rails.

[cjlarose]

Changes look perfect. I'll have to take another pass later and do some testing on some of my own applications, but I think this is just about good to go.

[cjlarose]

Tested on my own applications. Looks great! Thanks so much for this!

[gondalez]

I just wanted to say a belated thank you for this change, it helped to dry up our app's config nicely 🙏