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

Allow using env var to specify pidfile #36486

Merged
merged 1 commit into from Jun 20, 2019
Merged

Allow using env var to specify pidfile #36486

merged 1 commit into from Jun 20, 2019

Conversation

benthorner
Copy link
Contributor

@benthorner benthorner commented Jun 14, 2019

Summary

Previously it was only possible to specify the location of the pidfile
for the 'rails server' command with the '-P' flag. This adds support for
specifying the pidfile using a PIDFILE env var, which can still be
overridden by the '-P' flag and with the default pidfile path unchanged.

The motivation for this feature comes from using Docker to run multiple
instances of the same rails app. When developing a rails app with
Docker, it's common to bind-mount the rails root directory in the
running container, so that changes to files are shared between the
container and the host. However, this doesn't work so well with the
pidfile and it's necessary to (remember to) add a '-P' flag to the
'rails server' command line; being able to specify this flag using an
env var would make developing with Rails+Docker a bit simpler.

Other Information

@@ -1,3 +1,5 @@
# (Unreleased)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to add a section header.


- Support using environment variable to set pidfile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -114,7 +114,7 @@ class ServerCommand < Base # :nodoc:
desc: "Runs server as a Daemon."
class_option :using, aliases: "-u", type: :string,
desc: "Specifies the Rack server used to run the application (thin/puma/webrick).", banner: :name
class_option :pid, aliases: "-P", type: :string, default: DEFAULT_PID_PATH,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change affected to an output of help. Please add a default value to desc like port and binding.

@y-yagi
Copy link
Member

y-yagi commented Jun 15, 2019

You should need to fix prepare_restart also.

def prepare_restart
FileUtils.rm_f(options[:pid]) if options[:restart]
end

@benthorner
Copy link
Contributor Author

@y-yagi thanks for your comments - first time working on Rails and I missed a few things :-). I've addressed them all - couldn't find a particular test for the prepare_restart method, but I changed it to use the pid method now.

@y-yagi
Copy link
Member

y-yagi commented Jun 18, 2019

I missed to point out user_supplied_options. Can you fix to user_supplied_options whenPIDFILE env is specified?

user_supplied_options << :Host if ENV["HOST"] || ENV["BINDING"]
user_supplied_options << :Port if ENV["PORT"]
user_supplied_options.uniq

Previously it was only possible to specify the location of the pidfile
for the 'rails server' command with the '-P' flag. This adds support for
specifying the pidfile using a PIDFILE env var, which can still be
overridden by the '-P' flag and with the default pidfile path unchanged.

The motivation for this feature comes from using Docker to run multiple
instances of the same rails app. When developing a rails app with
Docker, it's common to bind-mount the rails root directory in the
running container, so that changes to files are shared between the
container and the host. However, this doesn't work so well with the
pidfile and it's necessary to (remember to) add a '-P' flag to the
'rails server' command line; being able to specify this flag using an
env var would make developing with Rails+Docker a bit simpler.
@benthorner
Copy link
Contributor Author

@y-yagi sure, done :-).

@y-yagi y-yagi merged commit bf625f7 into rails:master Jun 20, 2019
@y-yagi
Copy link
Member

y-yagi commented Jun 20, 2019

@benthorner Thanks!!

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

Successfully merging this pull request may close these issues.

None yet

2 participants