From 85c0913d701407ea00e20097cc9d082357005010 Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Thu, 2 Feb 2017 13:46:30 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=8E=A8=20=20config=20optimisations=20(#79?= =?UTF-8?q?21)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 84 +++++++----- core/server/config/utils.js | 3 +- core/test/unit/config/index_spec.js | 125 +++++++++++++----- .../fixtures/config/config.testing-mysql.json | 14 ++ .../utils/fixtures/config/config.testing.json | 14 ++ core/test/utils/fixtures/config/defaults.json | 9 ++ .../config/env/config.testing-mysql.json | 7 + .../fixtures/config/env/config.testing.json | 7 + .../test/utils/fixtures/config/overrides.json | 6 + 9 files changed, 196 insertions(+), 73 deletions(-) create mode 100644 core/test/utils/fixtures/config/config.testing-mysql.json create mode 100644 core/test/utils/fixtures/config/config.testing.json create mode 100644 core/test/utils/fixtures/config/defaults.json create mode 100644 core/test/utils/fixtures/config/env/config.testing-mysql.json create mode 100644 core/test/utils/fixtures/config/env/config.testing.json create mode 100644 core/test/utils/fixtures/config/overrides.json diff --git a/core/server/config/index.js b/core/server/config/index.js index 7b1195e4da..34c94e4c36 100644 --- a/core/server/config/index.js +++ b/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; diff --git a/core/server/config/utils.js b/core/server/config/utils.js index ffca641877..a60cd8263f 100644 --- a/core/server/config/utils.js +++ b/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 }); } diff --git a/core/test/unit/config/index_spec.js b/core/test/unit/config/index_spec.js index ab404fe251..e5a6d26834 100644 --- a/core/test/unit/config/index_spec.js +++ b/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' }); diff --git a/core/test/utils/fixtures/config/config.testing-mysql.json b/core/test/utils/fixtures/config/config.testing-mysql.json new file mode 100644 index 0000000000..de47f57b61 --- /dev/null +++ b/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" + } +} diff --git a/core/test/utils/fixtures/config/config.testing.json b/core/test/utils/fixtures/config/config.testing.json new file mode 100644 index 0000000000..de47f57b61 --- /dev/null +++ b/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" + } +} diff --git a/core/test/utils/fixtures/config/defaults.json b/core/test/utils/fixtures/config/defaults.json new file mode 100644 index 0000000000..d7c058838d --- /dev/null +++ b/core/test/utils/fixtures/config/defaults.json @@ -0,0 +1,9 @@ +{ + "database": { + "client": "sqlite3", + "connection": { + "filename": "/test.db" + }, + "debug": false + } +} diff --git a/core/test/utils/fixtures/config/env/config.testing-mysql.json b/core/test/utils/fixtures/config/env/config.testing-mysql.json new file mode 100644 index 0000000000..449e1028f2 --- /dev/null +++ b/core/test/utils/fixtures/config/env/config.testing-mysql.json @@ -0,0 +1,7 @@ +{ + "url": "http://localhost:2368", + "logging": { + "level": "info", + "transports": ["stdout"] + } +} diff --git a/core/test/utils/fixtures/config/env/config.testing.json b/core/test/utils/fixtures/config/env/config.testing.json new file mode 100644 index 0000000000..449e1028f2 --- /dev/null +++ b/core/test/utils/fixtures/config/env/config.testing.json @@ -0,0 +1,7 @@ +{ + "url": "http://localhost:2368", + "logging": { + "level": "info", + "transports": ["stdout"] + } +} diff --git a/core/test/utils/fixtures/config/overrides.json b/core/test/utils/fixtures/config/overrides.json new file mode 100644 index 0000000000..a33c045291 --- /dev/null +++ b/core/test/utils/fixtures/config/overrides.json @@ -0,0 +1,6 @@ +{ + "paths": { + "appRoot": ".", + "corePath": "core/" + } +}