🎨 config optimisations (#7921)

refs #7488

- rename file keys for config files, see https://github.com/TryGhost/Ghost/pull/7493/files
- add tests to avoid running into config hierarchy problems again
- overrides.json is the strongest!
- argv/env can override any default
- custom config can override defaults
- reorganise util functions for config again

core/server/config/index.js

@@ -1,45 +1,57 @@
 var Nconf = require('nconf'),
-    nconf = new Nconf.Provider(),
     path = require('path'),
+    debug = require('debug')('ghost:config'),
     localUtils = require('./utils'),
-    env = process.env.NODE_ENV || 'development';
+    env = process.env.NODE_ENV || 'development',
+    _private = {};
 
-/**
- * command line arguments
- */
-nconf.argv();
+_private.loadNconf = function loadNconf(options) {
+    options = options || {};
 
-/**
- * env arguments
- */
-nconf.env({
-    separator: '__'
-});
+    var baseConfigPath = options.baseConfigPath || __dirname,
+        customConfigPath = options.customConfigPath || process.cwd(),
+        nconf = new Nconf.Provider();
 
-/**
- * load config files
- * @TODO:
- * - why does this work? i have no idea!
- * - find out why argv override works, when defining these weird keys
- * - i could not find any nconf usages so that all config requirements work
- */
-nconf.file('ghost1', __dirname + '/overrides.json');
-nconf.file('ghost2', path.join(process.cwd(), 'config.' + env + '.json'));
-nconf.file('ghost3', __dirname + '/env/config.' + env + '.json');
-nconf.file('ghost4', __dirname + '/defaults.json');
+    /**
+     * no channel can override the overrides
+     */
+    nconf.file('overrides', path.join(baseConfigPath, 'overrides.json'));
 
-/**
- * transform all relative paths to absolute paths
- * transform sqlite filename path for Ghost-CLI
- */
-localUtils.makePathsAbsolute.bind(nconf)(nconf.get('paths'), 'paths');
-localUtils.makePathsAbsolute.bind(nconf)(nconf.get('database:connection'), 'database:connection');
+    /**
+     * command line arguments
+     */
+    nconf.argv();
 
-/**
- * values we have to set manual
- */
-nconf.set('env', env);
+    /**
+     * env arguments
+     */
+    nconf.env({
+        separator: '__'
+    });
 
-module.exports = nconf;
-module.exports.isPrivacyDisabled = localUtils.isPrivacyDisabled.bind(nconf);
-module.exports.getContentPath = localUtils.getContentPath.bind(nconf);
+    nconf.file('custom-env', path.join(customConfigPath, 'config.' + env + '.json'));
+    nconf.file('default-env', path.join(baseConfigPath, 'env', 'config.' + env + '.json'));
+    nconf.file('defaults', path.join(baseConfigPath, 'defaults.json'));
+
+    /**
+     * transform all relative paths to absolute paths
+     * transform sqlite filename path for Ghost-CLI
+     */
+    nconf.makePathsAbsolute = localUtils.makePathsAbsolute.bind(nconf);
+    nconf.isPrivacyDisabled = localUtils.isPrivacyDisabled.bind(nconf);
+    nconf.getContentPath = localUtils.getContentPath.bind(nconf);
+
+    nconf.makePathsAbsolute(nconf.get('paths'), 'paths');
+    nconf.makePathsAbsolute(nconf.get('database:connection'), 'database:connection');
+
+    /**
+     * values we have to set manual
+     */
+    nconf.set('env', env);
+
+    debug(nconf.get());
+    return nconf;
+};
+
+module.exports = _private.loadNconf();
+module.exports.loadNconf = _private.loadNconf;

core/server/config/utils.js

@@ -28,7 +28,8 @@ exports.makePathsAbsolute = function makePathsAbsolute(obj, parent) {
 
     if (!obj) {
         throw new errors.IncorrectUsageError({
-            message: 'makePathsAbsolute: Object is missing.'
+            message: 'makePathsAbsolute: Can\'t make paths absolute of non existing object.',
+            help: parent
         });
     }
 

core/test/unit/config/index_spec.js

@@ -1,25 +1,17 @@
-var should         = require('should'),
-    sinon          = require('sinon'),
-    Promise        = require('bluebird'),
-    moment         = require('moment'),
-    path           = require('path'),
-    fs             = require('fs'),
-    _              = require('lodash'),
+// jscs:disable requireDotNotation
 
-    testUtils      = require('../../utils'),
-    i18n           = require('../../../server/i18n'),
-    utils          = require('../../../server/utils'),
-    /*jshint unused:false*/
-    db             = require('../../../server/data/db/connection'),
+var should = require('should'),
+    path = require('path'),
+    rewire = require('rewire'),
+    _ = require('lodash'),
+    i18n = require('../../../server/i18n'),
+    configUtils = require('../../utils/configUtils');
 
-    // Thing we are testing
-    configUtils    = require('../../utils/configUtils'),
-    config         = configUtils.config;
-
-i18n.init();
+should.equal(true, true);
 
 describe('Config', function () {
     before(function () {
+        i18n.init();
         configUtils.restore();
     });
 
@@ -27,6 +19,71 @@ describe('Config', function () {
         configUtils.restore();
     });
 
+    describe('hierarchy of config channels', function () {
+        var originalEnv, originalArgv, customConfig, config;
+
+        beforeEach(function () {
+            originalEnv = _.clone(process.env);
+            originalArgv = _.clone(process.argv);
+            config = rewire('../../../server/config');
+        });
+
+        afterEach(function () {
+            process.env = originalEnv;
+            process.argv = originalArgv;
+        });
+
+        it('env parameter is stronger than file', function () {
+            process.env['database__client'] = 'test';
+
+            customConfig = config.loadNconf({
+                baseConfigPath: path.join(__dirname, '../../utils/fixtures/config'),
+                customConfigPath: path.join(__dirname, '../../utils/fixtures/config')
+            });
+
+            customConfig.get('database:client').should.eql('test');
+        });
+
+        it('argv is stronger than env parameter', function () {
+            process.env['database__client'] = 'test';
+            process.argv[2] = '--database:client=stronger';
+
+            customConfig = config.loadNconf({
+                baseConfigPath: path.join(__dirname, '../../utils/fixtures/config'),
+                customConfigPath: path.join(__dirname, '../../utils/fixtures/config')
+            });
+
+            customConfig.get('database:client').should.eql('stronger');
+        });
+
+        it('argv or env is NOT stronger than overrides', function () {
+            process.env['paths__corePath'] = 'try-to-override';
+            process.argv[2] = '--paths:corePath=try-to-override';
+
+            customConfig = config.loadNconf({
+                baseConfigPath: path.join(__dirname, '../../utils/fixtures/config'),
+                customConfigPath: path.join(__dirname, '../../utils/fixtures/config')
+            });
+
+            customConfig.get('paths:corePath').should.not.containEql('try-to-override');
+        });
+
+        it('overrides is stronger than every other config file', function () {
+            customConfig = config.loadNconf({
+                baseConfigPath: path.join(__dirname, '../../utils/fixtures/config'),
+                customConfigPath: path.join(__dirname, '../../utils/fixtures/config')
+            });
+
+            customConfig.get('paths:corePath').should.not.containEql('try-to-override');
+            customConfig.get('database:client').should.eql('sqlite3');
+            customConfig.get('database:connection:filename').should.eql('/hehe.db');
+            customConfig.get('database:debug').should.eql(true);
+            customConfig.get('url').should.eql('http://localhost:2368');
+            customConfig.get('logging:level').should.eql('error');
+            customConfig.get('logging:transports').should.eql(['stdout']);
+        });
+    });
+
     describe('Theme', function () {
         beforeEach(function () {
             configUtils.set({
@@ -43,14 +100,14 @@ describe('Config', function () {
         });
 
         it('should have exactly the right keys', function () {
-            var themeConfig = config.get('theme');
+            var themeConfig = configUtils.config.get('theme');
 
             // This will fail if there are any extra keys
             themeConfig.should.have.keys('title', 'description', 'logo', 'cover', 'timezone', 'icon');
         });
 
         it('should have the correct values for each key', function () {
-            var themeConfig = config.get('theme');
+            var themeConfig = configUtils.config.get('theme');
 
             // Check values are as we expect
             themeConfig.should.have.property('title', 'casper');
@@ -64,7 +121,7 @@ describe('Config', function () {
 
     describe('Timezone default', function () {
         it('should use timezone from settings when set', function () {
-            var themeConfig = config.get('theme');
+            var themeConfig = configUtils.config.get('theme');
 
             // Check values are as we expect
             themeConfig.should.have.property('timezone', 'Etc/UTC');
@@ -75,7 +132,7 @@ describe('Config', function () {
                 }
             });
 
-            config.get('theme').should.have.property('timezone', 'Africa/Cairo');
+            configUtils.config.get('theme').should.have.property('timezone', 'Africa/Cairo');
         });
 
         it('should set theme object with timezone by default', function () {
@@ -89,7 +146,7 @@ describe('Config', function () {
 
     describe('Index', function () {
         it('should have exactly the right keys', function () {
-            var pathConfig = config.get('paths');
+            var pathConfig = configUtils.config.get('paths');
 
             // This will fail if there are any extra keys
             pathConfig.should.have.keys(
@@ -107,7 +164,7 @@ describe('Config', function () {
         });
 
         it('should have the correct values for each key', function () {
-            var pathConfig = config.get('paths'),
+            var pathConfig = configUtils.config.get('paths'),
                 appRoot = path.resolve(__dirname, '../../../../');
 
             pathConfig.should.have.property('appRoot', appRoot);
@@ -115,27 +172,25 @@ describe('Config', function () {
         });
 
         it('should allow specific properties to be user defined', function () {
-            var contentPath = path.join(config.get('paths').appRoot, 'otherContent', '/');
+            var contentPath = path.join(configUtils.config.get('paths').appRoot, 'otherContent', '/');
 
             configUtils.set('paths:contentPath', contentPath);
-            config.get('paths').should.have.property('contentPath', contentPath);
-            config.getContentPath('images').should.eql(contentPath + 'images/');
+            configUtils.config.get('paths').should.have.property('contentPath', contentPath);
+            configUtils.config.getContentPath('images').should.eql(contentPath + 'images/');
         });
     });
 
     describe('Storage', function () {
         it('should default to local-file-store', function () {
-            config.get('paths').should.have.property('internalStoragePath', path.join(config.get('paths').corePath, '/server/storage/'));
+            configUtils.config.get('paths').should.have.property('internalStoragePath', path.join(configUtils.config.get('paths').corePath, '/server/storage/'));
 
-            config.get('storage').should.have.property('active', {
+            configUtils.config.get('storage').should.have.property('active', {
                 images: 'local-file-store',
                 themes: 'local-file-store'
             });
         });
 
         it('no effect: setting a custom active storage as string', function () {
-            var storagePath = path.join(config.get('paths').contentPath, 'storage', 's3');
-
             configUtils.set({
                 storage: {
                     active: 's3',
@@ -143,8 +198,8 @@ describe('Config', function () {
                 }
             });
 
-            config.get('storage').should.have.property('active', 's3');
-            config.get('storage').should.have.property('s3', {});
+            configUtils.config.get('storage').should.have.property('active', 's3');
+            configUtils.config.get('storage').should.have.property('s3', {});
         });
 
         it('able to set storage for themes (but not officially supported!)', function () {
@@ -157,15 +212,13 @@ describe('Config', function () {
                 }
             });
 
-            config.get('storage').should.have.property('active', {
+            configUtils.config.get('storage').should.have.property('active', {
                 images: 'local-file-store',
                 themes: 's3'
             });
         });
 
         it('should allow setting a custom active storage as object', function () {
-            var storagePath = path.join(config.get('paths').contentPath, 'storage', 's3');
-
             configUtils.set({
                 storage: {
                     active: {
@@ -175,7 +228,7 @@ describe('Config', function () {
                 }
             });
 
-            config.get('storage').should.have.property('active', {
+            configUtils.config.get('storage').should.have.property('active', {
                 images: 's2',
                 themes: 'local-file-store'
             });

core/test/utils/fixtures/config/config.testing-mysql.json

@@ -0,0 +1,14 @@
+{
+    "paths": {
+        "corePath": "try-to-override"
+    },
+    "database": {
+        "connection": {
+            "filename": "/hehe.db"
+        },
+        "debug": true
+    },
+    "logging": {
+        "level": "error"
+    }
+}

core/test/utils/fixtures/config/config.testing.json

@@ -0,0 +1,14 @@
+{
+    "paths": {
+        "corePath": "try-to-override"
+    },
+    "database": {
+        "connection": {
+            "filename": "/hehe.db"
+        },
+        "debug": true
+    },
+    "logging": {
+        "level": "error"
+    }
+}

core/test/utils/fixtures/config/defaults.json

@@ -0,0 +1,9 @@
+{
+    "database": {
+        "client": "sqlite3",
+        "connection": {
+            "filename": "/test.db"
+        },
+        "debug": false
+    }
+}

core/test/utils/fixtures/config/env/config.testing-mysql.json

@@ -0,0 +1,7 @@
+{
+    "url": "http://localhost:2368",
+    "logging": {
+        "level": "info",
+        "transports": ["stdout"]
+    }
+}

core/test/utils/fixtures/config/env/config.testing.json

@@ -0,0 +1,7 @@
+{
+    "url": "http://localhost:2368",
+    "logging": {
+        "level": "info",
+        "transports": ["stdout"]
+    }
+}

core/test/utils/fixtures/config/overrides.json

@@ -0,0 +1,6 @@
+{
+    "paths": {
+        "appRoot": ".",
+        "corePath": "core/"
+    }
+}