Add option to specify environment (default: RAILS_ENV)

[neilwilliams]

Reopening this PR, this time with less changes.

The primary purpose of this PR is to allow a rails app to configure where the 'environment' comes from. Previously, it was inferred from Rails.environment. We didn't want to pollute RAILS_ENV with actual environments (e.g. stage, preprod). So this change allows the two things to be separated.

Also includes a fix to allow contributors to run bundle exec rspec when using a standard ruby platform.

[neilwilliams]

@Fryguy / @pkuczynski I've raised this PR as requested.

[pkuczynski]

@neilwilliams looks good to me, however, this PR is missing:

• CHANGELOG entry
• explaining new option in README.md#general
• unit test

@Fryguy what you think?

[neilwilliams]

@neilwilliams looks good to me, however, this PR is missing:

• CHANGELOG entry
• explaining new option in README.md#general
• unit test

@Fryguy what you think?

@pkuczynski Thanks for the feedback.

I've updated the CHANGELOG, README and reverted version back down.

I haven't done a unit test yet... there isn't already a test for the railtie?

[cjlarose]

@neilwilliams Sorry you haven't gotten feedback on this change since your last push. @pkuczynski has added me as a maintainer and I'm helping out with PR triage now.

This PR looks almost ready to go. I added the waiting for change label, though, because it's missing tests like @pkuczynski pointed out.

I haven't done a unit test yet... there isn't already a test for the railtie?

Ideally, we'd have a test for the new branch that was introduced in this PR. We'd want a test that explicitly configures Config to use a different environment, and then verify that settings were loaded from the YAML file corresponding to the specified environment.

I'm not 100% sure if the tests are set up in a way that makes writing this new test easy, since the behavior we're testing is in the Railtie and that means spinning up a Rails app. If you're feeling ambitious @neilwilliams, feel free to dig into solving this problem. If not, I can hopefully find the time to add that new test infrastructure myself.

[neilwilliams]

@neilwilliams Sorry you haven't gotten feedback on this change since your last push. @pkuczynski has added me as a maintainer and I'm helping out with PR triage now.

This PR looks almost ready to go. I added the waiting for change label, though, because it's missing tests like @pkuczynski pointed out.

I haven't done a unit test yet... there isn't already a test for the railtie?

Ideally, we'd have a test for the new branch that was introduced in this PR. We'd want a test that explicitly configures Config to use a different environment, and then verify that settings were loaded from the YAML file corresponding to the specified environment.

I'm not 100% sure if the tests are set up in a way that makes writing this new test easy, since the behavior we're testing is in the Railtie and that means spinning up a Rails app. If you're feeling ambitious @neilwilliams, feel free to dig into solving this problem. If not, I can hopefully find the time to add that new test infrastructure myself.

Hi @cjlarose

@ChorjoDev Has been helping out with this, and we think we have a solution now - using the existing appraisal structure.

Will hopefully have something for you to review soon.

[neilwilliams]

Hi @cjlarose,

We've hit a bit of a brick wall with this test!

In order to specifically test the rails initialisation of the config object, we have to add the initializer into each app within the existing appraisal setup. e.g.

Config.setup do |config|
  config.environment = 'any_env'
  config.const_name = 'RailtieSettings'
  config.env_parse_values = true
end

With a new yml in config/settings/any_env.yml:

#
# An environment file that doesn't follow
# the default rails environments
#
some_api:
  scheme: https
  host: some-non-rails-environment
  path: /some-path

Then in an rspec test, attempt to assert the outcome:

require 'spec_helper'

context 'when a the environment config is set' do
  it 'should load a file that matches that environment' do
    if defined?(::Rails)
      expect(RailtieSettings.some_api.scheme).to eq 'https'
      expect(RailtieSettings.some_api.host).to eq 'some-non-rails-environment'
      expect(RailtieSettings.some_api.path).to eq '/some-path'
    end
  end
end

However, since this initial initialisation then conflicts with the rspec setup, and the resetting that occurs in the other tests, the new test causes the test pack to fail intermittantly. it's all to do with the timing of the tests and the order in which they run - horrible.

I'm not really sure how we can progress with this - only to say that by leaving the new initialzers in place, we're getting code coverage of the Railtie - we're just not able to consistently assert the outcome of it.

We've tried various different ways to get around this, but feel like we're no longer being productive trying.

I'd love to get this in, so we can stop referencing a github branch in our repos - but interested to hear your view on this, whether there's a better way to test this. I have a feeling this is why there wasn't a test there previously 😄

I'll push up what we have.

[neilwilliams]

Hi @cjlarose

Any ideas on this one? Happy for me to fix the build and get it in as it is?

We're in the process of upgrading to ruby 3.1 and rails 7, so I need to get this version up to date with your master anyway (the get the psych 4 fixes)- but it would be nice to get this functionality into master for anyone else to use too.

[neilwilliams]

I'm going to reraise this as a new commit - it's well out of date now.