From bd455afd87c25ba438b492903b1110730631b379 Mon Sep 17 00:00:00 2001 From: Scott Gress Date: Thu, 8 Dec 2016 14:06:37 -0600 Subject: [PATCH] Make our internal `rc` into a function that accepts a namespace arg (defaults to `sails`) This allows `node app.js` to share in the glory of humanized env vars, and protects against the returned object getting mutated --- accessible/rc.js | 2 +- bin/sails-console.js | 2 +- bin/sails-deploy.js | 2 +- bin/sails-generate.js | 2 +- bin/sails-lift.js | 2 +- bin/sails-new.js | 2 +- bin/sails-www.js | 2 +- errors/fatal.js | 2 +- errors/warn.js | 2 +- lib/app/configuration/rc.js | 106 +++++++++--------- ...pawning-sails-lift-child-process-in-cwd.js | 9 +- test/integration/lift.test.js | 48 ++++++++ 12 files changed, 118 insertions(+), 63 deletions(-) diff --git a/accessible/rc.js b/accessible/rc.js index 50f8f3318..876677e58 100644 --- a/accessible/rc.js +++ b/accessible/rc.js @@ -2,7 +2,7 @@ * Module dependencies */ -var rc = require('rc'); +var rc = require('../lib/app/configuration/rc'); /** diff --git a/bin/sails-console.js b/bin/sails-console.js index 6cd409c99..12da43ea5 100644 --- a/bin/sails-console.js +++ b/bin/sails-console.js @@ -9,7 +9,7 @@ var _ = require('@sailshq/lodash'); var chalk = require('chalk'); var CaptainsLog = require('captains-log'); -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); var Sails = require('../lib/app'); var SharedErrorHelpers = require('../errors'); var readReplHistoryAndStartTranscribing = require('./private/read-repl-history-and-start-transcribing'); diff --git a/bin/sails-deploy.js b/bin/sails-deploy.js index 76e1b5b1e..1789fa8b5 100644 --- a/bin/sails-deploy.js +++ b/bin/sails-deploy.js @@ -5,7 +5,7 @@ var _ = require('@sailshq/lodash'); var util = require('util'); var path = require('path'); -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); /** * `sails deploy` diff --git a/bin/sails-generate.js b/bin/sails-generate.js index f6eb6912f..1144b5b3f 100644 --- a/bin/sails-generate.js +++ b/bin/sails-generate.js @@ -10,7 +10,7 @@ var async = require('async'); var CaptainsLog = require('captains-log'); var sailsGen = require('sails-generate'); var package = require('../package.json'); -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); /** * `sails generate` diff --git a/bin/sails-lift.js b/bin/sails-lift.js index 9e3839e97..31dde9c59 100644 --- a/bin/sails-lift.js +++ b/bin/sails-lift.js @@ -7,7 +7,7 @@ var _ = require('@sailshq/lodash'); var chalk = require('chalk'); var captains = require('captains-log'); -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); var Sails = require('../lib/app'); var SharedErrorHelpers = require('../errors'); diff --git a/bin/sails-new.js b/bin/sails-new.js index aa9e84f40..80d57c768 100644 --- a/bin/sails-new.js +++ b/bin/sails-new.js @@ -6,7 +6,7 @@ var nodepath = require('path'); var _ = require('@sailshq/lodash'); var sailsgen = require('sails-generate'); var package = require('../package.json'); -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); var CaptainsLog = require('captains-log'); diff --git a/bin/sails-www.js b/bin/sails-www.js index cb69e410d..cacf34596 100644 --- a/bin/sails-www.js +++ b/bin/sails-www.js @@ -6,7 +6,7 @@ var nodepath = require('path'); var _ = require('@sailshq/lodash'); var CaptainsLog = require('captains-log'); var Process = require('machinepack-process'); -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); diff --git a/errors/fatal.js b/errors/fatal.js index c70ef37b1..010bccb7a 100644 --- a/errors/fatal.js +++ b/errors/fatal.js @@ -9,7 +9,7 @@ var CaptainsLog = require('captains-log'); // Once per process: // Build logger using best-available information // when this module is initially required. -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); var log = CaptainsLog(rconf.log); diff --git a/errors/warn.js b/errors/warn.js index 41f12942c..d34f75587 100644 --- a/errors/warn.js +++ b/errors/warn.js @@ -8,7 +8,7 @@ var CaptainsLog = require('captains-log'); // Once per process: // Build logger using best-available information // when this module is initially required. -var rconf = require('../lib/app/configuration/rc'); +var rconf = require('../lib/app/configuration/rc')(); var log = CaptainsLog(rconf.log); diff --git a/lib/app/configuration/rc.js b/lib/app/configuration/rc.js index 8ba4ae208..d4395de81 100644 --- a/lib/app/configuration/rc.js +++ b/lib/app/configuration/rc.js @@ -7,61 +7,65 @@ var rttc = require('rttc'); var minimist = require('minimist'); /** - * Locate and load .sailsrc files if they exist. - * - * NOTE: this occurs almost immediately when sails is required, - * and since `rc` is synchronous, the examination of env variables, - * cmdline opts, and .sailsrc files is immediate, and happens only once. - * - * @type {Object} + * Load configuration from .rc files and env vars + * @param {string} namespace Namespace to look for env vars under (defaults to `sails`) + * @return {dictionary} A dictionary of config values gathered from .rc files, with env vars and command line options merged on top */ -var conf = rc('sails'); - -// Load in overrides from the environment, using `rttc.parseHuman` to -// guesstimate the types. -// NOTE -- the code below is lifted from the `rc` module, and modified to: -// 1. Pass JSHint -// 2. Run parseHuman on values -// If at some point `rc` exposes metadata about which configs came from -// the environment, we can simplify our code by just running `parseHuman` -// on those values instead of doing the work to pluck them from the env. - -// We're only interested in env vars prefixed with `sails_`. -var prefix = 'sails_'; -// Cache the prefix length so we don't have to keep looking it up. -var l = prefix.length; - -// Loop through the env vars, looking for ones with the right prefix. -_.each(process.env, function(val, key) { - - // If this var's name has the right prefix... - if((key.indexOf(prefix)) === 0) { - - // Replace double-underscores with dots, to work with Lodash _.set(). - var keypath = key.substring(l).replace(/__/g,'.'); - - // Attempt to parse the value as JSON. - try { - val = rttc.parseHuman(val, 'json'); - } - // If that doesn't work, humanize the value without providing a schema. - catch(e) { - val = rttc.parseHuman(val); - } +module.exports = function(namespace) { + + // Default namespace to `sails`. + namespace = namespace || 'sails'; + + // Locate and load .rc files if they exist. + var conf = rc(namespace); + + // Load in overrides from the environment, using `rttc.parseHuman` to + // guesstimate the types. + // NOTE -- the code below is lifted from the `rc` module, and modified to: + // 1. Pass JSHint + // 2. Run parseHuman on values + // If at some point `rc` exposes metadata about which configs came from + // the environment, we can simplify our code by just running `parseHuman` + // on those values instead of doing the work to pluck them from the env. + + // Construct the expected env var prefix from the namespace. + var prefix = namespace + '_'; + + // Cache the prefix length so we don't have to keep looking it up. + var l = prefix.length; - // Override the current value at this keypath in `conf` (which currently contains - // the string value of the env var) with the now (possibly) humanized value. - _.set(conf, keypath, val); + // Loop through the env vars, looking for ones with the right prefix. + _.each(process.env, function(val, key) { + + // If this var's name has the right prefix... + if((key.indexOf(prefix)) === 0) { + + // Replace double-underscores with dots, to work with Lodash _.set(). + var keypath = key.substring(l).replace(/__/g,'.'); + + // Attempt to parse the value as JSON. + try { + val = rttc.parseHuman(val, 'json'); + } + // If that doesn't work, humanize the value without providing a schema. + catch(e) { + val = rttc.parseHuman(val); + } + + // Override the current value at this keypath in `conf` (which currently contains + // the string value of the env var) with the now (possibly) humanized value. + _.set(conf, keypath, val); + + } - } + }); -}); + // Load command line arguments, since they need to take precedence over env. + var argv = minimist(process.argv.slice(2)); -// Load command line arguments, since they need to take precedence over env. -var argv = minimist(process.argv.slice(2)); + // Merge the command line arguments back on top. + conf = _.merge(conf, argv); -// Merge the command line arguments back on top. -conf = _.merge(conf, argv); + return conf; -// Expose the final conf object to the world. -module.exports = conf; +}; diff --git a/test/helpers/test-spawning-sails-lift-child-process-in-cwd.js b/test/helpers/test-spawning-sails-lift-child-process-in-cwd.js index 33a3aa1c7..1a177d814 100644 --- a/test/helpers/test-spawning-sails-lift-child-process-in-cwd.js +++ b/test/helpers/test-spawning-sails-lift-child-process-in-cwd.js @@ -39,8 +39,8 @@ var MProcess = require('machinepack-process'); */ module.exports = function testSpawningSailsLiftChildProcessInCwd (opts){ - if (!_.isArray(opts.liftCliArgs)){ - throw new Error('Consistency violation: Missing or invalid option (`liftCliArgs` should be an array) in `testSpawningSailsLiftChildProcessInCwd()`. I may just be a test helper, but I\'m serious about assertions!!!'); + if (!_.isArray(opts.liftCliArgs) && !_.isArray(opts.cliArgs)){ + throw new Error('Consistency violation: Missing or invalid option (either `cliArgs` or `liftCliArgs` should be an array) in `testSpawningSailsLiftChildProcessInCwd()`. I may just be a test helper, but I\'m serious about assertions!!!'); } if (!_.isString(opts.pathToSailsCLI)){ throw new Error('Consistency violation: Missing or invalid option (`pathToSailsCLI` should be a string) in `testSpawningSailsLiftChildProcessInCwd()`. I may just be a test helper, but I\'m serious about assertions!!!'); @@ -76,11 +76,14 @@ module.exports = function testSpawningSailsLiftChildProcessInCwd (opts){ // This variable will hold the reference to the child process. var sailsLiftProc; + var cliArgs = opts.cliArgs ? opts.cliArgs : [opts.pathToSailsCLI, 'lift'].concat(opts.liftCliArgs); + // Spawn the child process before(function(done) { sailsLiftProc = MProcess.spawnChildProcess({ command: 'node', - cliArgs: [opts.pathToSailsCLI, 'lift'].concat(opts.liftCliArgs) + cliArgs: cliArgs, + environmentVars: opts.envVars }).execSync(); // For debugging, as needed: diff --git a/test/integration/lift.test.js b/test/integration/lift.test.js index 50f4af0a5..f26dfb166 100644 --- a/test/integration/lift.test.js +++ b/test/integration/lift.test.js @@ -6,7 +6,10 @@ var path = require('path'); var util = require('util'); var tmp = require('tmp'); var request = require('request'); +var assert = require('assert'); +var _ = require('@sailshq/lodash'); var MProcess = require('machinepack-process'); +var Filesystem = require('machinepack-fs'); var testSpawningSailsLiftChildProcessInCwd = require('../helpers/test-spawning-sails-lift-child-process-in-cwd'); var appHelper = require('./helpers/appHelper'); @@ -46,6 +49,12 @@ describe('Starting sails server with `sails lift`', function() { // And CD in. before(function (){ process.chdir(pathToTestApp); + Filesystem.writeSync({ + force: true, + destination: 'api/controllers/getconf.js', + string: 'module.exports = function (req, res) { return res.json(sails.config); }' + }).execSync(); + }); // Test `sails lift` in the CWD. @@ -53,13 +62,52 @@ describe('Starting sails server with `sails lift`', function() { testSpawningSailsLiftChildProcessInCwd({ pathToSailsCLI: pathToSailsCLI, liftCliArgs: ['--hooks.pubsub=false'], + envVars: _.extend({ 'sails_foo__bar': '{"abc": 123}'}, process.env), httpRequestInstructions: { method: 'GET', uri: 'http://localhost:1337', + }, + fnWithAdditionalTests: function (){ + it('should humanize the config passed in via env vars', function (done){ + request({ + method: 'GET', + uri: 'http://localhost:1337/getconf', + }, function(err, response, body) { + if (err) { return done(err); } + body = JSON.parse(body); + assert.equal(body.foo && body.foo.bar && body.foo.bar.abc, 123); + return done(); + }); + }); } }); }); + // Test `sails lift` in the CWD with env vars for config. + describe('running `node app.js`', function (){ + + testSpawningSailsLiftChildProcessInCwd({ + pathToSailsCLI: pathToSailsCLI, + cliArgs: ['app.js', '--hooks.pubsub=false'], + envVars: _.extend({ 'sails_foo__bar': '{"abc": 123}'}, process.env), + fnWithAdditionalTests: function (){ + it('should humanize the config passed in via env vars', function (done){ + request({ + method: 'GET', + uri: 'http://localhost:1337/getconf', + }, function(err, response, body) { + if (err) { return done(err); } + body = JSON.parse(body); + assert.equal(body.foo && body.foo.bar && body.foo.bar.abc, 123); + return done(); + }); + }); + } + }); + + }); + + // Test `sails lift --port=1492` in the CWD. describe('running `sails lift --port=1492`', function (){ testSpawningSailsLiftChildProcessInCwd({