Permalink
Browse files

Clean up option processing

  • Loading branch information...
1 parent 9da3722 commit 4cca6eb85df1662d429335132857bd7efa6c90f8 @mperham committed Jan 19, 2013
Showing with 45 additions and 16 deletions.
  1. +16 −0 Changes.md
  2. +23 −11 lib/sidekiq/cli.rb
  3. +2 −1 test/env_based_config.yml
  4. +4 −4 test/test_cli.rb
View
@@ -1,3 +1,19 @@
+HEAD
+-----------
+
+- Allow environment-specific sections within the config file which
+override the global values [dtaniwaki, #630]
+
+```
+---
+:concurrency: 50
+:verbose: false
+staging:
+ :verbose: true
+ :concurrency: 5
+```
+
+
2.6.5
-----------
View
@@ -54,22 +54,18 @@ class CLI
# Used for CLI testing
attr_accessor :code
attr_accessor :manager
+ attr_accessor :environment
def initialize
@code = nil
@interrupt_mutex = Mutex.new
@interrupted = false
- @environment = ENV['RAILS_ENV'] || ENV['RACK_ENV']
end
def parse(args=ARGV)
@code = nil
- cli = parse_options(args)
- @environment = cli[:environment] if cli[:environment]
- config = parse_config(cli)
- options.merge!(config.merge(cli))
-
+ setup_options(args)
initialize_logger
validate!
write_pid
@@ -112,16 +108,30 @@ def interrupt
private
+ def set_environment(cli_env)
+ @environment = cli_env || ENV['RAILS_ENV'] || ENV['RACK_ENV'] || 'development'
+ end
+
def die(code)
exit(code)
end
+ def setup_options(args)
+ cli = parse_options(args)
+ set_environment cli[:environment]
+
+ cfile = cli[:config_file]
+
+ config = (cfile ? parse_config(cfile) : {})
+ options.merge!(config.merge(cli))
+ end
+
def options
Sidekiq.options
end
def boot_system
- ENV['RACK_ENV'] = ENV['RAILS_ENV'] = @environment || 'development'
+ ENV['RACK_ENV'] = ENV['RAILS_ENV'] = environment
raise ArgumentError, "#{options[:require]} does not exist" unless File.exist?(options[:require])
@@ -240,12 +250,14 @@ def write_pid
end
end
- def parse_config(cli)
+ def parse_config(cfile)
opts = {}
- if cli[:config_file] && File.exist?(cli[:config_file])
- opts = YAML.load(ERB.new(IO.read(cli[:config_file])).result)
- opts = opts.merge(opts.delete(@environment)) if @environment && opts[@environment].is_a?(Hash)
+ if File.exist?(cfile)
+ opts = YAML.load(ERB.new(IO.read(cfile)).result)
+ opts = opts.merge(opts.delete(environment) || {})
parse_queues opts, opts.delete(:queues) || []
@mfkp

mfkp Feb 19, 2013

This line results in an error if you don't have the :queues set in the config.yml (in a capistrano deployment, same as @plentz below)

@mperham

mperham Feb 19, 2013

Owner

Thanks, see latest commit, look better?

@mfkp

mfkp Feb 19, 2013

Cool, haven't tested it but I imagine that should do the trick. Thanks

+ else
+ raise ArgumentError, "can't find config file #{cfile}"
end
@plentz

plentz Feb 4, 2013

Contributor

@mperham this change made sidekiq config file required when using capistrano.

  * 2013-02-04 00:15:44 00:15:44 == Currently executing `sidekiq:start'
  * executing "cd /var/www/foo/current ; bundle exec sidekiq -d -e production -C /var/www/foo/current/config/sidekiq.yml -i 0 -P /var/www/foo/current/tmp/pids/sidekiq.pid -L /var/www/foo/current/log/sidekiq.log"
    servers: ["foo.com"]
    [foo.com] executing command
 ** [out :: foo.com] can't find config file /var/www/foo/current/config/sidekiq.yml
 ** [out :: foo.com] /var/www/foo/shared/bundle/ruby/1.9.1/gems/sidekiq-2.7.0/lib/sidekiq/cli.rb:321:in `parse_config'
 ** [out :: foo.com] /var/www/foo/shared/bundle/ruby/1.9.1/gems/sidekiq-2.7.0/lib/sidekiq/cli.rb:179:in `setup_options'
 ** [out :: foo.com] /var/www/foo/shared/bundle/ruby/1.9.1/gems/sidekiq-2.7.0/lib/sidekiq/cli.rb:72:in `parse'
 ** [out :: foo.com] /var/www/foo/shared/bundle/ruby/1.9.1/gems/sidekiq-2.7.0/bin/sidekiq:7:in `<top (required)>'
 ** [out :: foo.com] /var/www/foo/shared/bundle/ruby/1.9.1/bin/sidekiq:23:in `load'
 ** [out :: foo.com] /var/www/foo/shared/bundle/ruby/1.9.1/bin/sidekiq:23:in `<main>'
 ** [out :: foo.com] 
    command finished in 2812ms
@mperham

mperham Feb 4, 2013

Owner

The Capistrano integration requires a config file, yes. This is expected.

@plentz

plentz Feb 4, 2013

Contributor

Isn't more interesting to check if sidekiq.yml exists and if yes, add the -C option? A pull request with this change is welcome?

@mperham

mperham Feb 4, 2013

Owner

No, config/sidekiq.yml is required.

opts
end
@@ -1,10 +1,11 @@
---
:pidfile: /tmp/sidekiq-config-test.pid
+:concurrency: 50
staging:
:verbose: false
:require: ./test/fake_env.rb
:logfile: /tmp/sidekiq.log
- :concurrency: 50
+ :concurrency: 5
:queues:
- [<%="very_"%>often, 2]
- [seldom, 1]
View
@@ -112,7 +112,7 @@ class TestCli < MiniTest::Unit::TestCase
Sidekiq.logger.info('test message')
- assert_match /test message/, File.read(@tmp_log_path), "didn't include the log message"
+ assert_match(/test message/, File.read(@tmp_log_path), "didn't include the log message")
end
it 'appends messages to a logfile' do
@@ -125,8 +125,8 @@ class TestCli < MiniTest::Unit::TestCase
Sidekiq.logger.info('test message')
log_file_content = File.read(@tmp_log_path)
- assert_match /already existant/, log_file_content, "didn't include the old message"
- assert_match /test message/, log_file_content, "didn't include the new message"
+ assert_match(/already existant/, log_file_content, "didn't include the old message")
+ assert_match(/test message/, log_file_content, "didn't include the new message")
end
end
@@ -213,7 +213,7 @@ class TestCli < MiniTest::Unit::TestCase
end
it 'sets concurrency' do
- assert_equal 50, Sidekiq.options[:concurrency]
+ assert_equal 5, Sidekiq.options[:concurrency]
end
it 'sets pid file' do

0 comments on commit 4cca6eb

Please sign in to comment.