From e53ce6a6af591e0da1e6d417cf153e77960ef32b Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 28 May 2018 09:16:16 -0400 Subject: [PATCH] fix(migrate): move setting of content-path to setup closes #708 - remove any need to save the config out of the migrations step - instead, we set the content path during ghost setup --- lib/commands/setup.js | 5 +++++ lib/tasks/migrate.js | 14 +++++--------- test/unit/commands/setup-spec.js | 25 ++++++++++++++++++++++--- test/unit/tasks/migrate-spec.js | 31 ++++++++----------------------- 4 files changed, 40 insertions(+), 35 deletions(-) diff --git a/lib/commands/setup.js b/lib/commands/setup.js index d7e64efae..5e7631edd 100644 --- a/lib/commands/setup.js +++ b/lib/commands/setup.js @@ -107,6 +107,11 @@ class SetupCommand extends Command { ctx.instance = this.system.getInstance(); ctx.instance.name = (argv.pname || url.parse(ctx.instance.config.get('url')).hostname).replace(/\./g, '-'); this.system.addInstance(ctx.instance); + + // Ensure we set the content path when we set up the instance + if (!ctx.instance.config.has('paths.contentPath')) { + ctx.instance.config.set('paths.contentPath', path.join(ctx.instance.dir, 'content')).save(); + } } }]; diff --git a/lib/tasks/migrate.js b/lib/tasks/migrate.js index 8c17ebff0..66013df71 100644 --- a/lib/tasks/migrate.js +++ b/lib/tasks/migrate.js @@ -6,14 +6,10 @@ const errors = require('../errors'); const ghostUser = require('../utils/use-ghost-user'); module.exports = function runMigrations(context) { - const config = context.instance.config; + const {config, dir} = context.instance; - if (!config.has('paths.contentPath')) { - config.set('paths.contentPath', path.join(context.instance.dir, 'content')).save(); - } - - const contentDir = path.join(context.instance.dir, 'content'); - const currentDir = path.join(context.instance.dir, 'current'); + const contentDir = path.join(dir, 'content'); + const currentDir = path.join(dir, 'current'); let knexMigratorPromise; const args = ['--init', '--mgpath', currentDir]; @@ -21,12 +17,12 @@ module.exports = function runMigrations(context) { // If we're using sqlite and the ghost user owns the content folder, then // we should run sudo, otherwise run normally if (ghostUser.shouldUseGhostUser(contentDir)) { - const knexMigratorPath = path.resolve(context.instance.dir, 'current/node_modules/.bin/knex-migrator-migrate'); + const knexMigratorPath = path.resolve(dir, 'current/node_modules/.bin/knex-migrator-migrate'); knexMigratorPromise = context.ui.sudo(`${knexMigratorPath} ${args.join(' ')}`, {sudoArgs: '-E -u ghost'}); } else { knexMigratorPromise = execa('knex-migrator-migrate', args, { preferLocal: true, - localDir: path.join(context.instance.dir, 'current') + localDir: path.join(dir, 'current') }); } diff --git a/test/unit/commands/setup-spec.js b/test/unit/commands/setup-spec.js index a3eeb7e6f..a294527af 100644 --- a/test/unit/commands/setup-spec.js +++ b/test/unit/commands/setup-spec.js @@ -1,9 +1,10 @@ 'use strict'; -const expect = require('chai').expect; +const {expect} = require('chai'); const sinon = require('sinon'); const proxyquire = require('proxyquire').noCallThru(); const Promise = require('bluebird'); const path = require('path'); +const configStub = require('../../utils/config-stub'); const modulePath = '../../../lib/commands/setup'; const SetupCommand = proxyquire(modulePath, {os: {platform: () => 'linux'}}); @@ -223,12 +224,17 @@ describe('Unit: Commands > Setup', function () { it('Initial stage is setup properly', function () { const listr = sinon.stub().resolves(); const aIstub = sinon.stub(); + const config = configStub(); + config.get.withArgs('url').returns('https://ghost.org'); + config.has.returns(false); + const system = { getInstance: () => { return { checkEnvironment: () => true, apples: true, - config: {get: () => 'https://ghost.org'} + config, + dir: '/var/www/ghost' }; }, addInstance: aIstub, @@ -262,18 +268,28 @@ describe('Unit: Commands > Setup', function () { expect(ctx.instance.name).to.equal('ghost-org'); expect(aIstub.calledOnce).to.be.true; expect(aIstub.args[0][0]).to.deep.equal(ctx.instance); + + expect(config.has.calledOnce).to.be.true; + expect(config.set.calledOnce).to.be.true; + expect(config.set.calledWithExactly('paths.contentPath', '/var/www/ghost/content')).to.be.true; + expect(config.save.calledOnce).to.be.true; }); }); it('Initial stage is setup properly, sanitizes pname argument', function () { const listr = sinon.stub().resolves(); const aIstub = sinon.stub(); + const config = configStub(); + config.get.withArgs('url').returns('https://ghost.org'); + config.has.returns(true); + const system = { getInstance: () => { return { checkEnvironment: () => true, apples: true, - config: {get: () => 'https://ghost.org'} + config, + dir: '/var/www/ghost' }; }, addInstance: aIstub, @@ -308,6 +324,9 @@ describe('Unit: Commands > Setup', function () { expect(ctx.instance.name).to.equal('example-com'); expect(aIstub.calledOnce).to.be.true; expect(aIstub.args[0][0]).to.deep.equal(ctx.instance); + expect(config.has.calledOnce).to.be.true; + expect(config.set.called).to.be.false; + expect(config.save.called).to.be.false; }); }); diff --git a/test/unit/tasks/migrate-spec.js b/test/unit/tasks/migrate-spec.js index 3a99a5cf3..6f097e27e 100644 --- a/test/unit/tasks/migrate-spec.js +++ b/test/unit/tasks/migrate-spec.js @@ -2,26 +2,15 @@ const {expect} = require('chai'); const sinon = require('sinon'); const proxyquire = require('proxyquire'); +const configStub = require('../../utils/config-stub'); const errors = require('../../../lib/errors'); const migratePath = '../../../lib/tasks/migrate'; -function getConfigStub(noContentPath) { - const config = { - get: sinon.stub(), - set: sinon.stub().returnsThis(), - has: sinon.stub(), - save: sinon.stub().returnsThis() - }; - - config.has.withArgs('paths.contentPath').returns(!noContentPath); - return config; -} - describe('Unit: Tasks > Migrate', function () { it('runs direct command if useGhostUser returns false', function () { - const config = getConfigStub(true); + const config = configStub(); const execaStub = sinon.stub().resolves(); const useGhostUserStub = sinon.stub().returns(false); @@ -37,15 +26,11 @@ describe('Unit: Tasks > Migrate', function () { expect(useGhostUserStub.args[0][0]).to.equal('/some-dir/content'); expect(execaStub.calledOnce).to.be.true; expect(sudoStub.called).to.be.false; - expect(config.has.calledOnce).to.be.true; - expect(config.set.calledOnce).to.be.true; - expect(config.set.args[0]).to.deep.equal(['paths.contentPath', '/some-dir/content']); - expect(config.save.called).to.be.true; }); }); it('runs sudo command if useGhostUser returns true', function () { - const config = getConfigStub(); + const config = configStub(); const execaStub = sinon.stub().resolves(); const useGhostUserStub = sinon.stub().returns(true); @@ -65,7 +50,7 @@ describe('Unit: Tasks > Migrate', function () { }); it('throws config error with db host if database not found', function () { - const config = getConfigStub(); + const config = configStub(); const execaStub = sinon.stub().returns(Promise.reject({stderr: 'CODE: ENOTFOUND'})); const useGhostUserStub = sinon.stub().returns(false); @@ -83,7 +68,7 @@ describe('Unit: Tasks > Migrate', function () { }); it('throws config error with db user if access denied error', function () { - const config = getConfigStub(); + const config = configStub(); const execaStub = sinon.stub().returns(Promise.reject({stderr: 'CODE: ER_ACCESS_DENIED_ERROR'})); const useGhostUserStub = sinon.stub().returns(false); @@ -101,7 +86,7 @@ describe('Unit: Tasks > Migrate', function () { }); it('throws system error if sqlite3 error is thrown by knex', function () { - const config = getConfigStub(); + const config = configStub(); const execaStub = sinon.stub().returns(Promise.reject({stdout: 'Knex: run\n$ npm install sqlite3 --save\nError:'})); const useGhostUserStub = sinon.stub().returns(false); @@ -123,7 +108,7 @@ describe('Unit: Tasks > Migrate', function () { process.argv = ['node', 'ghost', 'update']; - const config = getConfigStub(); + const config = configStub(); const execaStub = sinon.stub().rejects({stderr: 'YA_GOOFED'}); const useGhostUserStub = sinon.stub().returns(false); @@ -149,7 +134,7 @@ describe('Unit: Tasks > Migrate', function () { process.argv = ['node', 'ghost', 'setup', 'migrate']; - const config = getConfigStub(); + const config = configStub(); const execaStub = sinon.stub().rejects({stderr: 'YA_GOOFED'}); const useGhostUserStub = sinon.stub().returns(false);