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

Introduce APP_ENV and remove RACK_ENV #984

Merged
merged 1 commit into from Aug 17, 2016
Merged

Introduce APP_ENV and remove RACK_ENV #984

merged 1 commit into from Aug 17, 2016

Conversation

dmathieu
Copy link

RACK_ENV should not be used with values other than development and deployment. 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_ENV to have the deployment value.
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 with SINATRA_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.

@@ -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

Choose a reason for hiding this comment

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

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_sym

Then the use of RACK_ENV could be deprecated and checking it removed in the next major version.

Choose a reason for hiding this comment

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

Sorry, I missed your note about this being for 2.0. Perhaps my suggestion would enable this to be merged to the main branch?

Copy link
Author

Choose a reason for hiding this comment

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

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.

@zzak zzak added the feature label Mar 20, 2015
@zzak zzak added this to the future milestone Mar 20, 2015
@zzak
Copy link
Member

zzak commented Mar 20, 2015

Sorry you are hitting this bug.. thank you for the thorough investigation! <3

@jeremy
Copy link

jeremy commented Apr 2, 2015

Changing to SINATRA_ENV is legit for its own sake. However, doing it response to this blog post's reimagination of RACK_ENV may be premature.

TL;DR: Nobody's been doing it wrong; RACK_ENV is the Rack app environment. Rack::Server had an -E environment but adopted RACK_ENV from server & framework usage.

Discussion on Rack @ rack/rack#831 (comment) and Rails @ rails/rails#19404 (comment)

@jeremy
Copy link

jeremy commented Apr 2, 2015

History:

@zzak
Copy link
Member

zzak commented Jan 31, 2016

@dmathieu Needs a rebase <3

@dmathieu
Copy link
Author

Done 😸

@art-solopov
Copy link

😮 I didn't know this before! Apparently, my blog runs by pure coincidence.

@zzak
Copy link
Member

zzak commented May 9, 2016

@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_sym

This would allow users to choose SINATRA_ENV or fallback to the RACK_ENV if available.

I don't think we can simply ignore RACK_ENV but make a suggestion to enlighten people about it's proper usage.

But maybe RACK_ENV is always set, somewhere, so this warning will get annoying.

/cc @gudmundur

@mperham
Copy link

mperham commented Jul 11, 2016

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 APP_ENV so we don't have RAILS_ENV, PUMA_ENV, SINATRA_ENV variables for every piece?

@dmathieu
Copy link
Author

@zzak that works for me. I've updated my PR.
I don't mind using APP_ENV either.

@zzak
Copy link
Member

zzak commented Jul 11, 2016

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 RACK_ENV properly since we can safely assume anyone using Sinatra is also, and intending to, use Rack as well.

But since I feel that ship has sailed, I'm ok with using APP_ENV if there aren't any objections. I guess it depends on whether other frameworks are willing to adopt it?

I don't mind being the first.

@mperham
Copy link

mperham commented Jul 14, 2016

Happy to add it to sidekiq and I bet we could talk evanphx into adding it to puma. It should spread from there.

@gudmundur
Copy link

I'll make sure it lands in https://github.com/interagent/pliny as well.

@zzak
Copy link
Member

zzak commented Jul 30, 2016

@dmathieu Ok, lets change it to APP_ENV and see how it goes.

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 SINATRA_ENV, and still support RACK_ENV so I'm not of afraid of no 👻

@dmathieu
Copy link
Author

dmathieu commented Aug 1, 2016

LGTM. I've changed this PR to use APP_ENV instead of SINATRA_ENV.

@zzak zzak changed the title Introduce SINATRA_ENV and remove RACK_ENV Introduce APP_ENV and remove RACK_ENV Aug 17, 2016
@zzak zzak merged commit 8c2504c into sinatra:master Aug 17, 2016
phawk added a commit to phawk/sinatra-api that referenced this pull request May 18, 2017
simonneutert pushed a commit to simonneutert/sinatras-skeleton that referenced this pull request May 19, 2017
@stefansundin
Copy link
Contributor

The website still references RACK_ENV, can anyone update that? http://sinatrarb.com/configuration.html

@ioquatix
Copy link
Contributor

ioquatix commented Jan 8, 2020

Sorry to necro-bump this issue but after several years, have we actually reached any consensus or change on this issue?

@henrik
Copy link

henrik commented Jan 13, 2020

@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

@mperham
Copy link

mperham commented Jan 14, 2020

I've added support for APP_ENV in Sidekiq master.

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

Successfully merging this pull request may close these issues.

None yet

10 participants