🐷 Improve config error logs and messages

refs TryGhost#216

This PR changes the `Config.exists()` method in a way that rather than simply returning false, we're catching the error and return a `ConfigError` object that includes the received error object from `fs.readJsonSync()` as a message.

There are two places, where `Config.exists()` is being called and was expecting a boolean `false` to determine if the config is valid. But this can mean, that the config either doesn't exist or has a syntax error. It remained a secret to the user.

I changed this to check for an `options` property (I'm not very happy with the wording of this, but the CLI uses its own errors and not Ignition and I didn't want to change this within this PR), which than has the `message` property that includes a better error message which we then can log for the user.

e. g.

```
A ConfigError occured.

Error detected in the production configuration.

Message: The config.production.json file is missing or not valid JSON
Context: Error: /Users/aileennowak/Code/cli-installs/cli_3/config.production.json: ENOENT: no such file or directory, open '/Users/aileennowak/Code/cli-installs/cli_3/config.production.json'

Debug Information:
    Node Version: v6.11.0
    Ghost-CLI Version: 1.0.0-beta.3
    Environment: production
    Command: 'ghost start'
```

Author: aileen

lib/commands/doctor/checks/startup.js

@@ -13,10 +13,16 @@ module.exports = [{
     task: (ctx) => {
         let config = Config.exists(path.join(process.cwd(), `config.${ctx.environment}.json`));
 
-        if (config === false) {
+        /**
+        * CASE: `Config.exists()` returns an `options.message` property with a detailed error message
+        * in case an error (e. g. file doesn't exist or has syntaxerrors) occurred.
+        * If the `options` property doesn't exist, the config file was found and valid.
+        */
+        if (config.options && config.options.message) {
             return Promise.reject(new errors.ConfigError({
                 environment: ctx.environment,
-                message: 'Config file is not valid JSON'
+                message: `The config.${ctx.environment}.json file is missing or not valid JSON`,
+                context: config.options.message
             }));
         }
 

lib/errors.js

@@ -138,6 +138,10 @@ class ConfigError extends CliError {
                 chalk.blue(`Run \`${chalk.underline(`ghost config ${this.options.configKey} <new value>`)}\` to fix it.\n`);
         }
 
+        if (this.options.context) {
+            initial += `${chalk.gray('Context:')} ${this.options.context}\n`
+        }
+
         return initial;
     }
 }

lib/instance.js

@@ -131,12 +131,22 @@ class Instance {
      * @public
      */
     checkEnvironment() {
-        if (
-            this.system.production &&
-            Config.exists(path.join(this.dir, 'config.development.json')) &&
-            !Config.exists(path.join(this.dir, 'config.production.json'))
-        ) {
-            this.ui.log('Found a development config but not a production config, running in development mode instead', 'yellow');
+        let prodConfig = Config.exists(path.join(this.dir, 'config.production.json'));
+        let devConfig = Config.exists(path.join(this.dir, 'config.development.json'));
+
+        /**
+        * CASE: Attempt to start in production mode, but no production config is found.
+        * `Config.exists()` returns an `options.message` property with a detailed error message
+        * in case an error (e. g. file doesn't exist or has syntaxerrors) occurred.
+        * If the `options` property doesn't exist, the config file was found and valid.
+        */
+        if (this.system.production && prodConfig.options && !devConfig.options) {
+            this.ui.log('The production config is either invalid or missing, running in development mode instead', 'yellow');
+
+            if (prodConfig.options.message) {
+                this.ui.log('Message: ' + prodConfig.options.message, 'grey');
+            }
+
             this.system.setEnvironment(true, true);
         }
     }

lib/utils/config.js

@@ -5,6 +5,7 @@ const _get          = require('lodash/get');
 const _set          = require('lodash/set');
 const _has          = require('lodash/has');
 const fs            = require('fs-extra');
+const errors        = require('../errors');
 
 /**
  * Config class. Basic wrapper around a json file, but handles
@@ -19,12 +20,16 @@ class Config {
      * @param {string} filename Filename to load
      */
     constructor(filename) {
+        let config;
+
         if (!filename) {
             throw new Error('Config file not specified.');
         }
 
         this.file = filename;
-        this.values = Config.exists(this.file) || {};
+
+        config = Config.exists(this.file);
+        this.values = (config && !config.options) ? config : {};
     }
 
     /**
@@ -100,13 +105,17 @@ class Config {
      * @static
      * @method exists
      * @public
+     * @return {object} that either contains the property `options.message` in
+     * case of an error, or the parsed config values
      */
     static exists(filename) {
         try {
             var result = fs.readJsonSync(filename);
             return result;
         } catch (e) {
-            return false;
+            return new errors.ConfigError({
+                message: e
+            });
         }
     }
 };

test/unit/instance-spec.js

@@ -253,7 +253,14 @@ describe('Unit: Instance', function () {
         it('doesn\'t do anything if config.development.json doesn\'t exist', function () {
             let logStub = sandbox.stub();
             let environmentStub = sandbox.stub();
-            let existsStub = sandbox.stub(Config, 'exists').withArgs('/path/config.development.json').returns(false);
+            let existsStub = sandbox.stub(Config, 'exists');
+            existsStub.withArgs('/path/config.development.json').returns(
+                {
+                    options: {
+                        message: 'file does not exist'
+                    }
+                });
+            existsStub.withArgs('/path/config.production.json').returns(true);
             let testInstance = new Instance({ log: logStub }, {
                 setEnvironment: environmentStub,
                 production: true,
@@ -263,25 +270,30 @@ describe('Unit: Instance', function () {
             testInstance.checkEnvironment();
             expect(logStub.called).to.be.false;
             expect(environmentStub.called).to.be.false;
-            expect(existsStub.calledOnce).to.be.true;
+            expect(existsStub.calledTwice).to.be.true;
         });
 
         it('logs and sets environment if not production, config.dev exists, and config.production doesn\'t exist', function () {
             let logStub = sandbox.stub();
             let environmentStub = sandbox.stub();
             let existsStub = sandbox.stub(Config, 'exists');
             existsStub.withArgs('/path/config.development.json').returns(true);
-            existsStub.withArgs('/path/config.production.json').returns(false);
+            existsStub.withArgs('/path/config.production.json').returns(
+                {
+                    options: {
+                        message: 'file does not exist'
+                    }
+                });
             let testInstance = new Instance({ log: logStub }, {
                 setEnvironment: environmentStub,
                 production: true,
                 environment: 'production'
             }, '/path');
 
             testInstance.checkEnvironment();
-
-            expect(logStub.calledOnce).to.be.true;
-            expect(logStub.args[0][0]).to.match(/Found a development config/);
+            expect(logStub.calledTwice).to.be.true;
+            expect(logStub.args[0][0]).to.match(/The production config is either invalid or missing, running in development mode instead/);
+            expect(logStub.args[1][0]).to.match(/file does not exist/);
             expect(environmentStub.calledOnce).to.be.true;
             expect(environmentStub.args[0]).to.deep.equal([true, true]);
             expect(existsStub.calledTwice).to.be.true;

test/unit/utils/config-spec.js

@@ -1,7 +1,6 @@
 'use strict';
 const expect = require('chai').expect;
 const fs = require('fs-extra');
-const path = require('path');
 
 const Config = require('../../../lib/utils/config');
 
@@ -108,16 +107,18 @@ describe('Unit: Config', function () {
     });
 
     describe('exists()', function () {
-        it('returns false if file does not exist', function () {
+        it('returns error if file does not exist', function () {
             let result = Config.exists('does-not-exist.txt');
 
-            expect(result).to.be.false;
+            expect(result).to.have.property('options');
+            expect(result.options).to.have.property('message');
         });
 
-        it('returns false if file contains invalid JSON', function () {
+        it('returns error if file contains invalid JSON', function () {
             fs.writeFileSync('config-test.json', 'invalid json');
             let result = Config.exists('config-test.json');
-            expect(result).to.be.false;
+            expect(result).to.have.property('options');
+            expect(result.options).to.have.property('message');
             fs.removeSync('config-test.json');
         });