From 0841742ec81eff70906dfe50b04fcf3d59e9e12c Mon Sep 17 00:00:00 2001 From: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Date: Tue, 30 May 2023 13:18:52 -0400 Subject: [PATCH] [8.8] [cli] Adjust CLI to treat extra options consistently (#158257) (#158670) # Backport This will backport the following commits from `main` to `8.8`: - [[cli] Adjust CLI to treat extra options consistently (#158257)](https://github.com/elastic/kibana/pull/158257) ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) Co-authored-by: Alex Szabo --- src/cli/command.js | 44 ++++-- src/cli/command.test.js | 128 ++++++++++++++++++ .../serverless_config_flag.test.ts | 2 +- 3 files changed, 158 insertions(+), 16 deletions(-) create mode 100644 src/cli/command.test.js diff --git a/src/cli/command.js b/src/cli/command.js index b85037330027..936a8e9524fd 100644 --- a/src/cli/command.js +++ b/src/cli/command.js @@ -53,30 +53,44 @@ Command.prototype.collectUnknownOptions = function () { this.allowUnknownOption(); this.getUnknownOptions = function () { const opts = {}; + + /** + * Commander.js already presents the unknown args split by "=", + * and shorthand switches already split, like -asd => -a, -s, -d + */ const unknowns = this.unknownArgv(); + const singleFlagArgs = unknowns.filter((flag) => { + return flag.match(/^-[a-zA-Z0-9]$/); + }); + + if (singleFlagArgs.length) { + this.error( + `${title} shouldn't have unknown shorthand flag arguments (${singleFlagArgs}). Possibly an argumentation error?` + ); + } + while (unknowns.length) { - const opt = unknowns.shift().split('='); - if (opt[0].slice(0, 2) !== '--') { - this.error(`${title} "${opt[0]}" must start with "--"`); + const optName = unknowns.shift(); + + if (optName.slice(0, 2) !== '--') { + this.error(`${title} "${optName}" must start with "--"`); } - if (opt.length === 1) { - if (!unknowns.length || unknowns[0][0] === '-') { - this.error(`${title} "${opt[0]}" must have a value`); - } - - opt.push(unknowns.shift()); + if (unknowns.length === 0) { + this.error(`${title} "${optName}" must have a value`); } - let val = opt[1]; + const optValue = unknowns.shift(); + + let val = optValue; try { - val = JSON.parse(opt[1]); - } catch (e) { - val = opt[1]; + val = JSON.parse(optValue); + } catch { + val = optValue; } - set(opts, opt[0].slice(2), val); + set(opts, optName.slice(2), val); } return opts; @@ -96,7 +110,7 @@ Command.prototype.action = _.wrap(Command.prototype.action, function (action, fn const ret = fn.apply(this, args); if (ret && typeof ret.then === 'function') { ret.then(null, function (e) { - console.log('FATAL CLI ERROR', e.stack); + console.log('FATAL CLI ERROR', e.stack); process.exit(1); }); } diff --git a/src/cli/command.test.js b/src/cli/command.test.js new file mode 100644 index 000000000000..f00a0506866f --- /dev/null +++ b/src/cli/command.test.js @@ -0,0 +1,128 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import commander from 'commander'; + +import './command'; + +describe('Commander extensions', () => { + const mockExit = jest.fn((exitCode) => { + // Prevent exiting from shell, let's throw something instead. + throw new Error('Exit ' + exitCode); + }); + + beforeAll(() => { + process._real_exit = process.exit; + + process.exit = mockExit; + }); + + afterAll(() => { + process.exit = process._real_exit; + }); + + describe('regular options', () => { + it('collects multiple args', () => { + const args = makeArgs('test1', '--code', 'xoxo', '--param', 123); + + const { collectedOptions, unknownOptions } = parseArgs('test1', ['code', 'param'], args); + + expect(collectedOptions).toEqual({ code: 'xoxo', param: 123 }); + expect(unknownOptions).toEqual({}); + }); + }); + + describe('unknown args', () => { + it('will be collected besides defined ones', () => { + const args = makeArgs('test2', '--code', 'xoxo', '--specialConfig', 'foobar'); + + const { collectedOptions, unknownOptions } = parseArgs('test2', ['code'], args); + + expect(collectedOptions).toEqual({ code: 'xoxo' }); + expect(unknownOptions).toEqual({ specialConfig: 'foobar' }); + }); + + it('will be collected with dashes, when quoted', () => { + const args = makeArgs('test3', '--code', 'xoxo', '--specialConfig', '"---foobar"'); + + const { collectedOptions, unknownOptions } = parseArgs('test3', ['code'], args); + + expect(collectedOptions).toEqual({ code: 'xoxo' }); + expect(unknownOptions).toEqual({ specialConfig: '---foobar' }); + }); + + it('will be collected with dashes as compound option', () => { + const args = makeArgs('test4', '--code', 'xoxo', '--specialConfig=----foobar'); + + const { collectedOptions, unknownOptions } = parseArgs('test4', ['code'], args); + + expect(collectedOptions).toEqual({ code: 'xoxo' }); + expect(unknownOptions).toEqual({ specialConfig: '----foobar' }); + }); + + it('will crash if they contain bool flags', () => { + const args = makeArgs('test5', '--code', 'xoxo', '--secretOpt', '1234', '--useSpecial'); + + expect(() => parseArgs('test5', ['code'], args)).toThrow(/Exit/); + + expect(mockExit).toHaveBeenCalledWith(64); + }); + + it('will crash if they contain shorthand flags', () => { + const args = makeArgs('test6', '--code', 'xoxo', '-xvz'); + + expect(() => parseArgs('test6', ['code'], args)).toThrow(/Exit/); + + expect(mockExit).toHaveBeenCalledWith(64); + }); + }); +}); + +function prepareProgram(testName, options) { + const optionsDefinitions = options.map((optionName) => { + const short = optionName[0].toLowerCase(); + return `-${short}, --${optionName} <${optionName}>`; + }); + + let command = commander.command(testName); + + for (const option of optionsDefinitions) { + command = command.option(option); + } + + return command; +} + +function parseArgs(testName, options, args) { + let storedActionArgs = null; + + const command = prepareProgram(testName, options) + .collectUnknownOptions() + .action((command) => { + const colllectedOptions = {}; + + options.forEach((opt) => { + colllectedOptions[opt] = command[opt]; + }); + + storedActionArgs = colllectedOptions; + }); + + // Pass the args at the root command + commander.parse(args); + + return { + collectedOptions: storedActionArgs, + unknownOptions: command.getUnknownOptions(), + command, + }; +} + +function makeArgs(...args) { + return ['node', 'test.js', ...args]; +} diff --git a/src/cli/serve/integration_tests/serverless_config_flag.test.ts b/src/cli/serve/integration_tests/serverless_config_flag.test.ts index 6c67ba6261eb..ef706d6a75c1 100644 --- a/src/cli/serve/integration_tests/serverless_config_flag.test.ts +++ b/src/cli/serve/integration_tests/serverless_config_flag.test.ts @@ -28,7 +28,7 @@ describe('cli serverless project type', () => { expect(error).toBe(undefined); expect(stdout.toString('utf8')).toContain( - 'FATAL CLI ERROR Error: invalid --serverless value, must be one of es, oblt, security' + 'FATAL CLI ERROR Error: invalid --serverless value, must be one of es, oblt, security' ); expect(status).toBe(1);