Introduce APP_ENV and remove RACK_ENV #984
Conversation
| @@ -1834,7 +1834,7 @@ def self.force_encoding(data, *) data end | |||
|
|
|||
| reset! | |||
|
|
|||
| set :environment, (ENV['RACK_ENV'] || :development).to_sym | |||
| set :environment, (ENV['SINATRA_ENV'] || :development).to_sym | |||
There was a problem hiding this comment.
I agree with the idea of this change but perhaps there's a middle ground as this would be a significant breaking change as it stands:
set :environment, (ENV['SINATRA_ENV'] || ENV['RACK_ENV'] || :development).to_symThen the use of RACK_ENV could be deprecated and checking it removed in the next major version.
There was a problem hiding this comment.
Sorry, I missed your note about this being for 2.0. Perhaps my suggestion would enable this to be merged to the main branch?
There was a problem hiding this comment.
I think enabling SINATRA_ENV, with a deprecation message when falling back to RACK_ENV, and then entirely disabling it in 2.0 makes sense.
|
Sorry you are hitting this bug.. thank you for the thorough investigation! <3 |
|
Changing to TL;DR: Nobody's been doing it wrong; RACK_ENV is the Rack app environment. Rack::Server had an Discussion on Rack @ rack/rack#831 (comment) and Rails @ rails/rails#19404 (comment) |
|
History:
|
|
@dmathieu Needs a rebase <3 |
|
Done 😸 |
|
😮 I didn't know this before! Apparently, my blog runs by pure coincidence. |
|
@dmathieu How do you feel about this: # warn if RACK_ENV is set, to recommend SINATRA_ENV here, then:
set :environment, (ENV['SINATRA_ENV'] || ENV['RACK_ENV'] || :development).to_symThis would allow users to choose I don't think we can simply ignore But maybe /cc @gudmundur |
|
All of these pieces (Rails, Sinatra, puma, thin, sidekiq, etc) are all application infrastructure processes. We want to determine what environment this application process is running in. Why not |
|
@zzak that works for me. I've updated my PR. |
|
One thing I'll add to the discussion regarding this is, APP_ENV only seems appropriate if everyone wasn't using Rack to begin with. We might as well just use But since I feel that ship has sailed, I'm ok with using I don't mind being the first. |
|
Happy to add it to sidekiq and I bet we could talk evanphx into adding it to puma. It should spread from there. |
|
I'll make sure it lands in https://github.com/interagent/pliny as well. |
|
@dmathieu Ok, lets change it to Worst case, we're ahead of the curve which never gains traction and we just introduce Yet Another Environment Variable to peoples apps. In which case, we do anyways if it was |
|
LGTM. I've changed this PR to use APP_ENV instead of SINATRA_ENV. |
zzak
changed the title
|
The website still references RACK_ENV, can anyone update that? http://sinatrarb.com/configuration.html |
|
Sorry to necro-bump this issue but after several years, have we actually reached any consensus or change on this issue? |
|
@ioquatix I just came here from seeing the 2.0 release mentioning APP_ENV is now preferred, so it's listed there at any rate :) https://github.com/sinatra/sinatra/blob/master/CHANGELOG.md#200--2017-04-10 |
|
I've added support for APP_ENV in Sidekiq master. |
RACK_ENVshould not be used with values other thandevelopmentanddeployment. Therefore, it can't be used to check for `production environment.See http://www.hezmatt.org/~mpalmer/blog/2013/10/13/rack_env-its-not-for-you.html
In fact, it can even cause some issues if your app is using unicorn, as they're including internal rack middlewares relying on
RACK_ENVto have thedeploymentvalue.See https://github.com/defunkt/unicorn/blob/master/lib/unicorn.rb#L56-L79
Rack itself does that too.
https://github.com/rack/rack/blob/4e4ab39b0508aa3e59f5d7e53696ef6ae7c220ed/lib/rack/server.rb#L228
This removes using
RACK_ENV, and replaces it withSINATRA_ENV.Since it's a major changes, it should be merged in the Sinatra 2.0 branch. I'm not seeing anything of the kind, so feel free to stall this until there's discussion about it.