fix(instance): make running a function rather than a get/set

refs #435 - instead of using a getter/setter to determine running, make it a function. This allows any errors that occur during the isRunning check to not be swallowed, plus it makes the code a little cleaner.
TryGhost · Aug 11, 2017 · 0cef275 · 0cef275
1 parent 016ba10
commit 0cef275
Show file tree

Hide file tree

Showing 11 changed files with 123 additions and 120 deletions.
diff --git a/lib/commands/log.js b/lib/commands/log.js
@@ -21,7 +21,7 @@ class LogCommand extends Command {
             return Promise.reject(new errors.SystemError(`Ghost instance '${argv.name}' does not exist`));
         }
 
-        if (instance.running) {
+        if (instance.running()) {
             instance.loadRunningEnvironment();
         } else {
             instance.checkEnvironment();

diff --git a/lib/commands/restart.js b/lib/commands/restart.js
@@ -5,7 +5,7 @@ class RestartCommand extends Command {
     run() {
         let instance = this.system.getInstance();
 
-        if (!instance.running) {
+        if (!instance.running()) {
             return Promise.reject(new Error('Ghost instance is not currently running.'));
         }
 

diff --git a/lib/commands/start.js b/lib/commands/start.js
@@ -25,7 +25,7 @@ class StartCommand extends Command {
         let instance = this.system.getInstance();
         const runOptions = {quiet: argv.quiet};
 
-        if (instance.running) {
+        if (instance.running()) {
             return Promise.reject(new Error('Ghost is already running. Use `ghost ls` to see details.'));
         }
 
@@ -35,7 +35,7 @@ class StartCommand extends Command {
             let processInstance = instance.process;
 
             let start = () => Promise.resolve(processInstance.start(process.cwd(), this.system.environment)).then(() => {
-                instance.running = this.system.environment;
+                instance.running(this.system.environment);
             });
 
             return this.ui.run(start, 'Starting Ghost', runOptions).then(() => {

diff --git a/lib/commands/stop.js b/lib/commands/stop.js
@@ -40,14 +40,14 @@ class StopCommand extends Command {
 
         Command.checkValidInstall('stop');
 
-        if (!instance.running) {
+        if (!instance.running()) {
             return Promise.reject(new errors.SystemError('No running Ghost instance found here.'));
         }
 
         instance.loadRunningEnvironment();
 
         let stop = () => Promise.resolve(instance.process.stop(process.cwd())).then(() => {
-            instance.running = null;
+            instance.running(null);
         });
 
         return this.ui.run(stop, 'Stopping Ghost', runOptions).then(() => {

diff --git a/lib/commands/uninstall.js b/lib/commands/uninstall.js
@@ -31,7 +31,7 @@ class UninstallCommand extends Command {
 
             return this.ui.listr([{
                 title: 'Stopping Ghost',
-                skip: () => !instance.running,
+                skip: () => !instance.running(),
                 task: () => {
                     instance.loadRunningEnvironment(true);
                     // If the instance is currently running we need to make sure

diff --git a/lib/commands/update.js b/lib/commands/update.js
@@ -46,7 +46,7 @@ class UpdateCommand extends Command {
             context.installPath = path.join(process.cwd(), 'versions', context.version);
         }
 
-        if (instance.running) {
+        if (instance.running()) {
             instance.loadRunningEnvironment(true);
         }
 
@@ -71,7 +71,7 @@ class UpdateCommand extends Command {
             }
         }, {
             title: 'Stopping Ghost',
-            skip: (ctx) => !ctx.instance.running,
+            skip: (ctx) => !ctx.instance.running(),
             task: () => {
                 return this.runCommand(StopCommand, {quiet: true}).catch((error) => {
                     if (!(error instanceof errors.SystemError) || !error.message.match(/No running Ghost instance/)) {

diff --git a/lib/instance.js b/lib/instance.js
@@ -43,33 +43,6 @@ class Instance {
         return true;
     }
 
-    /**
-     * Environment this instance is currently running in,
-     * or null if this instance is not running
-     *
-     * @property running
-     * @type string|null
-     * @public
-     */
-    get running() {
-        if (!this.cliConfig.has('running')) {
-            return false;
-        }
-
-        this.loadRunningEnvironment();
-
-        if (!this.process.isRunning(this.dir)) {
-            this.cliConfig.set('running', null).save();
-            return false;
-        }
-
-        return true;
-    }
-    set running(environment) {
-        this.cliConfig.set('running', environment).save();
-        return true;
-    }
-
     /**
      * Environment-specific configuration file (contains Ghost-specific variables)
      *
@@ -120,6 +93,37 @@ class Instance {
         this.dir = dir;
     }
 
+    /**
+     * If no args specified, returns whether or not the instance is running
+     * If arg specified, sets the running environment to the passed environment
+     *
+     * @param {String} environment Environment to set
+     * @return {Boolean} true if instance is running, otherwise false
+     *
+     * @method running
+     * @public
+     */
+    running(environment) {
+        if (environment !== undefined) {
+            // setter
+            this.cliConfig.set('running', environment).save();
+            return true;
+        }
+
+        if (!this.cliConfig.has('running')) {
+            return false;
+        }
+
+        this.loadRunningEnvironment();
+
+        if (!this.process.isRunning(this.dir)) {
+            this.cliConfig.set('running', null).save();
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Checks the environment to see if a dev config exists
      * and not a production one. This is useful because the CLI
@@ -170,7 +174,7 @@ class Instance {
      * @public
      */
     summary() {
-        if (!this.running) {
+        if (!this.running()) {
             return {
                 name: this.name,
                 dir: this.dir.replace(os.homedir(), '~'),

diff --git a/lib/system.js b/lib/system.js
@@ -198,7 +198,7 @@ class System {
             }
 
             return this.getInstance(name);
-        }).filter((instance) => instance && (!running || instance.running));
+        }).filter((instance) => instance && (!running || instance.running()));
 
         if (namesToRemove.length) {
             namesToRemove.forEach((name) => delete instances[name]);

diff --git a/test/unit/commands/restart-spec.js b/test/unit/commands/restart-spec.js
@@ -8,7 +8,7 @@ const Instance = require('../../../lib/instance');
 describe('Unit: Command > Restart', function () {
     it('throws error if instance is not running', function () {
         class TestInstance extends Instance {
-            get running() { return false; }
+            running() { return false; }
         }
         let testInstance = new TestInstance();
 
@@ -26,8 +26,8 @@ describe('Unit: Command > Restart', function () {
     it('calls process restart method if instance is running', function () {
         let restartStub = sinon.stub().resolves();
         class TestInstance extends Instance {
-            get running() { return true; }
             get process() { return { restart: restartStub }; }
+            running() { return true; }
         }
         let testInstance = new TestInstance();
         let loadRunEnvStub = sinon.stub(testInstance, 'loadRunningEnvironment');

diff --git a/test/unit/instance-spec.js b/test/unit/instance-spec.js
@@ -87,73 +87,6 @@ describe('Unit: Instance', function () {
         });
     });
 
-    describe('running getter', function () {
-        it('returns false if running property not set in config', function () {
-            let hasStub = sandbox.stub().withArgs('running').returns(false);
-            class TestInstance extends Instance {
-                get cliConfig() { return { has: hasStub } };
-            }
-            let testInstance = new TestInstance({}, {}, '');
-
-            let running = testInstance.running;
-            expect(running).to.be.false;
-            expect(hasStub.calledOnce).to.be.true;
-        });
-
-        it('loads running environment and checks if process manager returns false', function () {
-            let hasStub = sandbox.stub().withArgs('running').returns(true);
-            let isRunningStub = sandbox.stub().returns(true);
-            class TestInstance extends Instance {
-                get cliConfig() { return { has: hasStub }; }
-                get process() { return { isRunning: isRunningStub } }
-            };
-            let testInstance = new TestInstance({}, {}, '');
-            let loadRunEnvStub = sandbox.stub(testInstance, 'loadRunningEnvironment');
-
-            let running = testInstance.running;
-            expect(running).to.be.true;
-            expect(hasStub.calledOnce).to.be.true;
-            expect(isRunningStub.calledOnce).to.be.true;
-            expect(loadRunEnvStub.calledOnce).to.be.true;
-        });
-
-        it('sets running to null in cliConfig if process manager\'s isRunning method returns false', function () {
-            let hasStub = sandbox.stub().withArgs('running').returns(true);
-            let setStub = sandbox.stub().withArgs('running', null).returnsThis();
-            let saveStub = sandbox.stub().returnsThis();
-            let isRunningStub = sandbox.stub().returns(false);
-            class TestInstance extends Instance {
-                get cliConfig() { return { has: hasStub, set: setStub, save: saveStub }; }
-                get process() { return { isRunning: isRunningStub }; }
-            }
-            let testInstance = new TestInstance({}, {}, '');
-            let loadRunEnvStub = sandbox.stub(testInstance, 'loadRunningEnvironment');
-
-            let running = testInstance.running;
-            expect(running).to.be.false;
-            expect(hasStub.calledOnce).to.be.true;
-            expect(setStub.calledOnce).to.be.true;
-            expect(saveStub.calledOnce).to.be.true;
-            expect(isRunningStub.calledOnce).to.be.true;
-            expect(loadRunEnvStub.calledOnce).to.be.true;
-        });
-    });
-
-    describe('running setter', function () {
-        it('sets running property in cliConfig', function () {
-            let setStub = sandbox.stub().withArgs('running', 'testing').returnsThis();
-            let saveStub = sandbox.stub();
-            class TestInstance extends Instance {
-                get cliConfig() { return { set: setStub, save: saveStub }; }
-            }
-            let testInstance = new TestInstance({}, {}, '');
-            testInstance.running = 'testing';
-
-            expect(setStub.calledOnce).to.be.true;
-            expect(saveStub.calledOnce).to.be.true;
-        });
-    });
-
     describe('config getter', function () {
         it('returns cached instance if it exists and environment hasn\'t changed', function () {
             let testInstance = new Instance({}, { environment: 'testing' }, '');
@@ -235,6 +168,71 @@ describe('Unit: Instance', function () {
         expect(testInstance.dir).to.equal('some_test_dir');
     });
 
+    describe('running', function () {
+        it('sets running property in cliConfig', function () {
+            let setStub = sandbox.stub().withArgs('running', 'testing').returnsThis();
+            let saveStub = sandbox.stub();
+            class TestInstance extends Instance {
+                get cliConfig() { return { set: setStub, save: saveStub }; }
+            }
+            let testInstance = new TestInstance({}, {}, '');
+            testInstance.running('testing');
+
+            expect(setStub.calledOnce).to.be.true;
+            expect(saveStub.calledOnce).to.be.true;
+        });
+
+        it('returns false if running property not set in config', function () {
+            let hasStub = sandbox.stub().withArgs('running').returns(false);
+            class TestInstance extends Instance {
+                get cliConfig() { return { has: hasStub } };
+            }
+            let testInstance = new TestInstance({}, {}, '');
+
+            let running = testInstance.running();
+            expect(running).to.be.false;
+            expect(hasStub.calledOnce).to.be.true;
+        });
+
+        it('loads running environment and checks if process manager returns false', function () {
+            let hasStub = sandbox.stub().withArgs('running').returns(true);
+            let isRunningStub = sandbox.stub().returns(true);
+            class TestInstance extends Instance {
+                get cliConfig() { return { has: hasStub }; }
+                get process() { return { isRunning: isRunningStub } }
+            };
+            let testInstance = new TestInstance({}, {}, '');
+            let loadRunEnvStub = sandbox.stub(testInstance, 'loadRunningEnvironment');
+
+            let running = testInstance.running();
+            expect(running).to.be.true;
+            expect(hasStub.calledOnce).to.be.true;
+            expect(isRunningStub.calledOnce).to.be.true;
+            expect(loadRunEnvStub.calledOnce).to.be.true;
+        });
+
+        it('sets running to null in cliConfig if process manager\'s isRunning method returns false', function () {
+            let hasStub = sandbox.stub().withArgs('running').returns(true);
+            let setStub = sandbox.stub().withArgs('running', null).returnsThis();
+            let saveStub = sandbox.stub().returnsThis();
+            let isRunningStub = sandbox.stub().returns(false);
+            class TestInstance extends Instance {
+                get cliConfig() { return { has: hasStub, set: setStub, save: saveStub }; }
+                get process() { return { isRunning: isRunningStub }; }
+            }
+            let testInstance = new TestInstance({}, {}, '');
+            let loadRunEnvStub = sandbox.stub(testInstance, 'loadRunningEnvironment');
+
+            let running = testInstance.running();
+            expect(running).to.be.false;
+            expect(hasStub.calledOnce).to.be.true;
+            expect(setStub.calledOnce).to.be.true;
+            expect(saveStub.calledOnce).to.be.true;
+            expect(isRunningStub.calledOnce).to.be.true;
+            expect(loadRunEnvStub.calledOnce).to.be.true;
+        });
+    });
+
     describe('checkEnvironment', function () {
         it('doesn\'t do anything if environment is not production', function () {
             let logStub = sandbox.stub();
@@ -326,9 +324,9 @@ describe('Unit: Instance', function () {
         it('returns shortened object if running is false', function () {
             let getStub = sandbox.stub().withArgs('active-version').returns('1.0.0');
             class TestInstance extends Instance {
-                get running() { return false; }
                 get name() { return 'testing'; }
                 get cliConfig() { return { get: getStub }; }
+                running() { return false; }
             }
             let testInstance = new TestInstance({}, {}, '');
             let result = testInstance.summary();
@@ -347,10 +345,10 @@ describe('Unit: Instance', function () {
 
             class TestInstance extends Instance {
                 get name() { return 'testing'; }
-                get running() { return true; }
                 get cliConfig() { return { get: cliGetStub}; }
                 get config() { return { get: getStub}; }
                 get process() { return { name: 'local' }; }
+                running() { return true; }
             }
             let testInstance = new TestInstance({}, {environment: 'testing'}, '');
             let loadRunEnvStub = sandbox.stub(testInstance, 'loadRunningEnvironment');