spring icon indicating copy to clipboard operation
spring copied to clipboard

Assign envs before preload

Open eval opened this issue 11 years ago • 7 comments

Currently ENV's of the client are not up to date during initialization of the app.

When having the following in config/application.rb

...
    config.force_ssl = !!ENV['FORCE_SSL']
...

The following occurs:

  1. FORCE_SSL=1 RAILS_ENV=test bin/rails runner 'p Rails.application.config.force_ssl' # => true
  2. RAILS_ENV=development bin/rails runner 'p Rails.application.config.force_ssl' # => true

If you reverse these steps, the result for both steps would be false.

Obviously we want 1 to yield true, and 2 to yield false.

eval avatar Mar 02 '14 14:03 eval

Thanks for the PR, some comments:

  • Have you thought about what happens if the application process gets reloaded? I.e. if you touch config/application.rb, when then will be the force_ssl value of the development environment? (lib/spring/application/boot.rb will call eager_preload)
  • Now that we're mutating the environment in the parent process, I think it would be worth doing the mutation where we need it, and then reverting ENV back to its previous value - my concern is that just leaving it as it is may give rise to other bugs.
  • Please write to the initialiser in the actual test rather than in helper.rb, as it only relates to this one test and doing it in the test makes it clearer what is going on.
  • Please add to CHANGELOG.md (under a new "Next release" heading)

Thanks!

jonleighton avatar Mar 02 '14 15:03 jonleighton

Thanks @jonleighton for the extensive feedback! I took a stab at implementing this - but I'm struggling to make the build green :poop: It's a bit unclear to me how the different env's (original_env, ENV and client_env) should be treated. Could you take a look and post some more pointers/comments?

eval avatar Mar 06 '14 00:03 eval

I took a look at this the other day and realised that it needs some more fundamental changes to make it work properly. I'll come back to it at some point when I can, but it might take me a while I'm afraid.

jonleighton avatar Apr 15 '14 21:04 jonleighton

If I can help in the process; let me know! :cake:

eval avatar Apr 16 '14 11:04 eval

@jonleighton can we give this PR another try? I rebased and updated the code to make it pass.

The original issue is solved now: application:<rails_env> gets initialized with the ENV from the command that starts it.

This is not perfect as we still don't fully 'watch' ENV (for subsequent commands using the same rails-env), but it's much better than the current situation where you simply cannot rely on ENV anywhere in config/**/*.rb.

WDYT?

eval avatar Sep 23 '14 16:09 eval

@eval Sorry for the slow reply. I'll take another look.

jonleighton avatar Nov 24 '14 22:11 jonleighton

@eval This looks good to me. Could rebase it please? Could you also document how we treat the ENV in the README? There are some nuances so I think it would prevent confusion to spell this out to people (the main one being that if you start the app with FORCE_SSL=1 and then set FORCE_SSL=0, it won't do what you expect). Thanks for your patience.

jonleighton avatar Nov 24 '14 22:11 jonleighton