-
Notifications
You must be signed in to change notification settings - Fork 139
Fix unicorn pid auto-detection behavior #88
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
base: master
Are you sure you want to change the base?
Changes from all commits
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 | ||
| 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 #{app_path} && unicorn -e \"#{code}\"", :roles => unicorn_roles).rstrip | ||
| pid == "unset" ? nil : File.expand_path(pid, app_path) | ||
| end | ||
|
Contributor
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) | ||
|
Contributor
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
Author
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
Contributor
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' | ||
|
Contributor
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
_rel_so it leads to thinking the method returns absolute path.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.
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.