Skip to content
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

minor configuration changes #67

Closed
wants to merge 1 commit into from
Closed

Conversation

maorhayoun
Copy link

unicorn pid file should be placed at shard path location by default.
unicorn execution should use the unicorn_environment directly (and if not set, use the app_env fallback)

happy to be corrected otherwise

aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this pull request Aug 20, 2013
@aspiers
Copy link
Contributor

aspiers commented Aug 20, 2013

Thanks for the report.

You are right that the pid file should usually be within the shared/ directory, however I think autodetection as described in #7 is a much better approach for the reasons I mentioned there.

Your other change is not quite right - see #70 for what I believe is the correct solution.

Therefore I suggest closing this PR in favour of #7 and #70.

aspiers pushed a commit to aspiers/capistrano-unicorn that referenced this pull request Aug 20, 2013
@maorhayoun
Copy link
Author

no argument about autodetection of pid path as the correct solution,
but as I see it, if no specific override is done to indicate different none-the-usual path for the pid file -
default path should still be valid

@aspiers
Copy link
Contributor

aspiers commented Aug 22, 2013

Agreed, and that's why I added ed30b08 to #70 which will take care of this.

@aspiers aspiers closed this Aug 22, 2013
@aspiers
Copy link
Contributor

aspiers commented Aug 22, 2013

Actually I just remembered that capistrano already automatically sets up a symlink from #{current_path}/tmp/pids to #{shared_path/tmp/pids} by default, so ed30b08 is unnecessary...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants