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 initialization when the Load Rails option is specified #197

Merged

Conversation

yujideveloper
Copy link
Contributor

@yujideveloper yujideveloper commented Apr 20, 2016

I want to use gems like SettingsLogic with Rails environements in the Shoryuken configuration file.
So, Rails must be loaded before loading the Shoryuken configuration file.

load_rails
else
initialize_options
initialize_logger
Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to use gems like SettingsLogic

@yujideveloper how would that let people to use --rails and load settings without using SettingsLogic or similar?

Choose a reason for hiding this comment

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

ERB can't be processed if using something from the Rails project, eg.

queues:
  - [<%= Rails.application.config.some_setting %>, 1]
  - [<%= Settings.some_other_setting %>, 1]

because options are initialised before the Rails environment is loaded. That config file results in an uninitialized constant error.

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 for slow response.
What I want to say is same as this comment.

@mariokostelac
Copy link
Contributor

I think you are right, we have to load rails upfront. This functionality was somehow broken in last release.

@mariokostelac
Copy link
Contributor

I think approach is good, but let's add some clarity to the codeflow.
load_rails loads config and initialises the logger in https://github.com/yujideveloper/shoryuken/blob/654536c35187ead505e8389c1635f58f9327c065/lib/shoryuken/environment_loader.rb#L102-L104. This makes initialize method messy because it's not clear why logger and options do not have to be loaded if we are loading rails.
I'd rather remove log line from load_rails (because it requires logger) and have initializer like:

load_rails if options[:rails]
initialize_options
initialize_logger
...

@yujideveloper
Copy link
Contributor Author

yujideveloper commented Jun 6, 2016

@mariokostelac @phstc
Thank you for review.
I've updated codes. How do you think?

@mariokostelac
Copy link
Contributor

I've checked manually against the codebase that was not able to have shoryuken upgraded because of loading config before rails and I am happy to say it does work!
Awesome work @yujideveloper.

@mariokostelac mariokostelac merged commit 555953f into ruby-shoryuken:master Jun 6, 2016
@yujideveloper yujideveloper deleted the bugfix/fix-initialization branch June 6, 2016 09:00
@eugeneius eugeneius mentioned this pull request Feb 26, 2017
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.

None yet

4 participants