Skip to content

Commit

Permalink
Make our internal rc into a function that accepts a namespace arg (…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
sgress454 committed Dec 8, 2016
1 parent b865f30 commit bd455af
Show file tree
Hide file tree
Showing 12 changed files with 118 additions and 63 deletions.
2 changes: 1 addition & 1 deletion accessible/rc.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Module dependencies
*/

var rc = require('rc');
var rc = require('../lib/app/configuration/rc');


/**
Expand Down
2 changes: 1 addition & 1 deletion bin/sails-console.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion bin/sails-deploy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion bin/sails-generate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
2 changes: 1 addition & 1 deletion bin/sails-lift.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down
2 changes: 1 addition & 1 deletion bin/sails-new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');


Expand Down
2 changes: 1 addition & 1 deletion bin/sails-www.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')();



Expand Down
2 changes: 1 addition & 1 deletion errors/fatal.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);


Expand Down
2 changes: 1 addition & 1 deletion errors/warn.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);


Expand Down
106 changes: 55 additions & 51 deletions lib/app/configuration/rc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
9 changes: 6 additions & 3 deletions test/helpers/test-spawning-sails-lift-child-process-in-cwd.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)){

This comment has been minimized.

Copy link
@mikermcneil

mikermcneil Dec 8, 2016

Member

@sgress454 re: cliArgs - this test util was intended to be for sails lift -- so i'd rather not have it test node app or other stuff. Looks like it's only being used this place at one point in the code though, so easy to change.

Also, if we're adding envVars (which I think is a good idea), then we've just got to add them to the comments above as well.

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!!!');
Expand Down Expand Up @@ -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:
Expand Down
48 changes: 48 additions & 0 deletions test/integration/lift.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -46,20 +49,65 @@ 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.
describe('running `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);

This comment has been minimized.

Copy link
@mikermcneil

mikermcneil Dec 8, 2016

Member

needs status code assertion first, then try/catch around parse and assert

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);

This comment has been minimized.

Copy link
@mikermcneil

mikermcneil Dec 8, 2016

Member

same as above

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({
Expand Down

1 comment on commit bd455af

@mikermcneil
Copy link
Member

Choose a reason for hiding this comment

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

For future reference, the test is failing because:

stdout: info: No local Sails install detected; using globally-installed Sails.
...
...
stderr: debug: -------------------------------------------------------

          ✓ should still be lifted
          ✓ should respond with a 200 status code when a `GET` request is sent to `http://localhost:1337`
          ✓ should humanize the config passed in via env vars
      running `node app.js`
        and then waiting for a bit
stderr: Encountered an error when attempting to require('sails'):

stderr: Error: Cannot find module 'sails'
    at Function.Module._resolveFilename (module.js:326:15)
    at Function.Module._load (module.js:277:25)
    at Module.require (module.js:354:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/private/var/folders/_s/347n05_x2rgb_0w6s6y0ytr00000gn/T/tmp-291179ZUWIae91wSm/testApp/app.js:35:11)
    at Module._compile (module.js:410:26)
    at Object.Module._extensions..js (module.js:417:10)
    at Module.load (module.js:344:32)
    at Function.Module._load (module.js:301:12)
    at Function.Module.runMain (module.js:442:10)

stderr: --

stderr: To run an app using `node app.js`, you usually need to have a version of `sails` installed in the same directory as your app.
To do that, run `npm install sails`

stderr: 
Alternatively, if you have sails installed globally (i.e. you did `npm install -g sails`), you can use `sails lift`.
When you run `sails lift`, your app will still use a local `./node_modules/sails` dependency if it exists,

stderr: but if it doesn't, the app will run with the global sails instead!

          ✓ should still be lifted
          1) should humanize the config passed in via env vars
          2) "after all" hook

Please sign in to comment.