-
Notifications
You must be signed in to change notification settings - Fork 139
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 unicorn pid auto-detection behavior #88
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,35 +2,20 @@ module CapistranoUnicorn | |
module Utility | ||
|
||
def local_unicorn_config | ||
File.exist?(unicorn_config_rel_file_path) ? | ||
unicorn_config_rel_file_path | ||
: unicorn_config_stage_rel_file_path | ||
File.exist?(unicorn_config_file_path) ? | ||
unicorn_config_file_path | ||
: unicorn_config_stage_file_path | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please can you explain why these 3 lines need to be changed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, so there are two separate issues: one is finding the path to the unicorn config file (like @michalorman experienced), and the other is finding the path for the pid file, right? If so, these should be handled in separate commits. |
||
end | ||
|
||
def extract_pid_file | ||
tmp = Tempfile.new('unicorn.rb') | ||
begin | ||
conf = local_unicorn_config | ||
tmp.write <<-EOF.gsub(/^ */, '') | ||
config_file = "#{conf}" | ||
|
||
# stub working_directory to avoid chdir failure since this will | ||
# run client-side: | ||
def working_directory(path); end | ||
|
||
instance_eval(File.read(config_file), config_file) if config_file | ||
puts set[:pid] | ||
exit 0 | ||
EOF | ||
tmp.close | ||
extracted_pid = `unicorn -c "#{tmp.path}"` | ||
$?.success? ? extracted_pid.rstrip : nil | ||
rescue StandardError => e | ||
return nil | ||
ensure | ||
tmp.close | ||
tmp.unlink | ||
end | ||
code = <<-EOC.gsub(/^ */, '').gsub(/\n/, '; ') | ||
cfg = Unicorn::Configurator.new(:config_file => '#{local_unicorn_config}') | ||
puts cfg.set[:pid] | ||
exit 0 | ||
EOC | ||
|
||
pid = capture("cd #{current_path} && unicorn -e \"#{code}\"", :roles => unicorn_roles).rstrip | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switching from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
|
||
pid == "unset" ? nil : File.expand_path(pid, app_path) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, this is probably the culprit which caused #85 in the case where the pid file is relative. |
||
|
||
# Check if a remote process exists using its pid file | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,39 +32,23 @@ | |
end | ||
|
||
it "should default to a sensible pid file when auto-detection failed" do | ||
@configuration.should_receive(shell).with(/unicorn -c /).and_return('') do |cmd| | ||
`false` # Simulate failure by setting $? | ||
end | ||
@configuration.should_receive(:capture).and_return('unset') | ||
@configuration.logger.stub(:important) | ||
@configuration.fetch(:unicorn_pid).should == app_path + "/tmp/pids/unicorn.pid" | ||
end | ||
|
||
shared_examples "auto-detect pid file from unicorn config" do | ||
|pid_file, primary_exists, config_file| | ||
which_config = primary_exists ? 'primary' : 'stage' | ||
it "should auto-detect pid file from #{which_config} unicorn config" do | ||
# Tempfile.new in Ruby 1.9.2 will call File.exist? | ||
allow(File).to receive(:exist?).with(/tmp/) | ||
|
||
File.should_receive(:exist?).with('config/unicorn.rb').and_return(primary_exists) | ||
tmpfile = nil | ||
@configuration.should_receive(shell).with(/unicorn -c /) do |cmd| | ||
(cmd =~ /^unicorn -c "(.+)"$/).should be_true | ||
tmpfile = $~[1] | ||
tmpfile.should include("tmp") | ||
File.read(tmpfile).should include(%!config_file = "#{config_file}"!) | ||
`true` # Simulate success by setting $? | ||
pid_file | ||
end | ||
@configuration.fetch(:unicorn_pid).should == pid_file | ||
shared_examples "auto-detect pid file from unicorn config" do |pid_file| | ||
it "should auto-detect pid file from unicorn config" do | ||
@configuration.should_receive(:capture).and_return(pid_file) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this looks sloppy to me. It means that most of the code in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think better solution is to add new spec file There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's probably fine, as long as all the code paths get tested. |
||
@configuration.fetch(:unicorn_pid).should == File.expand_path(pid_file, app_path) | ||
end | ||
end | ||
|
||
include_examples "auto-detect pid file from unicorn config", \ | ||
'/path/to/pid/from/config/file', true, "config/unicorn.rb" | ||
'/path/to/pid/from/config/file' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
||
include_examples "auto-detect pid file from unicorn config", \ | ||
'/path/to/pid/from/stage/config/file', false, "config/unicorn/production.rb" | ||
'relative/path/to/pid' | ||
|
||
specify "Gemfile should default correctly" do | ||
@configuration.fetch(:bundle_gemfile).should == app_path + "/Gemfile" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not keen on this - why did you remove the error logging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it's just mistake. I will revert it.