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

Add support for APP_ENV environment variable #2702

Conversation

jacobherrington
Copy link
Contributor

Description

Using APP_ENV to set the environment is supported by Sinatra and
Sidekiq, so it makes sense to support the same behavior in Puma.

Like Sidekiq, APP_ENV will take precedence over RACK_ENV and RAILS_ENV.
APP_ENV defers to any argument passed via the --environment flag.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

test/test_cli.rb Outdated

Puma::CLI.new []

assert_equal 'test', ENV['RACK_ENV']
Copy link
Member

Choose a reason for hiding this comment

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

assert_equal 'test', cli.environment

test/test_cli.rb Outdated
ENV['RAILS_ENV'] = @environment
ENV['APP_ENV'] = 'test'

Puma::CLI.new []
Copy link
Member

Choose a reason for hiding this comment

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

cli = Puma::CLI.new []

@nateberkopec nateberkopec added feature waiting-for-changes Waiting on changes from the requestor labels Sep 19, 2021
@nateberkopec
Copy link
Member

One change, otherwise LGTM and thanks for implementing!

Using APP_ENV to set the environment is supported by Sinatra and
Sidekiq, so it makes sense to support the same behavior in Puma.

Like Sidekiq, APP_ENV will take precedence over RACK_ENV and RAILS_ENV.
APP_ENV defers to any argument passed via the --environment flag.

Closes puma#2692
@jacobherrington jacobherrington force-pushed the jacobherrington/add-support-for-app-env branch from ee2a68e to ff6f08c Compare September 20, 2021 15:12
@jacobherrington
Copy link
Contributor Author

@nateberkopec Updated and force pushed the branch!

@jacobherrington
Copy link
Contributor Author

@nateberkopec It looks like Puma::CLI.new [] doesn't have an environment method, is that unexpected?

@nateberkopec nateberkopec merged commit ff6f08c into puma:master Sep 21, 2021
@nateberkopec
Copy link
Member

Fixed it up: f21ae53

I couldn't remember the API.

What I was trying to get across was that the original test didn't actually test anything, it was making assertions about the setup and not about the action you took during the test. Unfortunately all the other tests around this behavior are exactly the same (I just deleted them).

@jacobherrington
Copy link
Contributor Author

jacobherrington commented Sep 21, 2021

@nateberkopec I see. I was curious about how this worked, but I (silly of me) assumed the old tests were trustworthy.

I'll look into writing some new tests to cover the old cases, thanks for your help!

@jacobherrington jacobherrington deleted the jacobherrington/add-support-for-app-env branch September 21, 2021 15:58
@nateberkopec
Copy link
Member

That would be great! If you want to refactor how the CLI object works a bit too so we don't have to send or instance_variable_get that would be nice.

Puma's an old project... some of the tests are good, others are not 😆 I don't blame you for following what was already there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-changes Waiting on changes from the requestor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants