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

Update pumactl to remove development as default environment #2035

Merged

Conversation

AndrewSpeed
Copy link
Contributor

What changed?

This PR:

  • removes the default environment of development from pumactl
  • defaults to development when searching for config files to load, if one is not provided to pumactl
  • passes the value passed to the environment option over RACK_ENV to pumactl, if provided

fixes #2023

@AndrewSpeed AndrewSpeed force-pushed the dont-default-environment-to-development branch from e021a7f to 6b7b2ed Compare October 15, 2019 21:53
Copy link
Contributor

@composerinteralia composerinteralia left a comment

Choose a reason for hiding this comment

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

🎉

end

def test_environment_without_rack_env
ENV.delete "RACK_ENV" # remove from travis
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of mutating the environment, which could lead to order-dependent test failures if there is another test that unexpectedly relies on a specific environment variable being set, it might be nice to reuse the with_env helper.

Or, if we are OK with adding an additional dependency, at thoughtbot we often use climate_control for this sort of thing. It has the advantage of being threadsafe, which might help if we plan to parallelize these tests.

Copy link
Member

Choose a reason for hiding this comment

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

Let's just use with_env for now, but keep our eyes on climate_control for the future.

def test_environment_specific_config_file_exist
ENV.delete "RACK_ENV"
port = 6002
Dir.mktmpdir do |tmp_dir|
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of this file manipulation makes the test a bit more difficult to read. What do you think about extracting a helper like:

def with_config_file(path_to_config, port)
  path = Pathname.new(path_to_config)
  Dir.mktmpdir do |tmp_dir|
    Dir.chdir(tmp_dir) do
      FileUtils.mkdir(path.dir)
      File.open(path, "w") { |f| f << "port #{port}" }
      yield
    end
  end
end

def test_environment_specific_config_file_exist
  port = 6002
  path_to_config = "config/puma.rb"

  with_config_file(path_to_config, port) do
    control_cli = Puma::ControlCLI.new ["-e", "production", "halt"]
    assert_equal path_to_config,
      control_cli.instance_variable_get("@config_file")
  end
end

@@ -0,0 +1,16 @@
class TestConfigFileBase < Minitest::Test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seemed like the most appropriate place for this file, but let me know if there's another convention I should be following

@@ -1,26 +1,10 @@
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/config_file"
Copy link
Member

Choose a reason for hiding this comment

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

Works for me!

@nateberkopec nateberkopec merged commit aa457c1 into puma:master Oct 18, 2019
@nateberkopec
Copy link
Member

Solid PR, thanks for the contribution 👍

@AndrewSpeed AndrewSpeed deleted the dont-default-environment-to-development branch October 18, 2019 10:09
hahmed pushed a commit to hahmed/puma that referenced this pull request Oct 18, 2019
* Update pumactl to remove development as default environment

* Extract with_config_file test helper method

* Extract config file base test class, use with_env for pumactl tests
@jdoss jdoss mentioned this pull request Nov 1, 2019
This was referenced Mar 14, 2021
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.

Pumactl broken environment option
3 participants