feat(log): Log improvements for ghost update

refs TryGhost#562 - be able to pass a command suggestion - added basic toString fn to error module
kirrg001 · Feb 2, 2018 · 59f5d03 · 59f5d03
1 parent 509aa5a
commit 59f5d03
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 9 deletions.
diff --git a/extensions/nginx/test/acme-spec.js b/extensions/nginx/test/acme-spec.js
@@ -138,7 +138,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
         });
 
         it('Rejects when acme.sh fails', function () {
-            const gotStub = sinon.stub().rejects({stderr: 'CODE: ENOTFOUND'});
+            const gotStub = sinon.stub().rejects({stderr: 'CODE: ENOTFOUND', cmd: 'acme'});
             const emptyStub = sinon.stub();
             const existsStub = sinon.stub().returns(false);
             const downloadStub = sinon.stub().resolves();
@@ -155,7 +155,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
             return acme.install({sudo: sudoStub, logVerbose: logStub}).then(() => {
                 expect(false, 'Promise should have been rejected').to.be.true;
             }).catch((reject) => {
-                expect(reject.message).to.equal('An error occurred.');
+                expect(reject.message).to.equal('Error occurred running command: \'acme\'');
                 expect(reject.options.stderr).to.equal('CODE: ENOTFOUND');
                 expect(logStub.calledTwice).to.be.true;
                 expect(sudoStub.calledOnce).to.be.true;

diff --git a/lib/errors.js b/lib/errors.js
@@ -24,7 +24,6 @@ class CliError extends Error {
         // see https://nodejs.org/api/errors.html#errors_error_capturestacktrace_targetobject_constructoropt
         Error.captureStackTrace(this, this.constructor);
 
-        this.context = options.context || {};
         this.options = options;
 
         this.help = 'Please refer to https://docs.ghost.org/v1/docs/troubleshooting#section-cli-errors for troubleshooting.'
@@ -66,13 +65,31 @@ class CliError extends Error {
         return (this.options.log === false) ? false : true;
     }
 
+    /**
+     * Basics to log: message, help and suggestion
+     * @returns {string}
+     */
+    toStringBasics() {
+        let output = `${chalk.yellow('Message:')} ${this.message}\n`;
+
+        if (this.options.help) {
+            output += `${chalk.gray('Help:')} ${this.options.help}\n`;
+        }
+
+        if (this.options.suggestion) {
+            output += `${chalk.green('Suggestion:')} ${this.options.suggestion}\n`;
+        }
+
+        return output;
+    }
+
     /**
      * Error message string
      * @param {boolean} verbose - Whether or not to render verbose errors
      * @return {string}
      */
     toString(verbose) {
-        let output = `${chalk.yellow('Message:')} ${this.message}\n`;
+        let output = this.toStringBasics();
 
         if (verbose) {
             output += `${chalk.yellow('Stack:')} ${this.stack}\n\n`;
@@ -86,10 +103,6 @@ class CliError extends Error {
             }
         }
 
-        if (this.options.help) {
-            output += `${chalk.gray('Help:')} ${this.options.help}\n`;
-        }
-
         return output;
     }
 }
@@ -101,8 +114,15 @@ class CliError extends Error {
  * @extends CliError
  */
 class ProcessError extends CliError {
+    constructor(options) {
+        options = options || {};
+
+        options.message = options.message || `Error occurred running command: '${options.cmd}'`;
+        super(options);
+    }
+
     toString(verbose) {
-        let output = chalk.red(`Error occurred running command: '${this.options.cmd}'\n\n`);
+        let output = this.toStringBasics();
 
         if (this.options.killed) {
             // Process was killed

diff --git a/lib/tasks/migrate.js b/lib/tasks/migrate.js
@@ -61,6 +61,15 @@ module.exports = function runMigrations(context) {
                     'Please either uninstall and reinstall Ghost-CLI, or switch to MySQL',
                 help: 'https://docs.ghost.org/v1/docs/troubleshooting#section-sqlite3-install-failure'
             });
+        } else {
+            // only show suggestion on `ghost update`
+            error = new errors.ProcessError({
+                message: 'The database migration in Ghost encountered an error.',
+                stderr: error.stderr,
+                environment: context.instance.system.environment,
+                help: 'https://docs.ghost.org/v1/docs/troubleshooting#section-general-update-error',
+                suggestion: process.argv.slice(2, 3).join(' ') === 'update' ? 'ghost update --rollback' : null
+            });
         }
 
         config.set('logging.transports', transports).save();

diff --git a/test/unit/tasks/migrate-spec.js b/test/unit/tasks/migrate-spec.js
@@ -135,4 +135,58 @@ describe('Unit: Tasks > Migrate', function () {
             expect(config.set.args[1]).to.deep.equal(['logging.transports', ['stdout', 'file']]);
         });
     });
+
+    it('error on `ghost update`', function () {
+        const originalArgv = process.argv;
+
+        process.argv = ['node', 'ghost', 'update'];
+
+        const config = getConfigStub();
+        config.get.withArgs('logging.transports', null).returns(['stdout', 'file']);
+        const execaStub = sinon.stub().rejects({stderr: 'YA_GOOFED'});
+        const useGhostUserStub = sinon.stub().returns(false);
+
+        const migrate = proxyquire(migratePath, {
+            execa: execaStub,
+            '../utils/use-ghost-user': useGhostUserStub
+        });
+
+        return migrate({instance: {config: config, dir: '/some-dir', system: {environment: 'testing'}}}).then(() => {
+            expect(false, 'error should have been thrown').to.be.true;
+            process.argv = originalArgv;
+        }).catch((error) => {
+            expect(error).to.be.an.instanceof(errors.ProcessError);
+            expect(error.options.stderr).to.match(/YA_GOOFED/);
+            expect(error.options.suggestion).to.eql('ghost update --rollback');
+            expect(error.options.help).to.exist;
+            process.argv = originalArgv;
+        });
+    });
+
+    it('error on `ghost setup migrate`', function () {
+        const originalArgv = process.argv;
+
+        process.argv = ['node', 'ghost', 'setup', 'migrate'];
+
+        const config = getConfigStub();
+        config.get.withArgs('logging.transports', null).returns(['stdout', 'file']);
+        const execaStub = sinon.stub().rejects({stderr: 'YA_GOOFED'});
+        const useGhostUserStub = sinon.stub().returns(false);
+
+        const migrate = proxyquire(migratePath, {
+            execa: execaStub,
+            '../utils/use-ghost-user': useGhostUserStub
+        });
+
+        return migrate({instance: {config: config, dir: '/some-dir', system: {environment: 'testing'}}}).then(() => {
+            expect(false, 'error should have been thrown').to.be.true;
+            process.argv = originalArgv;
+        }).catch((error) => {
+            expect(error).to.be.an.instanceof(errors.ProcessError);
+            expect(error.options.stderr).to.match(/YA_GOOFED/);
+            expect(error.options.suggestion).to.not.exist;
+            expect(error.options.help).to.exist;
+            process.argv = originalArgv;
+        });
+    });
 });