From f013178c11208fc079a1a4b081d37a6c1efea86e Mon Sep 17 00:00:00 2001 From: Andrew Babichev Date: Wed, 12 Dec 2018 00:01:00 +0200 Subject: [PATCH 1/4] Check config file existence --- lib/sidekiq/cli.rb | 4 ++++ test/test_cli.rb | 8 ++++++++ 2 files changed, 12 insertions(+) diff --git a/lib/sidekiq/cli.rb b/lib/sidekiq/cli.rb index d3e5ff0eb..15a687785 100644 --- a/lib/sidekiq/cli.rb +++ b/lib/sidekiq/cli.rb @@ -293,6 +293,10 @@ def validate! die(1) end + if options[:config_file] && !File.exist?(options[:config_file]) + raise ArgumentError, "No such file #{options[:config_file]}" + end + [:concurrency, :timeout].each do |opt| raise ArgumentError, "#{opt}: #{options[opt]} is not a valid value" if options.has_key?(opt) && options[opt].to_i <= 0 end diff --git a/test/test_cli.rb b/test/test_cli.rb index 9fea92f5f..5898a211d 100644 --- a/test/test_cli.rb +++ b/test/test_cli.rb @@ -260,6 +260,14 @@ class TestCLI < Minitest::Test exit = assert_raises(SystemExit) { @cli.parse(%w[sidekiq -r ./test/fixtures]) } assert_equal 1, exit.status end + + describe 'when config file path does not exist' do + it 'raises argument error' do + assert_raises(ArgumentError) do + @cli.parse(%w[sidekiq -r ./test/fake_env.rb -C /non/existent/path]) + end + end + end end end end From 30a44ed54b5eb217d49dc7de04261b7c97011fd1 Mon Sep 17 00:00:00 2001 From: Andrew Babichev Date: Wed, 12 Dec 2018 12:07:47 +0200 Subject: [PATCH 2/4] Eager config file check --- lib/sidekiq/cli.rb | 51 +++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/lib/sidekiq/cli.rb b/lib/sidekiq/cli.rb index 15a687785..96f36fa49 100644 --- a/lib/sidekiq/cli.rb +++ b/lib/sidekiq/cli.rb @@ -27,7 +27,7 @@ class CLI attr_accessor :launcher attr_accessor :environment - def parse(args=ARGV) + def parse(args = ARGV) setup_options(args) initialize_logger validate! @@ -227,17 +227,30 @@ def symbolize_keys_deep!(hash) alias_method :☠, :exit def setup_options(args) + # parse CLI options opts = parse_options(args) + set_environment opts[:environment] - options[:queues] << 'default' if options[:queues].empty? + # check config file presence + if opts[:config_file] + if opts[:config_file] && !File.exist?(opts[:config_file]) + raise ArgumentError, "No such file #{opts[:config_file]}" + end + else + %w[config/sidekiq.yml config/sidekiq.yml.erb].each do |filename| + opts[:config_file] ||= filename if File.exist?(filename) + end + end - cfile = opts[:config_file] - opts = parse_config(cfile).merge(opts) if cfile + # parse config file options + opts = parse_config(opts[:config_file]).merge(opts) if opts[:config_file] + opts[:queues] = Array(opts[:queues]) << 'default' if opts[:queues].nil? || opts[:queues].empty? opts[:strict] = true if opts[:strict].nil? opts[:concurrency] = Integer(ENV["RAILS_MAX_THREADS"]) if !opts[:concurrency] && ENV["RAILS_MAX_THREADS"] + # merge with defaults options.merge!(opts) end @@ -293,10 +306,6 @@ def validate! die(1) end - if options[:config_file] && !File.exist?(options[:config_file]) - raise ArgumentError, "No such file #{options[:config_file]}" - end - [:concurrency, :timeout].each do |opt| raise ArgumentError, "#{opt}: #{options[opt]} is not a valid value" if options.has_key?(opt) && options[opt].to_i <= 0 end @@ -371,11 +380,8 @@ def parse_options(argv) logger.info @parser die 1 end - @parser.parse!(argv) - %w[config/sidekiq.yml config/sidekiq.yml.erb].each do |filename| - opts[:config_file] ||= filename if File.exist?(filename) - end + @parser.parse!(argv) opts end @@ -396,22 +402,17 @@ def write_pid end def parse_config(cfile) - opts = {} - if File.exist?(cfile) - opts = YAML.load(ERB.new(IO.read(cfile)).result) || opts - - if opts.respond_to? :deep_symbolize_keys! - opts.deep_symbolize_keys! - else - symbolize_keys_deep!(opts) - end + opts = YAML.load(ERB.new(IO.read(cfile)).result) || {} - opts = opts.merge(opts.delete(environment.to_sym) || {}) - parse_queues(opts, opts.delete(:queues) || []) + if opts.respond_to? :deep_symbolize_keys! + opts.deep_symbolize_keys! else - # allow a non-existent config file so Sidekiq - # can be deployed by cap with just the defaults. + symbolize_keys_deep!(opts) end + + opts = opts.merge(opts.delete(environment.to_sym) || {}) + parse_queues(opts, opts.delete(:queues) || []) + ns = opts.delete(:namespace) if ns # logger hasn't been initialized yet, puts is all we have. From 4366b9e26ee6c890092059117c0c7fd641de6c43 Mon Sep 17 00:00:00 2001 From: Andrew Babichev Date: Wed, 12 Dec 2018 12:28:32 +0200 Subject: [PATCH 3/4] Parse expanded path to default sidekiq.yml config file in Rails app --- lib/sidekiq/cli.rb | 11 +++++++---- test/dummy/config/sidekiq.yml | 2 ++ test/test_cli.rb | 14 +++++++++++++- 3 files changed, 22 insertions(+), 5 deletions(-) create mode 100644 test/dummy/config/sidekiq.yml diff --git a/lib/sidekiq/cli.rb b/lib/sidekiq/cli.rb index 96f36fa49..d70ab036e 100644 --- a/lib/sidekiq/cli.rb +++ b/lib/sidekiq/cli.rb @@ -238,8 +238,11 @@ def setup_options(args) raise ArgumentError, "No such file #{opts[:config_file]}" end else - %w[config/sidekiq.yml config/sidekiq.yml.erb].each do |filename| - opts[:config_file] ||= filename if File.exist?(filename) + if File.directory?(opts[:require]) + %w[config/sidekiq.yml config/sidekiq.yml.erb].each do |filename| + path = File.expand_path(filename, opts[:require]) + opts[:config_file] ||= path if File.exist?(path) + end end end @@ -401,8 +404,8 @@ def write_pid end end - def parse_config(cfile) - opts = YAML.load(ERB.new(IO.read(cfile)).result) || {} + def parse_config(path) + opts = YAML.load(ERB.new(File.read(path)).result) || {} if opts.respond_to? :deep_symbolize_keys! opts.deep_symbolize_keys! diff --git a/test/dummy/config/sidekiq.yml b/test/dummy/config/sidekiq.yml new file mode 100644 index 000000000..774e4c88f --- /dev/null +++ b/test/dummy/config/sidekiq.yml @@ -0,0 +1,2 @@ +--- +:concurrency: 25 diff --git a/test/test_cli.rb b/test/test_cli.rb index 5898a211d..9de77837f 100644 --- a/test/test_cli.rb +++ b/test/test_cli.rb @@ -244,6 +244,18 @@ class TestCLI < Minitest::Test assert_equal 3, Sidekiq.options[:queues].count { |q| q == 'seldom' } end end + + describe 'default config file' do + describe 'when required path is a directory' do + focus + it 'tries config/sidekiq.yml' do + @cli.parse(%w[sidekiq -r ./test/dummy]) + + assert_equal 'sidekiq.yml', File.basename(Sidekiq.options[:config_file]) + assert_equal 25, Sidekiq.options[:concurrency] + end + end + end end end @@ -285,7 +297,7 @@ class TestCLI < Minitest::Test after do Sidekiq.logger = @logger end - + describe 'pidfile' do it 'writes process pid to file' do Sidekiq.options[:pidfile] = '/tmp/sidekiq.pid' From 3c1252fc53fe8c6c6f1ca09ce4e8cd496e9c49e3 Mon Sep 17 00:00:00 2001 From: Andrew Babichev Date: Wed, 12 Dec 2018 12:35:12 +0200 Subject: [PATCH 4/4] Cleanup --- lib/sidekiq/cli.rb | 8 ++++---- test/test_cli.rb | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/sidekiq/cli.rb b/lib/sidekiq/cli.rb index d70ab036e..afa236278 100644 --- a/lib/sidekiq/cli.rb +++ b/lib/sidekiq/cli.rb @@ -251,7 +251,7 @@ def setup_options(args) opts[:queues] = Array(opts[:queues]) << 'default' if opts[:queues].nil? || opts[:queues].empty? opts[:strict] = true if opts[:strict].nil? - opts[:concurrency] = Integer(ENV["RAILS_MAX_THREADS"]) if !opts[:concurrency] && ENV["RAILS_MAX_THREADS"] + opts[:concurrency] = Integer(ENV["RAILS_MAX_THREADS"]) if opts[:concurrency].nil? && ENV["RAILS_MAX_THREADS"] # merge with defaults options.merge!(opts) @@ -429,10 +429,10 @@ def parse_queues(opts, queues_and_weights) queues_and_weights.each { |queue_and_weight| parse_queue(opts, *queue_and_weight) } end - def parse_queue(opts, q, weight=nil) + def parse_queue(opts, queue, weight = nil) opts[:queues] ||= [] - raise ArgumentError, "queues: #{q} cannot be defined twice" if opts[:queues].include?(q) - [weight.to_i, 1].max.times { opts[:queues] << q } + raise ArgumentError, "queues: #{queue} cannot be defined twice" if opts[:queues].include?(queue) + [weight.to_i, 1].max.times { opts[:queues] << queue } opts[:strict] = false if weight.to_i > 0 end end diff --git a/test/test_cli.rb b/test/test_cli.rb index 9de77837f..07aa95729 100644 --- a/test/test_cli.rb +++ b/test/test_cli.rb @@ -247,7 +247,6 @@ class TestCLI < Minitest::Test describe 'default config file' do describe 'when required path is a directory' do - focus it 'tries config/sidekiq.yml' do @cli.parse(%w[sidekiq -r ./test/dummy])