From 7fa2f437c181334066eee6c50648387c66ba404b Mon Sep 17 00:00:00 2001 From: Juan Cruz Viotti Date: Tue, 10 Jan 2017 19:46:59 -0400 Subject: [PATCH] feat(CLI): replace `--robot` with an `ETCHER_CLI_ROBOT` env var (#1010) The `--robot` option of the CLI causes the program to output machine-parseable strings, which can be easily consumed by the GUI to update progress and other information. The problem is that if the CLI fails to parse its command line arguments when being called from the GUI for whatever reason, the `.fail()` Yargs handler will be called, which doesn't output error information in the usual "robot" format, causing the GUI to not understand the error message. Since the `.fail()` Yargs handler doesn't have access to the passed options, we moved the "robot" functionality to an environment variable, which we can easily check from there. As a bonus, this PR refactors the whole robot logic into `lib/shared/robot.js` and adds unit tests to it. See: https://github.com/resin-io/etcher/issues/986 Change-Type: major Changelog-Entry: Replace the `--robot` CLI option with an `ETCHER_CLI_ROBOT` environment variable. Signed-off-by: Juan Cruz Viotti --- docs/ARCHITECTURE.md | 5 +- docs/CLI.md | 13 +- lib/cli/cli.js | 19 +- lib/cli/etcher.js | 43 ++-- lib/shared/child-writer/index.js | 24 +-- lib/shared/child-writer/utils.js | 1 - lib/shared/child-writer/writer-proxy.js | 14 +- lib/shared/robot.js | 235 ++++++++++++++++++++++ tests/shared/robot.spec.js | 256 ++++++++++++++++++++++++ 9 files changed, 545 insertions(+), 65 deletions(-) create mode 100644 lib/shared/robot.js create mode 100644 tests/shared/robot.spec.js diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6965d929..7db7c9fc 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -94,8 +94,9 @@ contains certain features to ease communication: - [Well-documented exit codes.][exit-codes] -- A `--robot` option, which causes the Etcher CLI to output state in a way that -can be easily machine-parsed. +- An environment variable called `ETCHER_CLI_ROBOT` option, which when set + causes the Etcher CLI to output state in a way that can be easily + parser by a machine. Summary ------- diff --git a/docs/CLI.md b/docs/CLI.md index 2f266661..095fc607 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -40,7 +40,6 @@ Options --version, -v show version number --drive, -d drive --check, -c validate write - --robot, -r parse-able output without interactivity --yes, -y confirm non-interactively --unmount, -u unmount on success ``` @@ -48,13 +47,13 @@ Options The robot option ---------------- -The `--robot` option is very particular since it allows other applications to -easily consume the output of the Etcher CLI in real-time. When using the -`--robot` option, the `--yes` option is implicit, therefore you need to -manually specify `--drive`. +Setting the `ETCHER_CLI_ROBOT` environment variable allows other applications +to easily consume the output of the Etcher CLI in real-time When using the +`ETCHER_CLI_ROBOT` option, the `--yes` option is implicit, therefore you need +to manually specify `--drive`. -When `--robot` is used, the program will output JSON lines containing the -progress state and other useful information. For example: +When `ETCHER_CLI_ROBOT` is used, the program will output JSON lines containing +the progress state and other useful information. For example: ``` $ sudo etcher image.iso --robot --drive /dev/disk2 diff --git a/lib/cli/cli.js b/lib/cli/cli.js index da2c2cff..f1cb22e3 100644 --- a/lib/cli/cli.js +++ b/lib/cli/cli.js @@ -20,6 +20,7 @@ const _ = require('lodash'); const fs = require('fs'); const yargs = require('yargs'); const utils = require('./utils'); +const robot = require('../shared/robot'); const EXIT_CODES = require('../shared/exit-codes'); const packageJSON = require('../../package.json'); @@ -63,9 +64,14 @@ module.exports = yargs .version(_.constant(packageJSON.version)) // Error reporting - .fail((message, error) => { - yargs.showHelp(); - utils.printError(error || message); + .fail(function(message, error) { + if (robot.isEnabled(process.env)) { + robot.printError(error || message); + } else { + yargs.showHelp(); + utils.printError(error || message); + } + process.exit(1); }) @@ -76,7 +82,7 @@ module.exports = yargs }) .check((argv) => { - if (argv.robot && !argv.drive) { + if (robot.isEnabled(process.env) && !argv.drive) { throw new Error('Missing drive'); } @@ -105,11 +111,6 @@ module.exports = yargs alias: 'c', default: true }, - robot: { - describe: 'parse-able output without interactivity', - boolean: true, - alias: 'r' - }, yes: { describe: 'confirm non-interactively', boolean: true, diff --git a/lib/cli/etcher.js b/lib/cli/etcher.js index 127caa98..4d82f98d 100644 --- a/lib/cli/etcher.js +++ b/lib/cli/etcher.js @@ -25,6 +25,7 @@ const drivelist = Bluebird.promisifyAll(require('drivelist')); const writer = require('./writer'); const utils = require('./utils'); const options = require('./cli'); +const robot = require('../shared/robot'); const EXIT_CODES = require('../shared/exit-codes'); isElevated().then((elevated) => { @@ -51,7 +52,7 @@ isElevated().then((elevated) => { // If `options.yes` is `false`, pass `undefined`, // otherwise the question will not be asked because // `false` is a defined value. - yes: options.robot || options.yes || undefined + yes: robot.isEnabled(process.env) || options.yes || undefined } }); @@ -79,16 +80,13 @@ isElevated().then((elevated) => { validateWriteOnSuccess: options.check }, (state) => { - if (options.robot) { - console.log(JSON.stringify({ - command: 'progress', - data: { - type: state.type, - percentage: Math.floor(state.percentage), - eta: state.eta, - speed: Math.floor(state.speed) - } - })); + if (robot.isEnabled(process.env)) { + robot.printMessage('progress', { + type: state.type, + percentage: Math.floor(state.percentage), + eta: state.eta, + speed: Math.floor(state.speed) + }); } else { progressBars[state.type].update(state); } @@ -98,13 +96,10 @@ isElevated().then((elevated) => { }).then((results) => { return Bluebird.try(() => { - if (options.robot) { - return console.log(JSON.stringify({ - command: 'done', - data: { - sourceChecksum: results.sourceChecksum - } - })); + if (robot.isEnabled(process.env)) { + return robot.printMessage('done', { + sourceChecksum: results.sourceChecksum + }); } console.log('Your flash is complete!'); @@ -120,16 +115,8 @@ isElevated().then((elevated) => { }).catch((error) => { return Bluebird.try(() => { - if (options.robot) { - return console.error(JSON.stringify({ - command: 'error', - data: { - message: error.message, - description: error.description, - stacktrace: error.stack, - code: error.code - } - })); + if (robot.isEnabled(process.env)) { + return robot.printError(error); } utils.printError(error); diff --git a/lib/shared/child-writer/index.js b/lib/shared/child-writer/index.js index 3ae9d005..0027e497 100644 --- a/lib/shared/child-writer/index.js +++ b/lib/shared/child-writer/index.js @@ -24,6 +24,7 @@ const rendererUtils = require('./renderer-utils'); const utils = require('./utils'); const CONSTANTS = require('./constants'); const EXIT_CODES = require('../exit-codes'); +const robot = require('../robot'); /** * @summary Perform a write @@ -97,32 +98,21 @@ exports.write = (image, drive, options) => { ipc.server.on('error', emitError); ipc.server.on('message', (data) => { let message; - try { - message = JSON.parse(data); - } catch (error) { - error.description = `${data}, ${error.message}`; - error.message = 'Invalid message from the writer process'; - return emitError(error); - } - if (!message.command || !message.data) { - const error = new Error('Invalid message from the writer process'); - error.description = `No command or data: ${data}`; + try { + message = robot.parseMessage(data); + } catch (error) { return emitError(error); } // The error object is decomposed by the CLI for serialisation // purposes. We compose it back to an `Error` here in order // to provide better encapsulation. - if (message.command === 'error') { - const error = new Error(message.data.message); - error.code = message.data.code; - error.description = message.data.description; - error.stack = message.data.stacktrace; - return emitError(error); + if (robot.getCommand(message) === 'error') { + return emitError(robot.recomposeErrorMessage(message)); } - emitter.emit(message.command, message.data); + emitter.emit(robot.getCommand(message), robot.getData(message)); }); ipc.server.on('start', () => { diff --git a/lib/shared/child-writer/utils.js b/lib/shared/child-writer/utils.js index 88f0af30..8e09fa6f 100644 --- a/lib/shared/child-writer/utils.js +++ b/lib/shared/child-writer/utils.js @@ -72,7 +72,6 @@ exports.getCLIWriterArguments = (options) => { const argv = [ options.entryPoint, options.image, - '--robot', '--drive', options.device, diff --git a/lib/shared/child-writer/writer-proxy.js b/lib/shared/child-writer/writer-proxy.js index a399eba2..d9e86ffb 100644 --- a/lib/shared/child-writer/writer-proxy.js +++ b/lib/shared/child-writer/writer-proxy.js @@ -152,7 +152,19 @@ return isElevated().then((elevated) => { ipc.connectTo(process.env.IPC_SERVER_ID, () => { ipc.of[process.env.IPC_SERVER_ID].on('error', reject); ipc.of[process.env.IPC_SERVER_ID].on('connect', () => { - const child = childProcess.spawn(EXECUTABLE, ETCHER_ARGUMENTS); + + const child = childProcess.spawn(EXECUTABLE, ETCHER_ARGUMENTS, { + env: { + + // The CLI might call operating system utilities (like `diskutil`), + // so we must ensure the `PATH` is inherited. + PATH: process.env.PATH, + + ELECTRON_RUN_AS_NODE: 1, + ETCHER_CLI_ROBOT: 1 + } + }); + ipc.of[process.env.IPC_SERVER_ID].on('disconnect', _.bind(child.kill, child)); child.on('error', reject); child.on('close', resolve); diff --git a/lib/shared/robot.js b/lib/shared/robot.js new file mode 100644 index 00000000..738b4997 --- /dev/null +++ b/lib/shared/robot.js @@ -0,0 +1,235 @@ +/* + * Copyright 2016 resin.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +const _ = require('lodash'); + +/** + * @summary Check whether we should emit parseable output + * @function + * @public + * + * @param {Object} environment - environment + * @returns {Boolean} whether we should emit parseable output + * + * @example + * if (robot.isEnabled(process.env)) { + * console.log('We should emit parseable output'); + * } + */ +exports.isEnabled = (environment) => { + const value = _.get(environment, 'ETCHER_CLI_ROBOT', false); + return Boolean(value === 'false' ? false : value); +}; + +/** + * @summary Build a machine-parseable message + * @function + * @private + * + * @param {String} title - message title + * @param {Object} [data] - message data + * @returns {String} parseable message + * + * @example + * const message = robot.buildMessage('progress', { + * percentage: 50 + * }); + * + * console.log(message); + * > '{"command":"progress","data":{"percentage":50}}' + */ +exports.buildMessage = (title, data = {}) => { + if (!_.isPlainObject(data)) { + throw new Error(`Invalid data: ${data}`); + } + + return JSON.stringify({ + command: title, + data: data + }); +}; + +/** + * @summary Parse a machine-parseable message + * @function + * @public + * + * @param {String} string - message string + * @returns {Object} parsed message + * + * @example + * const result = robot.parseMessage('{"command":"progress","data":{"foo":50}}'); + * console.log(message); + * > { + * > command: 'progress', + * > data: { + * > foo: 50 + * > } + * > } + */ +exports.parseMessage = (string) => { + let output; + + try { + output = JSON.parse(string); + } catch (error) { + error.message = 'Invalid message'; + error.description = `${string}, ${error.message}`; + throw error; + } + + if (!output.command || !output.data) { + const error = new Error('Invalid message'); + error.description = `No command or data: ${string}`; + throw error; + } + + return output; +}; + +/** + * @summary Build a machine-parseable error message + * @function + * @private + * + * @param {(String|Error)} error - error + * @returns {String} parseable error message + * + * @example + * const error = new Error('foo'); + * const errorMessage = robot.buildErrorMessage(error); + * + * console.log(error.command); + * > 'error' + * + * console.log(error.data.message); + * > 'foo' + * + * error.data.stacktrace === error.stack; + * > true + */ +exports.buildErrorMessage = (error) => { + if (_.isString(error)) { + error = new Error(error); + } + + return exports.buildMessage('error', { + message: error.message, + description: error.description, + stacktrace: error.stack, + code: error.code + }); +}; + +/** + * @summary Recompose an error message + * @function + * @public + * + * @param {String} message - error message + * @returns {Error} error object + * + * @example + * const message = robot.buildErrorMessage(new Error('foo')); + * const error = robot.recomposeErrorMessage(robot.parseMessage(message)); + * + * error instanceof Error; + * > true + * + * console.log(error.message); + * > 'foo' + */ +exports.recomposeErrorMessage = (message) => { + const error = new Error(message.data.message); + _.assign(error, _.omit(message.data, 'stacktrace')); + error.stack = message.data.stacktrace; + return error; +}; + +/** + * @summary Get message command + * @function + * @public + * + * @param {Object} message - message + * @returns {String} command + * + * @example + * const command = robot.getCommand({ + * command: 'foo', + * data: {} + * }); + * + * console.log(command); + * > 'foo' + */ +exports.getCommand = (message) => { + return _.get(message, 'command'); +}; + +/** + * @summary Get message data + * @function + * @public + * + * @param {Object} message - message + * @returns {Object} data + * + * @example + * const data = robot.getData({ + * command: 'foo', + * data: { + * foo: 1 + * } + * }); + * + * console.log(data); + * > { foo: 1 } + */ +exports.getData = (message) => { + return _.get(message, 'data', {}); +}; + +/** + * @summary Print an error in a machine-friendly way + * @function + * @public + * + * @param {(Error|String)} error - error + * + * @example + * robot.printError(new Error('This is an error')); + */ +exports.printError = (error) => { + console.error(exports.buildErrorMessage(error)); +}; + +/** + * @summary Print a message in a machine-friendly way + * @function + * @public + * + * @param {String} message - message + * @param {Object} [data] - data + * + * @example + * robot.printMessage('progress', { percentage: 50 }); + */ +exports.printMessage = (message, data) => { + console.log(exports.buildMessage(message, data)); +}; diff --git a/tests/shared/robot.spec.js b/tests/shared/robot.spec.js new file mode 100644 index 00000000..5ea96d64 --- /dev/null +++ b/tests/shared/robot.spec.js @@ -0,0 +1,256 @@ +/* + * Copyright 2016 resin.io + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +const m = require('mochainon'); +const _ = require('lodash'); +const robot = require('../../lib/shared/robot'); + +describe('Shared: Robot', function() { + + describe('.isEnabled()', function() { + + it('should return false if ETCHER_CLI_ROBOT is not set', function() { + m.chai.expect(robot.isEnabled({})).to.be.false; + }); + + it('should return true if ETCHER_CLI_ROBOT=1', function() { + m.chai.expect(robot.isEnabled({ + ETCHER_CLI_ROBOT: 1 + })).to.be.true; + }); + + it('should return false if ETCHER_CLI_ROBOT=0', function() { + m.chai.expect(robot.isEnabled({ + ETCHER_CLI_ROBOT: 0 + })).to.be.false; + }); + + it('should return true if ETCHER_CLI_ROBOT="true"', function() { + m.chai.expect(robot.isEnabled({ + ETCHER_CLI_ROBOT: 'true' + })).to.be.true; + }); + + it('should return false if ETCHER_CLI_ROBOT="false"', function() { + m.chai.expect(robot.isEnabled({ + ETCHER_CLI_ROBOT: 'false' + })).to.be.false; + }); + + it('should return true if ETCHER_CLI_ROBOT=true', function() { + m.chai.expect(robot.isEnabled({ + ETCHER_CLI_ROBOT: true + })).to.be.true; + }); + + it('should return false if ETCHER_CLI_ROBOT=false', function() { + m.chai.expect(robot.isEnabled({ + ETCHER_CLI_ROBOT: false + })).to.be.false; + }); + + }); + + describe('.buildMessage()', function() { + + it('should build a message without data', function() { + const message = robot.buildMessage('hello'); + const result = '{"command":"hello","data":{}}'; + m.chai.expect(message).to.equal(result); + }); + + it('should build a message with data', function() { + const message = robot.buildMessage('hello', { + foo: 1, + bar: 2 + }); + const result = '{"command":"hello","data":{"foo":1,"bar":2}}'; + m.chai.expect(message).to.equal(result); + }); + + it('should throw if data is defined but it not an object', function() { + m.chai.expect(() => { + robot.buildMessage('hello', 'world'); + }).to.throw('Invalid data: world'); + }); + + }); + + describe('.buildErrorMessage()', function() { + + it('should build a message from a simple error', function() { + const error = new Error('foo'); + const message = robot.buildErrorMessage(error); + + m.chai.expect(JSON.parse(message)).to.deep.equal({ + command: 'error', + data: { + message: 'foo', + stacktrace: error.stack + } + }); + }); + + it('should save the error description', function() { + const error = new Error('foo'); + error.description = 'error description'; + const message = robot.buildErrorMessage(error); + + m.chai.expect(JSON.parse(message)).to.deep.equal({ + command: 'error', + data: { + message: 'foo', + description: 'error description', + stacktrace: error.stack + } + }); + }); + + it('should save the error code', function() { + const error = new Error('foo'); + error.code = 'MYERROR'; + const message = robot.buildErrorMessage(error); + + m.chai.expect(JSON.parse(message)).to.deep.equal({ + command: 'error', + data: { + message: 'foo', + code: 'MYERROR', + stacktrace: error.stack + } + }); + }); + + it('should handle a string error', function() { + const message = JSON.parse(robot.buildErrorMessage('foo')); + m.chai.expect(message.data.message).to.equal('foo'); + m.chai.expect(message.data.stacktrace).to.be.a.string; + m.chai.expect(_.isEmpty(message.data.stacktrace)).to.be.false; + }); + + }); + + describe('.parseMessage()', function() { + + it('should parse a valid message', function() { + const message = robot.buildMessage('foo', { + bar: 1 + }); + + m.chai.expect(robot.parseMessage(message)).to.deep.equal({ + command: 'foo', + data: { + bar: 1 + } + }); + }); + + it('should parse a valid without data', function() { + const message = robot.buildMessage('foo'); + m.chai.expect(robot.parseMessage(message)).to.deep.equal({ + command: 'foo', + data: {} + }); + }); + + it('should throw if input is not valid JSON', function() { + m.chai.expect(() => { + robot.parseMessage('Hello world\nFoo Bar'); + }).to.throw('Invalid message'); + }); + + it('should throw if input has no command', function() { + m.chai.expect(() => { + robot.parseMessage('{"data":{"foo":"bar"}}'); + }).to.throw('Invalid message'); + }); + + it('should throw if input has no data', function() { + m.chai.expect(() => { + robot.parseMessage('{"command":"foo"}'); + }).to.throw('Invalid message'); + }); + + }); + + describe('.getCommand()', function() { + + it('should get the command of a message', function() { + const message = robot.parseMessage(robot.buildMessage('hello', { + foo: 1, + bar: 2 + })); + + m.chai.expect(robot.getCommand(message)).to.equal('hello'); + }); + + }); + + describe('.getData()', function() { + + it('should get the data of a message', function() { + const message = robot.parseMessage(robot.buildMessage('hello', { + foo: 1, + bar: 2 + })); + + m.chai.expect(robot.getData(message)).to.deep.equal({ + foo: 1, + bar: 2 + }); + }); + + it('should return an empty object if the message has no data', function() { + m.chai.expect(robot.getData({ + command: 'foo' + })).to.deep.equal({}); + }); + + }); + + describe('.recomposeErrorMessage()', function() { + + it('should return an instance of Error', function() { + const error = new Error('Foo bar'); + const message = robot.parseMessage(robot.buildErrorMessage(error)); + m.chai.expect(robot.recomposeErrorMessage(message)).to.be.an.instanceof(Error); + }); + + it('should be able to recompose an error object', function() { + const error = new Error('Foo bar'); + const message = robot.parseMessage(robot.buildErrorMessage(error)); + m.chai.expect(robot.recomposeErrorMessage(message)).to.deep.equal(error); + }); + + it('should be able to recompose an error object with a code', function() { + const error = new Error('Foo bar'); + error.code = 'FOO'; + const message = robot.parseMessage(robot.buildErrorMessage(error)); + m.chai.expect(robot.recomposeErrorMessage(message)).to.deep.equal(error); + }); + + it('should be able to recompose an error object with a description', function() { + const error = new Error('Foo bar'); + error.description = 'My description'; + const message = robot.parseMessage(robot.buildErrorMessage(error)); + m.chai.expect(robot.recomposeErrorMessage(message)).to.deep.equal(error); + }); + + }); + +});