Local environment variables to address issue #553

[lirantal]

implemented feature to address issue #553 - provide a local.js file for local development configuration parameters that will not get committed to remote repositories and will accidentally expose API keys, passwords, and sensitive local configuration

[lirantal]

@trainerbill let's leave the environment variables for another PR because I want to separate the work on both of these angles.

about requiring the local.js, what's wrong with that? it's not removing definitions defined in other environment configs. For example:
in development.js you have: app: {title: 'a dev app'}, app_secret_password: ''
in local.js you have only app_secret_password: '234'
in the config you'll get both app: {title: 'a dev app'}, app_secret_password: '234'

The difference between loadashe's _extend and _merge is simply that merge traverses inside objects but I don't see it being that much of an issue, and it's actually for the best to be more explicit on the objects you put as configurations even if the facebook API URL is the same on development.js as well as local.js.

Anyway, I forgot that it can't find env/local.js now because I added it in gitignore first and should've done that afterwards so I still need to fix it.

[lirantal]

btw @trainerbill I was working on it during bus ride to work... :-)

[lirantal]

@roieki we're good, do you want to quickly review?

[trainerbill]

I think this looks good. I do think you should change to _.merge though to keep local.js clean. You would only want to put the config parameters that you don't want git tracked in local.js. All the other params should remain in the respected env .js config.

[lirantal]

@trainerbill that's exactly the way it works, you can put in local.js only the objects you want to override, and if you override an object it will be entirely, not just single elements, but I think that's ok for now until someone can prove a useful case where it's requiring more flexibility there.

[simison]

Actually, e.g. these:

  mailer: {
    service: 'Gmail',
    options: {
      auth: { user: 'username_gmail', pass: 'pass_gmail' }
    }
  },

and

  mailer: {
    options: {
      host: 'smtp.example.com',
      port: '587',
      auth: { user: 'username', pass: 'pass' }
    }
  },

...would be combined to this:

  mailer: {
    service: 'Gmail',
    options: {
      host: 'smtp.example.com',
      port: '587',
      auth: { user: 'username', pass: 'pass' }
    }
  },

And thus that would be a problem with _.merge(). Right?

[trainerbill]

@lirantal I guess I would prefer the object not to be overridden fully. For example this is what I am doing:

development.js:

paypal: {
        clientID: process.env.PAYPAL_ID || 'APP_ID',
        clientSecret: process.env.PAYPAL_SECRET || 'APP_SECRET',
        callbackURL: '/auth/paypal/callback',
        sandbox: true
    },

local.js

paypal: {
        clientID: 'blah2',
        clientSecret: 'blah2',

    }

If I do this, paypal.callbackURL and paypal.sandbox will be lost. If I put both in local.js then when I move to production the sandbox: true will follow and I will need to make the update to local.js. It would be best if local.js updated the env.js with only what is provided and does not lose information from env.js. That way the enviroment config remains consistent and local.js only updates the information you don't want tracked by git. My production.js looks like:

paypal: {
        clientID: process.env.PAYPAL_ID || 'APP_ID',
        clientSecret: process.env.PAYPAL_SECRET || 'APP_SECRET',
        callbackURL: '/auth/paypal/callback'
    },

And it would work without me making additional changes to local.js. I think you will find there will be many issues like this if you go the full override route.

[lirantal]

@trainerbill I reconsidered and you're right. Right now I'm also copying development.js over to local.js as a default when you run grunt but I guess that's not really ideal either? say you now build a production instance then it will copy over development.js stuff to local.js and will override anything that's on production. What do you think?

[lirantal]

@trainerbill we can do something like angular-fullstack does which is include a local.sample.js file which is committed to git but is just used include some basic explanation and possibly commented configuration and then copy that one into local.js, regardless of the environment we are currently running.

What do you say?

[trainerbill]

Yeah I like that idea. As long as by copy, you mean merge and not overwrite! If you want to model something like angular fullstack then I would suggest looking at how they import the local.env.js to the process.env object in the gruntfile. In my opinion, this is the ideal situation since by default the env/config files are looking for process.env variables.

linkedin: {
        clientID: process.env.LINKEDIN_ID || 'APP_ID',
        clientSecret: process.env.LINKEDIN_SECRET || 'APP_SECRET',
        callbackURL: '/auth/linkedin/callback'
    },

That way all you need in local.env.js is

  LINKEDIN_ID: 'bla',
 LINKEDIN_SECRET: 'blah'

Then the default env/config.js files will pick up the local configuration automatically. I may be biased because that is the way I have been doing it for awhile now, but it seems to be a super slick process. That way you can even specifically export LINKEDIN_ID as well and it would work as normal.

[rschwabco]

+1 on the local.sample.js file - it makes it clear that it's just a sample, and should be renamed (and updated) before using it. I think a helper script could also be used, just to make sure you don't end up committing sensitive info. Maybe a script that checks if the environment vars are defined, and if not - prompts you to define them (and perhaps even exports them for, maybe adds them to a .bashrc file you point at it?)

[lirantal]

@roieki, @trainerbill please review, everything has been taken into account and build passes.
@simison I addressed your situation as well, there is only extend() between environments and a merge() for the local.

[simison]

👍

P.S. Something like this with 0.4.0 branch:

  var config = _.extend(
    require(path.join(process.cwd(), 'config/env/default')),
    require(path.join(process.cwd(), 'config/env/', process.env.NODE_ENV)) || {}
  );
  config = _.merge(config, (fs.existsSync('./config/env/local.js') && require('./env/local.js')) || {});

[lirantal]

Great, I'll merge and we can work it out with 0.4 later

[lirantal]

re-opening for merge.

[ilanbiala]

Shouldn't this have been merged into 0.4.0?

[lirantal]

it made sense to me to merge it on master as a small and immediate fix

[ilanbiala]

@lirantal but then when 0.4.0 is merged into master, we can't just choose 0.4.0 as the copy to keep...which means conflicts..

[lirantal]

@ilanbiala you're absolutely right. To amend this I will get the same kind of PR change into 0.4 and merge it if tests are successful to make sure this won't have any conflicts further.

[ilanbiala]

:)