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

Overload environment variables #61

Merged
merged 2 commits into from Jan 24, 2014

Conversation

Projects
None yet
8 participants
@ecbypi
Contributor

ecbypi commented Sep 26, 2013

Addresses #38 and #57 and is an alternative to #58. Allows overriding ENV variables via Dotenv.overload via Dotenv::Environment#apply! that follows the ruby convention of using a bang for destructive methods.

This adds some duplication between Dotenv.load, Dotenv.overload and to an extent, Dotenv.load!. I'd be happy to update the pull request to address that or submit a follow-up request that does after getting some feedback.

ecbypi added some commits Sep 25, 2013

Add `Dotenv::Environment#apply!`
`#apply` will override any existing environment variables addressing
dotenv#38 and dotenv#57.
Add `Dotenv.overload`
Provides an interface to `Dotenv::Environment#apply`, overriding any
environment variables if they are already set.
@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Oct 4, 2013

YES! I can really really endorse this PR. So much in fact that I will be using @ecbypi's commits in my own fork till merged. This is great!!!

metaskills commented Oct 4, 2013

YES! I can really really endorse this PR. So much in fact that I will be using @ecbypi's commits in my own fork till merged. This is great!!!

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Oct 4, 2013

BTW, I created this blog post on the loveliness of the new .overload feature here. http://metaskills.net/2013/10/03/using-dotenv-in-rails/

metaskills commented Oct 4, 2013

BTW, I created this blog post on the loveliness of the new .overload feature here. http://metaskills.net/2013/10/03/using-dotenv-in-rails/

@fgrehm

This comment has been minimized.

Show comment
Hide comment
@fgrehm

fgrehm Oct 4, 2013

👍 for this :)

fgrehm commented Oct 4, 2013

👍 for this :)

@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Oct 4, 2013

Contributor

@metaskills The dotenv gem already does what you explained in your blog post without this patch. The .load method can take the path to any number of environment files.

Dotenv.load '.env.test', '.env'

Since Dotenv::Environment#apply only sets ENV variables if they aren't set (using ||=), any values in .env.test will take precedence over .env. If you add dotenv-rails to your Gemfile, the railtie does this for you.

The Dotenv.overload method is for allowing you to reload changes in the same .env files. The use case that motivated the patch was when restarting unicorn (see #57, #38). Since the new process master inherits its environment from the old one, any changes to the .env file are not applied when restarting unicorn when using the USR2 signal. Placing a call to Dotenv.overload in a before_fork block will effect any changes in the .env file.

Contributor

ecbypi commented Oct 4, 2013

@metaskills The dotenv gem already does what you explained in your blog post without this patch. The .load method can take the path to any number of environment files.

Dotenv.load '.env.test', '.env'

Since Dotenv::Environment#apply only sets ENV variables if they aren't set (using ||=), any values in .env.test will take precedence over .env. If you add dotenv-rails to your Gemfile, the railtie does this for you.

The Dotenv.overload method is for allowing you to reload changes in the same .env files. The use case that motivated the patch was when restarting unicorn (see #57, #38). Since the new process master inherits its environment from the old one, any changes to the .env file are not applied when restarting unicorn when using the USR2 signal. Placing a call to Dotenv.overload in a before_fork block will effect any changes in the .env file.

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Oct 4, 2013

Here is an example application usage to talk around. The link is to a dotenv branch with this single commit added.

The dotenv gem already does what you explained in your blog post without this patch.

@ecbypi Yes, was aware that it does take any number of files and I did read the code for that railtie but had issues with the use case of files I setup. If I change over to just dotenv-rails and remove my two lines in application.rb and test_helper.rb, it all goes to pot. My test runs never use the .env.test file. Etc. Below is my expected output from said application. Do you mind showing me a better way? BTW, love the unicorn advice!

$ rails console
>> GmoneyClient.url # => DEVELOPMENT

$ GMONEY_URL=SHELL rails console
>> GmoneyClient.url # => SHELL

$ rails console production
>> GmoneyClient.url # => PRODUCTION

$ rails console test
>> GmoneyClient.url # => TEST

$ rake test:units
>> GmoneyClient.url.must_equal 'TEST'
>> PASSED

$ echo "GMONEY_URL=LOCAL" > .env.local

$ rails console
>> GmoneyClient.url # => LOCAL

$ GMONEY_URL=SHELL rails console
>> GmoneyClient.url # => LOCAL

$ rake test:units
>> GmoneyClient.url.must_equal 'TEST'
>> PASSED

metaskills commented Oct 4, 2013

Here is an example application usage to talk around. The link is to a dotenv branch with this single commit added.

The dotenv gem already does what you explained in your blog post without this patch.

@ecbypi Yes, was aware that it does take any number of files and I did read the code for that railtie but had issues with the use case of files I setup. If I change over to just dotenv-rails and remove my two lines in application.rb and test_helper.rb, it all goes to pot. My test runs never use the .env.test file. Etc. Below is my expected output from said application. Do you mind showing me a better way? BTW, love the unicorn advice!

$ rails console
>> GmoneyClient.url # => DEVELOPMENT

$ GMONEY_URL=SHELL rails console
>> GmoneyClient.url # => SHELL

$ rails console production
>> GmoneyClient.url # => PRODUCTION

$ rails console test
>> GmoneyClient.url # => TEST

$ rake test:units
>> GmoneyClient.url.must_equal 'TEST'
>> PASSED

$ echo "GMONEY_URL=LOCAL" > .env.local

$ rails console
>> GmoneyClient.url # => LOCAL

$ GMONEY_URL=SHELL rails console
>> GmoneyClient.url # => LOCAL

$ rake test:units
>> GmoneyClient.url.must_equal 'TEST'
>> PASSED
@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Oct 5, 2013

Contributor

@metaskills I recommend using foreman to set ENV variables before the process starts or stubbing out ENV variables in test_helper.rb. When you run rake, the RAILS_ENV defaults to development. You can solve this without foreman by prefixing the command with RAILS_ENV=test. Dotenv.overload also solves this, but it isn't the intended use case.

Contributor

ecbypi commented Oct 5, 2013

@metaskills I recommend using foreman to set ENV variables before the process starts or stubbing out ENV variables in test_helper.rb. When you run rake, the RAILS_ENV defaults to development. You can solve this without foreman by prefixing the command with RAILS_ENV=test. Dotenv.overload also solves this, but it isn't the intended use case.

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Oct 5, 2013

@ecbypi Thanks for replying back. All of these details on how Rails boots are well known to me.

I want to be clear that (a) The dotenv gem does not already do what I needed and (b) that my use case for Dotenv.overload is a valid one even though it it not your intended use case. I believe this pull request is a great addition to this gem and my goal was to show a broader need.

metaskills commented Oct 5, 2013

@ecbypi Thanks for replying back. All of these details on how Rails boots are well known to me.

I want to be clear that (a) The dotenv gem does not already do what I needed and (b) that my use case for Dotenv.overload is a valid one even though it it not your intended use case. I believe this pull request is a great addition to this gem and my goal was to show a broader need.

@zacharydanger

This comment has been minimized.

Show comment
Hide comment
@zacharydanger

zacharydanger Oct 14, 2013

👍 The .overload method would be very useful for me, too.

zacharydanger commented Oct 14, 2013

👍 The .overload method would be very useful for me, too.

@allaire

This comment has been minimized.

Show comment
Hide comment
@allaire

allaire Nov 4, 2013

👍 I think its also a valid approach for #45@metaskills I guess you had this previous issue (issue 45) with your tests and that's why needed overload ?

allaire commented Nov 4, 2013

👍 I think its also a valid approach for #45@metaskills I guess you had this previous issue (issue 45) with your tests and that's why needed overload ?

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills commented Nov 4, 2013

@allaire Yes, exactly.

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Nov 5, 2013

@bkeepers Is there any desire to merge this? I've noticed this project's activity is still up but have not seen you comment either way.

metaskills commented Nov 5, 2013

@bkeepers Is there any desire to merge this? I've noticed this project's activity is still up but have not seen you comment either way.

@bkeepers

This comment has been minimized.

Show comment
Hide comment
@bkeepers

bkeepers Nov 6, 2013

Owner

Sorry, I haven't had a lot of time to participate.

One of the reasons I haven't commented is I've been wrestling with #71. Dotenv was originally meant to just be a shim in development, but people started using it in production and some of those features snuck in. I'm considering going back to the core being development-only, and having an additional gem(s) for production features.

Owner

bkeepers commented Nov 6, 2013

Sorry, I haven't had a lot of time to participate.

One of the reasons I haven't commented is I've been wrestling with #71. Dotenv was originally meant to just be a shim in development, but people started using it in production and some of those features snuck in. I'm considering going back to the core being development-only, and having an additional gem(s) for production features.

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Nov 6, 2013

@bkeepers, thanks for the response.

I love that idea for distinct gems, but I would suggest that overload be a core feature, a building block if you will, that allows others to write gems or directly work with Dotenv.

To me it is analogous to a solid Hash interface that core should provide. With that in place, other gems like dotenv-rails can not be required to monkey patch but instead use the building blocks provided.

metaskills commented Nov 6, 2013

@bkeepers, thanks for the response.

I love that idea for distinct gems, but I would suggest that overload be a core feature, a building block if you will, that allows others to write gems or directly work with Dotenv.

To me it is analogous to a solid Hash interface that core should provide. With that in place, other gems like dotenv-rails can not be required to monkey patch but instead use the building blocks provided.

@huydx

This comment has been minimized.

Show comment
Hide comment
@huydx

huydx Nov 12, 2013

Hi everyone, @bkeepers , I'm getting an issue of zero down-time deploy with capistrano, and .env is not get reloaded after I change some value. I think my issue is related to this pull-request but it seems that this pull-request is not yet being merged, so could anyone tell me how to fix my problem.
I know that dotenv means to be used in development but my source code has been using many ENV variable already, and set ENV manually is so frustrate. So is there any walkaround exists?

huydx commented Nov 12, 2013

Hi everyone, @bkeepers , I'm getting an issue of zero down-time deploy with capistrano, and .env is not get reloaded after I change some value. I think my issue is related to this pull-request but it seems that this pull-request is not yet being merged, so could anyone tell me how to fix my problem.
I know that dotenv means to be used in development but my source code has been using many ENV variable already, and set ENV manually is so frustrate. So is there any walkaround exists?

@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Nov 12, 2013

Contributor

@huydx You can add the following lines to a before_exec block in your unicorn config:

before_exec do
  env = Dotenv::Environment.new('.env')
  env.each { |k, v| ENV[k] = v }
end

If you use my fork with the changes I've made, you can do the following:

before_exec do
  Dotenv.overload
end
Contributor

ecbypi commented Nov 12, 2013

@huydx You can add the following lines to a before_exec block in your unicorn config:

before_exec do
  env = Dotenv::Environment.new('.env')
  env.each { |k, v| ENV[k] = v }
end

If you use my fork with the changes I've made, you can do the following:

before_exec do
  Dotenv.overload
end
@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Nov 12, 2013

Contributor

@bkeepers I found a simple way to accomplish what I need when restarting unicorn in two lines of code (noted in my comment to @huydx). I initially thought this would be a useful addition to the gem, but I now feel it might be dangerous to add a utility to override ENV variables in production. Since the gem already provides the utilities to do this in a way more explicit than a vague method call, I'm fine if you want to close/reject this pull request.

I'd be happy to add some information about alternative ways to accomplish what this pull request accomplishes to the README or the project's wiki.

Contributor

ecbypi commented Nov 12, 2013

@bkeepers I found a simple way to accomplish what I need when restarting unicorn in two lines of code (noted in my comment to @huydx). I initially thought this would be a useful addition to the gem, but I now feel it might be dangerous to add a utility to override ENV variables in production. Since the gem already provides the utilities to do this in a way more explicit than a vague method call, I'm fine if you want to close/reject this pull request.

I'd be happy to add some information about alternative ways to accomplish what this pull request accomplishes to the README or the project's wiki.

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Nov 12, 2013

I want to be clear that (a) The dotenv gem does not already do what I needed
and (b) that my use case for Dotenv.overload is a valid one even though it it not
your intended use case.

From my earlier comment ☝️

I'd be happy to add some information about alternative ways to accomplish what this pull request accomplishes

@ecbypi Can those examples not be limited to your use case and encapsulate mine?

metaskills commented Nov 12, 2013

I want to be clear that (a) The dotenv gem does not already do what I needed
and (b) that my use case for Dotenv.overload is a valid one even though it it not
your intended use case.

From my earlier comment ☝️

I'd be happy to add some information about alternative ways to accomplish what this pull request accomplishes

@ecbypi Can those examples not be limited to your use case and encapsulate mine?

@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Nov 12, 2013

Contributor

@metaskills Of course.

Contributor

ecbypi commented Nov 12, 2013

@metaskills Of course.

@bkeepers

This comment has been minimized.

Show comment
Hide comment
@bkeepers

bkeepers Nov 12, 2013

Owner

I found a simple way to accomplish what I need when restarting unicorn in two lines

This can actually be even more concise.

before_exec do
  ENV.update Dotenv::Environment.new('.env')
end
Owner

bkeepers commented Nov 12, 2013

I found a simple way to accomplish what I need when restarting unicorn in two lines

This can actually be even more concise.

before_exec do
  ENV.update Dotenv::Environment.new('.env')
end
@ecbypi

This comment has been minimized.

Show comment
Hide comment
@ecbypi

ecbypi Nov 12, 2013

Contributor

😮 Cool

Contributor

ecbypi commented Nov 12, 2013

😮 Cool

@huydx

This comment has been minimized.

Show comment
Hide comment
@huydx

huydx Nov 13, 2013

@bkeepers @ecbypi cool, I've merge @ecbypi code to my local dotenv env and it just work. Btw, overloading seems to be a dangerous way to deal with ENV variable, but how about overloading with some Warning log?

huydx commented Nov 13, 2013

@bkeepers @ecbypi cool, I've merge @ecbypi code to my local dotenv env and it just work. Btw, overloading seems to be a dangerous way to deal with ENV variable, but how about overloading with some Warning log?

@adambrod

This comment has been minimized.

Show comment
Hide comment
@adambrod

adambrod Jan 2, 2014

This thread was super-helpful for me. For the record, with Unicorn 4.6.3 I had to use the code in my unicorn.rb (the earlier example was missing the server param and unicorn didn't like that):

before_exec do |server|
  # Make sure all the .env properties are updated
  ENV.update Dotenv::Environment.new('.env')
end

adambrod commented Jan 2, 2014

This thread was super-helpful for me. For the record, with Unicorn 4.6.3 I had to use the code in my unicorn.rb (the earlier example was missing the server param and unicorn didn't like that):

before_exec do |server|
  # Make sure all the .env properties are updated
  ENV.update Dotenv::Environment.new('.env')
end

bkeepers added a commit that referenced this pull request Jan 24, 2014

@bkeepers bkeepers merged commit eaa70db into bkeepers:master Jan 24, 2014

1 check passed

default The Travis CI build passed
Details
@bkeepers

This comment has been minimized.

Show comment
Hide comment
@bkeepers

bkeepers Jan 24, 2014

Owner

Alright, I decided to just pull this. I can see it being useful, in non-production environments.

Owner

bkeepers commented Jan 24, 2014

Alright, I decided to just pull this. I can see it being useful, in non-production environments.

n3rdgir1 pushed a commit to asynchrony/dotenv that referenced this pull request Feb 19, 2014

sheax0r pushed a commit to sheax0r/dotenv that referenced this pull request Sep 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment