Add better configuration handling

[jcamenisch]

Goals:

• [ ] Any missing, required environment variables should throw an error at application boot time.
• defaults should be configurable in a central place.

[jcamenisch]

I think this approach might work well: https://gist.github.com/jcamenisch/b878c667a894f4d903a2

[ericboehs]

I don't know, this may be overkill. I think if we just always call ENV.fetch('MY_VAR'), we'll know where it was called and that it's missing. If we add a comment to the line in .env.example.

I would like it to throw at boot time though.

[jcamenisch]

Do you have any simpler/lighter suggestions?

One idea: just assign ENV variables to Rails.configuration in application.rb. Downside: This would de-emphasize the difference between environment-specific stuff and other configurations.

Another idea: just assign each env variable to a Ruby constant at boot time, like

AIRBRAKE_API_KEY = ENV['AIRBRAKE_API_KEY'] || '' # 'see http://airbrake.io/'
WIDGET_PRICE = ENV.fetch('WIDGET_PRICE').to_d   # The price for one widget

This would accomplish almost everything we'd want, but wouldn't be quite as helpful in forcing everyone into the right practices.

You could also do it this way, but that relies too heavily on discipline for my taste. It also doesn't provide the self-documentation benefits for each variable.

[ericboehs]

I almost like the last way. Either that or parse over the .env.example and raise if any are missing.

On Sat, Nov 23, 2013 at 8:17 AM, Jonathan Camenisch notifications@github.com wrote:

Do you have any simpler/lighter suggestions? One idea: just assign ENV variables to Rails.configuration in application.rb. Downside: This would de-emphasize the difference between environment-specific stuff and other configurations. Another idea: just assign each env variable to a Ruby constant at boot time, like

AIRBRAKE_API_KEY = ENV['AIRBRAKE_API_KEY'] || '' # 'see http://airbrake.io/'
WIDGET_PRICE = ENV.fetch('WIDGET_PRICE').to_d   # The price for one widget

This would accomplish almost everything we'd want, but wouldn't be quite as helpful in forcing everyone into the right practices.

You could also do it this way, but that relies too heavily on discipline for my taste. It also doesn't provide the self-documentation benefits for each variable.

Reply to this email directly or view it on GitHub: https://github.com/brightbit/rails-template/issues/2#issuecomment-29133130

[vlucas]

I had this same issue for the PHP version of dotenv I made. I added a required method that I used in the LifeChurch staff app so it can be more self-documenting if LC staff tries to clone and run the project.

The required method throws an exception if any of the ENV vars are missing:

Dotenv::load(__DIR__);
Dotenv::required(array('DB_HOST', 'DB_NAME', 'DB_USER', 'DB_PASS'));

Maybe we could do something similar, or contribute this to the dotenv Ruby gem too?

[ericboehs]

@vlucas that'd be nice

On Sat, Nov 23, 2013 at 2:10 PM, Vance Lucas notifications@github.comwrote:

I had this same issue for the PHP version of dotenvhttps://github.com/vlucas/phpdotenvI made. I added a required method that I used in the LifeChurch staff app so it can be more self-documenting if LC staff tries to close and run the project.

The required method throws an exception if any of the ENV vars are missing:

Dotenv::load(DIR);Dotenv::required(array('DB_HOST', 'DB_NAME', 'DB_USER', 'DB_PASS'));

Maybe we could do something similar, or contribute this to the dotenv Ruby gem too?

— Reply to this email directly or view it on GitHubhttps://github.com/brightbit/rails-template/issues/2#issuecomment-29140522 .

[jcamenisch]

Dotenv is for development. I'm pretty sure they wouldn't accept a PR that's targeted toward a production use case. On Nov 23, 2013 3:18 PM, "Eric Boehs" notifications@github.com wrote:

@vlucas that'd be nice

On Sat, Nov 23, 2013 at 2:10 PM, Vance Lucas notifications@github.comwrote:

I had this same issue for the PHP version of dotenv< https://github.com/vlucas/phpdotenv>I made. I added a required method that I used in the LifeChurch staff app so it can be more self-documenting if LC staff tries to close and run the project.

The required method throws an exception if any of the ENV vars are missing:

Dotenv::load(DIR);Dotenv::required(array('DB_HOST', 'DB_NAME', 'DB_USER', 'DB_PASS'));

Maybe we could do something similar, or contribute this to the dotenv Ruby gem too?

— Reply to this email directly or view it on GitHub< https://github.com/brightbit/rails-template/issues/2#issuecomment-29140522>

.

— Reply to this email directly or view it on GitHubhttps://github.com/brightbit/rails-template/issues/2#issuecomment-29140683 .

[jcamenisch]

This started with the statement that 48 lines of Ruby was overkill, and now we're talking about parsing over a .env.example file when booting the production app? Can you expound a little on what we're trying to shoot for? On Nov 23, 2013 1:30 PM, "Eric Boehs" notifications@github.com wrote:

I almost like the last way. Either that or parse over the .env.example and raise if any are missing.

On Sat, Nov 23, 2013 at 8:17 AM, Jonathan Camenisch notifications@github.com wrote:

Do you have any simpler/lighter suggestions? One idea: just assign ENV variables to Rails.configuration in application.rb. Downside: This would de-emphasize the difference between environment-specific stuff and other configurations. Another idea: just assign each env variable to a Ruby constant at boot time, like

AIRBRAKE_API_KEY = ENV['AIRBRAKE_API_KEY'] || '' # 'see
http://airbrake.io/'
WIDGET_PRICE = ENV.fetch('WIDGET_PRICE').to_d # The price for one widget

This would accomplish almost everything we'd want, but wouldn't be quite as helpful in forcing everyone into the right practices. You could also do it this way, but that relies too heavily on discipline for my taste. It also doesn't

provide the self-documentation benefits for each variable.

Reply to this email directly or view it on GitHub:

https://github.com/brightbit/rails-template/issues/2#issuecomment-29133130

— Reply to this email directly or view it on GitHubhttps://github.com/brightbit/rails-template/issues/2#issuecomment-29138298 .

[jcamenisch]

I think I made a mistake here when I included the functionality to specify a class with each variable spec, and have the library auto-convert the string into the desired class. I think I like the feature, but it's not part of the essence, and it distracts from the comparison of solutions.

To let you know how that got in there, I was looking at some other libraries for idioms on how to design the configuration API. I ended up imitating optparse, because it seemed like the pattern would work pretty well.

Optparse takes a class parameter, and converts the supplied parameter to it. I liked the idea and threw in an implementation of it, adding several lines of code.

I'd rip that out for you to see, but that sounds like a pain to do on my phone. :p

So, this was me playing with some ideas. The API doesn't seem to be hitting you right, or maybe my vision isn't clear. I'd like to understand more what you're thinking.

[vlucas]

Good point. All the required method does is check if the ENV var is set, so it's safe for production use. In PHP-land I'd do something like this:

if(BULLET_ENV !== 'production') {
    Dotenv::load(__DIR__);
}
Dotenv::required(array('DB_HOST', 'DB_NAME', 'DB_USER', 'DB_PASS'));

So dotenv is still loaded as a dependency, but it's not loading and parsing a .env file on each request.

[jcamenisch]

This is what the Cfg module looks like without the class conversion functionality: https://gist.github.com/jcamenisch/b878c667a894f4d903a2 —29 total lines instead of 48.

[ericboehs]

Still way confusing.

I was thinking something simple like: ENV.keys.include? Dotenv.load('.env.example').keys

Edit:

Looks like that doesn't work. The proper way to get the .env.example keys is: Dotenv::Environment.new '.env.example'.

Also the way I was doing the array subset check won't work.

It could be done via: (Dotenv::Environment.new('.env.example').keys - ENV.keys).empty?

Working it out a little more:

missing_vars = Dotenv::Environment.new('.env.example').keys - ENV.keys
raise "Missing ENV vars: #{missing_vars}" if missing_vars.any?

This SO question has a couple other ways to do it.

[jcamenisch]

We don't want everything in .env.example to be required. Some things can have default values or just be left undefined.

What part is confusing? The usage or the implementation? You don't really need to understand the implementation, as long as the usage is clear. On Nov 25, 2013 11:54 AM, "Eric Boehs" notifications@github.com wrote:

Still way confusing.

I was thinking something simple like: ENV.keys.include? Dotenv.load('.env.example').keys

— Reply to this email directly or view it on GitHubhttps://github.com/brightbit/rails-template/issues/2#issuecomment-29218743 .

[ericboehs]

I don't really like the use of a Cfg object when we already have a ENV object.

And the implementation is confusing. I don't see why we should just ignore the implementation and not review it.

[ericboehs]

I don't particularly have a recommendation for adding optional variables aside creating a .env.required file though. I usually prefer the least amount of code. Also I don't see the need to put the description as code. Code comments are fine.

[ericboehs]

Really if we just start using the convention of ENV.fetch('MY_VAR'), this issue could be closed.

[jcamenisch]

Really if we just start using the convention of ENV.fetch('MY_VAR'), this issue could be closed.

That doesn't keep a broken app from booting in production

[jcamenisch]

I don't really like the use of a Cfg object when we already have a ENV object.

The reason for using Object-oriented programming is "encapsulation," which means packaging up some logic so you can reuse it over and over again without having to think again about how it works.

In this case, we need to accomplish a few things:

1. Read an environment variable. (ENV['YOLO'])
2. Fall back on a default if it's not there. (ENV['YOLO'] || 'boo')
3. Convert to the desired type. (ENV.has_key? 'YOLO' && ENV['YOLO'] !~ /false|no|off|disable|0/)
4. Ensure the app fails loudly (with clarity) if the environment variable is missing. (unless ENV.has_key?('YOLO') raise 'You forgot something, yo!'; yolo = ENV['YOLO'] !~ /false|no|off|disable|0/ })
5. Ensure the app fails loudly at boot time, so end users never see a 500 error. (# Go write an initializer somewhere, yo!)

Given those requirements, we could do the "simple" thing, and agree on a few standard practices. Once we establish those practices, all we have to do is communicate them to everyone, including those who join the project in the future, and then just remember to always be good little programmers and follow our best little practices that are written down in some README somewhere.

Being a lazy programmer, with a horrible memory, who spends half my day just trying to remember where I/we put stuff, the above paragraph sounds like pain.

The alternative is to separate out the tedious parts that don't require an actual mind, and delegate those parts to a small bit of code that will do them for us. We could write the code in such a way that it's impossible to forget the stuff we need to do ourselves, and it's pleasant when we do it.

And the implementation is confusing. I don't see why we should just ignore the implementation and not review it.

When you encapsulate something in library code, the idea is that you stop thinking about how it works and just use it.

When you write the library, code review is great, and I'm all for it. However, it seems like that is clouding this discussion, and you're proposing things like using Dotenv.load, which is even harder to understand and review, and is written to solve a different problem.

If we don't want some object other than ENV, we could monkey patch ENV to do all this work for us. Or, we could name our object in a way that might be less confusing to you, like EnvDecorator or EnvWrapper or WhitelistedEnv or WhiteEnv for short, or ... something.

[jcamenisch]

... You could also do it this way, ...

I almost like the last way. ...

I will say, this solved the "don't deploy a broken app" problem beautifully for me in Sounding Board. I pushed some code to Heroku without setting a few needed environment variables, and it refused to boot the new slug, giving me an error message that was easy to understand. I configured the missing variable, and deployed again until I got them all (or finally caved in and compared the whole list from heroku config with my .env file). Even with an unpublished app, I think this saved me a good bit of time vs. waiting for a 500 error to see the problem.

[jcamenisch]

I guess we could always just use this: https://github.com/jcamenisch/ENV_BANG

[ericboehs]

When you replied with your thought out reply (13 days ago), I wanted to respond to it in whole but didn't have a chance to at the time.

For the most part I agreed but didn't like adding the extra file to the project. For some reason as a gem, I like the solution.

Proceed.

[jcamenisch]

Oh, cool. Thanks for letting me talk (argue?) that out.