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

Assign envs before preload #267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eval
Copy link

@eval eval commented Mar 2, 2014

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.

@jonleighton
Copy link
Member

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!

@eval
Copy link
Author

eval commented Mar 6, 2014

Thanks @jonleighton for the extensive feedback!
I took a stab at implementing this - but I'm struggling to make the build green 💩
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?

@jonleighton
Copy link
Member

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.

@eval
Copy link
Author

eval commented Apr 16, 2014

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

@eval eval force-pushed the prevent-stale-envs-during-preload branch 3 times, most recently from 5f43198 to 5f51d14 Compare September 23, 2014 16:02
@eval eval force-pushed the prevent-stale-envs-during-preload branch from 5f51d14 to c6471b4 Compare September 23, 2014 16:13
@eval
Copy link
Author

eval commented Sep 23, 2014

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

@jonleighton
Copy link
Member

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

# ...and ENV-keys whose values have not changed since the start of spring
allowed_keys += original_env.select {|k, v| ENV[k] == v }.keys
# never allowed:
allowed_keys -= %w(RUBYOPT RUBY_ROOT BUNDLE_GEMFILE GEM_ROOT GEM_HOME GEM_PATH)
Copy link
Member

Choose a reason for hiding this comment

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

What does this protect against? I'm just concerned about whether this will lead to potential confusion. If someone is fiddling with these vars maybe it's better to just assume they know what they're doing. But maybe you envisaged a scenario where not excluding these vars will cause particular problems?

@jonleighton
Copy link
Member

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

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

Successfully merging this pull request may close these issues.

2 participants