fix multiple issues relating to variables #70

aspiers · 2013-08-20T01:50:35Z

fixes rails_env / unicorn_env / app_env are redundant or undocumented #46
- documents the variables properly
- renames misleading and broken app_env variable
fixes Support for --no-default-middleware flag #64 via a new unicorn_options variable
fixes one or two other minor issues
adds multistage docs to the README
bump version to 0.2.0
adds a NEWS file
fixes You should set the pid path via the command line... #7

modojenkins · 2013-08-21T11:36:16Z

+1

aspiers · 2013-08-22T19:27:08Z

I just removed misleading info from https://github.com/sosedoff/capistrano-unicorn/wiki/Using-capistrano-unicorn-with-multistage-environment regarding app_env. However in future I think we should avoid storing docs in the wiki and instead store them in the git repo, because it's crucial that docs are coupled to the code they are documenting. When a commit changes the code, it must also update the corresponding docs. If they are in different places they can get out of sync far too easily.

sosedoff · 2013-08-22T19:32:54Z

@aspiers we can use "docs" folder to store manuals, better than on-site wiki

aspiers · 2013-08-22T19:39:32Z

Rebased after merge of #69.

aspiers · 2013-08-22T19:40:34Z

@sosedoff Agreed! Luckily there is only one wiki page at the moment, and this pull request makes that obsolete anyway ...

aspiers · 2013-08-23T15:13:33Z

Any feedback?

sfsekaran · 2013-08-23T18:44:22Z

This all looks good to me, after a quick code review. It's all straightforward.

aspiers · 2013-08-24T09:12:59Z

@sfsekaran Thanks! Should I merge then? Not sure what the protocol is for how many +1s are required for this.

aspiers · 2013-08-24T13:46:50Z

OK, I think I'm done now. I ended up making several more changes, including a fix for #7, i.e. auto-detection of the pid file path from the unicorn config. I also added a NEWS.md file since all these changes could get a bit confusing, and are not 100% backwards-compatible.

Would greatly appreciate someone testing this branch, since there are quite a lot of changes and I could have broken something (although I took great care not to!)

aspiers · 2013-08-29T23:42:10Z

Hmm, I'll have to rebase this now that we have a test suite :-/

sfsekaran · 2013-08-29T23:44:19Z

It would be awesome if you had time to write a few tests!

—

Sathya Sekaran
Sent from Mailbox for iPhone

On Thu, Aug 29, 2013 at 4:42 PM, Adam Spiers notifications@github.com
wrote:

Hmm, I'll have to rebase this now that we have a test suite :-/

Reply to this email directly or view it on GitHub:
#70 (comment)

aspiers · 2013-08-29T23:45:15Z

Already working on it :)

aspiers · 2013-09-02T08:59:50Z

Almost finished this yesterday, will push soon ...

… variables This is the first step towards resolving sosedoff#46 and sosedoff#64.

This contributes towards resolving sosedoff#46 and sosedoff#64.

This is the final commit in the series which fixes sosedoff#46.

…s to unicorn Fixes sosedoff#64 via: set :unicorn_options, '-N' (although I'm not sure why you'd need this over setting :unicorn_rack_env to 'none').

Accurate docs for this is now coupled to the release, and should not live in the wiki, since claims about app_env in older versions may or may not be accurate, but they certainly aren't at this point in the code base.

Grouping all path calculations together improves legibility and maintainability.

Also re-order and re-structure assignment and display of variables for legibility.

aspiers · 2013-09-02T20:58:21Z

OK, this has pretty thorough tests now, and I fixed a few minor issues whilst writing them. So I'm pretty confident this is good to merge :)

aspiers · 2013-09-06T10:39:13Z

Please can someone review and merge?

sfsekaran · 2013-09-06T10:44:06Z

@aspiers Have you tested this out on a production env yet? It all looks good to me otherwise.

aspiers · 2013-09-06T10:46:21Z

Yes I'm using it to deploy two public apps (although they are tiny and low risk). The new unicorn:show_vars task helps build confidence that it is calculating the right values, before you run any task which actually deploys anything. But the test suite is also pretty comprehensive at this point.

sfsekaran · 2013-09-06T10:50:30Z

@aspiers Sounds great. Merging.

fix multiple issues relating to variables

sfsekaran · 2013-09-06T10:52:50Z

@sosedoff When you get a chance--it's time for that gem release to v0.2.0.

sfsekaran · 2013-09-06T10:57:25Z

@aspiers Thank you for all the hard work! This was an excellent effort. I especially appreciate the attention to documentation here.

aspiers · 2013-09-06T10:58:58Z

@sfsekaran Thanks :) I hate the idea of anyone else being confused by the docs in the same way I was, so I had to fix them :)

sosedoff · 2013-09-06T22:20:40Z

When i run test suite i get 4 failures even though travis build passes. Ruby 2.0.0p247:

Failures:

  1) CapistranoUnicorn::CapistranoIntegration loaded into a configuration testing variables app paths app in current_path it should behave like an app in path should auto-detect pid file from primary unicorn config
     Failure/Error: @configuration.fetch(:unicorn_pid).should == pid_file
       expected: "/path/to/pid/from/config/file"
            got: "/path/to/myapp/tmp/pids/unicorn.pid" (using ==)
     Shared Example Group: "an app in path" called from ./spec/capistrano_integration_spec.rb:93
     # ./spec/capistrano_integration_spec.rb:60:in `block (6 levels) in <top (required)>'

  2) CapistranoUnicorn::CapistranoIntegration loaded into a configuration testing variables app paths app in current_path it should behave like an app in path should auto-detect pid file from stage unicorn config
     Failure/Error: @configuration.fetch(:unicorn_pid).should == pid_file
       expected: "/path/to/pid/from/stage/config/file"
            got: "/path/to/myapp/tmp/pids/unicorn.pid" (using ==)
     Shared Example Group: "an app in path" called from ./spec/capistrano_integration_spec.rb:93
     # ./spec/capistrano_integration_spec.rb:60:in `block (6 levels) in <top (required)>'

  3) CapistranoUnicorn::CapistranoIntegration loaded into a configuration testing variables app paths app in a subdirectory it should behave like an app in path should auto-detect pid file from primary unicorn config
     Failure/Error: @configuration.fetch(:unicorn_pid).should == pid_file
       expected: "/path/to/pid/from/config/file"
            got: "/path/to/myapp/mysubdir/tmp/pids/unicorn.pid" (using ==)
     Shared Example Group: "an app in path" called from ./spec/capistrano_integration_spec.rb:103
     # ./spec/capistrano_integration_spec.rb:60:in `block (6 levels) in <top (required)>'

  4) CapistranoUnicorn::CapistranoIntegration loaded into a configuration testing variables app paths app in a subdirectory it should behave like an app in path should auto-detect pid file from stage unicorn config
     Failure/Error: @configuration.fetch(:unicorn_pid).should == pid_file
       expected: "/path/to/pid/from/stage/config/file"
            got: "/path/to/myapp/mysubdir/tmp/pids/unicorn.pid" (using ==)
     Shared Example Group: "an app in path" called from ./spec/capistrano_integration_spec.rb:103
     # ./spec/capistrano_integration_spec.rb:60:in `block (6 levels) in <top (required)>'

aspiers · 2013-09-06T22:34:38Z

This means the pid file auto-detection, which involves running unicorn -c on a temporary config file, is failing. Maybe you don't have unicorn installed? There is no development dependency on it, and there probably should be ... which begs the question why the Travis runs are succeeding. Weird.

sfsekaran · 2013-09-06T22:36:08Z

Maybe Travis uses Unicorn. :'D

On Friday, September 6, 2013 at 3:34 PM, Adam Spiers wrote:

This means the auto-detection, which involves running unicorn -c on a temporary config file, is failing. Maybe you don't have unicorn installed? There is no development dependency on it, and there probably should be ... which begs the question why the Travis runs are succeeding. Weird.

—
Reply to this email directly or view it on GitHub (#70 (comment)).

sosedoff · 2013-09-07T18:00:22Z

I added debug into build steps, there is no unicorn installed. See the build https://travis-ci.org/sosedoff/capistrano-unicorn/jobs/11100220

aspiers · 2013-09-07T18:03:59Z

Weird, I don't know why yours is failing then. But it should be really easy for you to debug - the code is super simple!

sfsekaran · 2013-09-17T06:27:22Z

Any movement on this? It bothers me--I see four failures locally as well.

aspiers · 2013-09-17T10:29:24Z

@sfsekaran I do not see any failures, just warnings, e.g.

An expectation of :success? was set on nil. Called from /home/adam/.GIT/3rd-party/capistrano-unicorn/spec/config_spec.rb:36:in `block (6 levels) in <top (required)>'. Use allow_message_expectations_on_nil to disable warnings.

I did a git bisect, and the warnings arise from 9c355a5 in #74 which you merged into master.

It's not right to mock $?.success?, because $? can be nil at the time the expectation is set, resulting in errors like: An expectation of :success? was set on nil. Called from /home/adam/.GIT/3rd-party/capistrano-unicorn/spec/config_spec.rb:36:in `block Instead we simply ensure that $? is set to an appropriate value. This may or may not fix the failures that some people have been seeing (mentioned in sosedoff#70).

sfsekaran · 2013-09-17T17:14:54Z

@aspiers Note that I merged #74 fairly recently, and I saw four failures before I paired on it.

aspiers mentioned this pull request Aug 20, 2013

minor configuration changes #67

Closed

aspiers mentioned this pull request Aug 22, 2013

You should set the pid path via the command line... #7

Closed

sfsekaran mentioned this pull request Aug 23, 2013

unicorn:reload sometimes doesn't kill the old pid #33

Closed

Fix 'recpies' typo

300295e

Adam Spiers added 13 commits September 2, 2013 21:42

Accurately document unicorn_env, app_env, and unicorn_config_filename…

6e91a25

… variables This is the first step towards resolving sosedoff#46 and sosedoff#64.

Make unicorn_env default to rails_env or 'production'

883b1f6

This contributes towards resolving sosedoff#46 and sosedoff#64.

Rename app_env to unicorn_rack_env and fix default value

4a93a19

This is the final commit in the series which fixes sosedoff#46.

Add unicorn_options variable which allows passing of arbitrary option…

a39f9b2

…s to unicorn Fixes sosedoff#64 via: set :unicorn_options, '-N' (although I'm not sure why you'd need this over setting :unicorn_rack_env to 'none').

Add unicorn:show_vars task for debugging variable values

1695cfb

Eliminate unnecessary blanket rescue

3100501

Clarify the default for unicorn_user

f0811cf

Embed multistage docs directly within the README

c9739ad

Accurate docs for this is now coupled to the release, and should not live in the wiki, since claims about app_env in older versions may or may not be accurate, but they certainly aren't at this point in the code base.

ensure RAILS_ENV is set correctly

4b09dec

simplify quoting of long shell command

ee092db

add some quotes to be safe

ed1241b

add NEWS.md to aid with upgrades

01498d4

fix incorrect docs for app_subdir

320f2e3

Adam Spiers added 6 commits September 2, 2013 21:43

remove unnecessary use of fetch

27a5dbd

move primary and secondary config paths into _cset section

614684f

Grouping all path calculations together improves legibility and maintainability.

rename primary_config_path and secondary_config_path for clarity

7549d86

add relative path calculations in preparation for pid auto-detection

00a5f2e

Also re-order and re-structure assignment and display of variables for legibility.

attempt to auto-detect path of pid file (fixes sosedoff#7)

e5e4108

fix tense used when describing default variable values

c9e5c2c

sfsekaran added a commit that referenced this pull request Sep 6, 2013

Merge pull request #70 from aspiers/variables

887b69c

fix multiple issues relating to variables

sfsekaran merged commit 887b69c into sosedoff:master Sep 6, 2013

aspiers deleted the variables branch September 6, 2013 10:58

aspiers mentioned this pull request Sep 17, 2013

fix warnings about expectations of :success? on nil #76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix multiple issues relating to variables #70

fix multiple issues relating to variables #70

aspiers commented Aug 20, 2013

modojenkins commented Aug 21, 2013

aspiers commented Aug 22, 2013

sosedoff commented Aug 22, 2013

aspiers commented Aug 22, 2013

aspiers commented Aug 22, 2013

aspiers commented Aug 23, 2013

sfsekaran commented Aug 23, 2013

aspiers commented Aug 24, 2013

aspiers commented Aug 24, 2013

aspiers commented Aug 29, 2013

sfsekaran commented Aug 29, 2013

Hmm, I'll have to rebase this now that we have a test suite :-/

aspiers commented Aug 29, 2013

aspiers commented Sep 2, 2013

aspiers commented Sep 2, 2013

aspiers commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

aspiers commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

aspiers commented Sep 6, 2013

sosedoff commented Sep 6, 2013

aspiers commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

sosedoff commented Sep 7, 2013

aspiers commented Sep 7, 2013

sfsekaran commented Sep 17, 2013

aspiers commented Sep 17, 2013

sfsekaran commented Sep 17, 2013

fix multiple issues relating to variables #70

fix multiple issues relating to variables #70

Conversation

aspiers commented Aug 20, 2013

modojenkins commented Aug 21, 2013

aspiers commented Aug 22, 2013

sosedoff commented Aug 22, 2013

aspiers commented Aug 22, 2013

aspiers commented Aug 22, 2013

aspiers commented Aug 23, 2013

sfsekaran commented Aug 23, 2013

aspiers commented Aug 24, 2013

aspiers commented Aug 24, 2013

aspiers commented Aug 29, 2013

sfsekaran commented Aug 29, 2013

Hmm, I'll have to rebase this now that we have a test suite :-/

aspiers commented Aug 29, 2013

aspiers commented Sep 2, 2013

aspiers commented Sep 2, 2013

aspiers commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

aspiers commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

aspiers commented Sep 6, 2013

sosedoff commented Sep 6, 2013

aspiers commented Sep 6, 2013

sfsekaran commented Sep 6, 2013

sosedoff commented Sep 7, 2013

aspiers commented Sep 7, 2013

sfsekaran commented Sep 17, 2013

aspiers commented Sep 17, 2013

sfsekaran commented Sep 17, 2013