From 51608146237fb963d386cfbcee379aa55327afc3 Mon Sep 17 00:00:00 2001 From: Benjin Dubishar Date: Thu, 4 Nov 2021 20:16:58 -0400 Subject: [PATCH] Dedupe shell command execution logic (#17516) * Moved to shellExecutionHelper * First crack * fixed the deploy tests * PR comments * trigger GitHub actions Co-authored-by: llali --- .../sql-database-projects/src/common/utils.ts | 48 ------------- .../src/models/deploy/deployService.ts | 31 +++++---- .../src/test/deploy/deployService.test.ts | 51 +++++++++----- .../src/test/netCoreTool.test.ts | 3 +- .../src/tools/autorestHelper.ts | 4 +- .../src/tools/netcoreTool.ts | 2 +- .../src/tools/shellExecutionHelper.ts | 68 ++++++++++++------- 7 files changed, 100 insertions(+), 107 deletions(-) diff --git a/extensions/sql-database-projects/src/common/utils.ts b/extensions/sql-database-projects/src/common/utils.ts index fe7a8b41ca..7aaad73e3a 100644 --- a/extensions/sql-database-projects/src/common/utils.ts +++ b/extensions/sql-database-projects/src/common/utils.ts @@ -12,7 +12,6 @@ import * as glob from 'fast-glob'; import * as dataworkspace from 'dataworkspace'; import * as mssql from '../../../mssql'; import * as vscodeMssql from 'vscode-mssql'; -import * as childProcess from 'child_process'; import * as fse from 'fs-extra'; import * as which from 'which'; import { promises as fs } from 'fs'; @@ -426,53 +425,6 @@ export async function createFolderIfNotExist(folderPath: string): Promise } } -export async function executeCommand(cmd: string, outputChannel: vscode.OutputChannel, sensitiveData: string[] = [], timeout: number = 5 * 60 * 1000): Promise { - return new Promise((resolve, reject) => { - if (outputChannel) { - let cmdOutputMessage = cmd; - - sensitiveData.forEach(element => { - cmdOutputMessage = cmdOutputMessage.replace(element, '***'); - }); - - outputChannel.appendLine(` > ${cmdOutputMessage}`); - } - let child = childProcess.exec(cmd, { - timeout: timeout - }, (err, stdout) => { - if (err) { - - // removing sensitive data from the exception - sensitiveData.forEach(element => { - err.cmd = err.cmd?.replace(element, '***'); - err.message = err.message?.replace(element, '***'); - }); - reject(err); - } else { - resolve(stdout); - } - }); - - // Add listeners to print stdout and stderr if an output channel was provided - - if (child?.stdout) { - child.stdout.on('data', data => { outputDataChunk(outputChannel, data, ' stdout: '); }); - } - if (child?.stderr) { - child.stderr.on('data', data => { outputDataChunk(outputChannel, data, ' stderr: '); }); - } - }); -} - -export function outputDataChunk(outputChannel: vscode.OutputChannel, data: string | Buffer, header: string): void { - data.toString().split(/\r?\n/) - .forEach(line => { - if (outputChannel) { - outputChannel.appendLine(header + line); - } - }); -} - export async function retry( name: string, attempt: () => Promise, diff --git a/extensions/sql-database-projects/src/models/deploy/deployService.ts b/extensions/sql-database-projects/src/models/deploy/deployService.ts index 84edc05178..5110e96115 100644 --- a/extensions/sql-database-projects/src/models/deploy/deployService.ts +++ b/extensions/sql-database-projects/src/models/deploy/deployService.ts @@ -14,6 +14,7 @@ import * as vscode from 'vscode'; import * as os from 'os'; import { ConnectionResult } from 'azdata'; import * as templates from '../../templates/templates'; +import { ShellExecutionHelper } from '../../tools/shellExecutionHelper'; interface DockerImageSpec { label: string; @@ -22,9 +23,11 @@ interface DockerImageSpec { } export class DeployService { - constructor(private _outputChannel: vscode.OutputChannel) { + constructor(private _outputChannel: vscode.OutputChannel, shellExecutionHelper: ShellExecutionHelper | undefined = undefined) { + this._shellExecutionHelper = shellExecutionHelper ?? new ShellExecutionHelper(this._outputChannel); } + private _shellExecutionHelper: ShellExecutionHelper; private DefaultSqlRetryTimeoutInSec: number = 10; private DefaultSqlNumberOfRetries: number = 3; @@ -92,7 +95,7 @@ export class DeployService { private async verifyDocker(): Promise { try { - await utils.executeCommand(`docker version --format {{.Server.APIVersion}}`, this._outputChannel); + await this.executeCommand(`docker version --format {{.Server.APIVersion}}`); // TODO verify min version } catch (error) { throw Error(constants.dockerNotRunningError(utils.getErrorMessage(error))); @@ -170,7 +173,7 @@ export class DeployService { // Waiting a bit to make sure docker container doesn't crash // const runningDockerId = await utils.retry('Validating the docker container', async () => { - return await utils.executeCommand(`docker ps -q -a --filter label=${imageSpec.label} -q`, this._outputChannel); + return this.executeCommand(`docker ps -q -a --filter label=${imageSpec.label} -q`); }, (dockerId) => { return Promise.resolve({ validated: dockerId !== undefined, errorMessage: constants.dockerContainerNotRunningErrorMessage }); }, (dockerId) => { @@ -186,7 +189,7 @@ export class DeployService { this.logToOutput(constants.dockerContainerFailedToRunErrorMessage); if (createdDockerId) { // Get the docker logs if docker was created but crashed - await utils.executeCommand(constants.dockerLogMessage(createdDockerId), this._outputChannel); + await this.executeCommand(constants.dockerLogMessage(createdDockerId)); } } @@ -201,13 +204,13 @@ export class DeployService { // Running commands to build the docker image this.logToOutput('Building docker image ...'); - await utils.executeCommand(`docker pull ${profile.dockerBaseImage}`, this._outputChannel); - await utils.executeCommand(`docker build -f ${dockerFilePath} -t ${dockerImageSpec.tag} ${root}`, this._outputChannel); - await utils.executeCommand(`docker images --filter label=${dockerImageSpec.label}`, this._outputChannel); + await this.executeCommand(`docker pull ${profile.dockerBaseImage}`); + await this.executeCommand(`docker build -f ${dockerFilePath} -t ${dockerImageSpec.tag} ${root}`); + await this.executeCommand(`docker images --filter label=${dockerImageSpec.label}`); this.logToOutput('Running docker container ...'); - await utils.executeCommand(`docker run -p ${profile.port}:1433 -e "MSSQL_SA_PASSWORD=${profile.password}" -d --name ${dockerImageSpec.containerName} ${dockerImageSpec.tag}`, this._outputChannel, sensitiveData); - return await utils.executeCommand(`docker ps -q -a --filter label=${dockerImageSpec.label} -q`, this._outputChannel); + await this.executeCommand(`docker run -p ${profile.port}:1433 -e "MSSQL_SA_PASSWORD=${profile.password}" -d --name ${dockerImageSpec.containerName} ${dockerImageSpec.tag}`, sensitiveData); + return await this.executeCommand(`docker ps -q -a --filter label=${dockerImageSpec.label} -q`); } private async getConnectionString(connectionUri: string): Promise { @@ -379,7 +382,7 @@ export class DeployService { // await this.createFile(startFilePath, 'echo starting the container!'); if (os.platform() !== 'win32') { - await utils.executeCommand(`chmod +x '${startFilePath}'`, this._outputChannel); + await this.executeCommand(`chmod +x '${startFilePath}'`); } // Create the Dockerfile @@ -401,8 +404,12 @@ RUN ["/bin/bash", "/opt/commands/start.sh"] await fse.writeFile(filePath, content); } + public async executeCommand(cmd: string, sensitiveData: string[] = [], timeout: number = 5 * 60 * 1000): Promise { + return await this._shellExecutionHelper.runStreamedCommand(cmd, undefined, sensitiveData, timeout); + } + public async getCurrentDockerContainer(imageLabel: string): Promise { - const currentIds = await utils.executeCommand(`docker ps -q -a --filter label=${imageLabel}`, this._outputChannel); + const currentIds = await this.executeCommand(`docker ps -q -a --filter label=${imageLabel}`); return currentIds ? currentIds.split(/\r?\n/) : []; } @@ -412,7 +419,7 @@ RUN ["/bin/bash", "/opt/commands/start.sh"] if (id) { for (let commandId = 0; commandId < commandsToClean.length; commandId++) { const command = commandsToClean[commandId]; - await utils.executeCommand(`${command} ${id}`, this._outputChannel); + await this.executeCommand(`${command} ${id}`); } } } diff --git a/extensions/sql-database-projects/src/test/deploy/deployService.test.ts b/extensions/sql-database-projects/src/test/deploy/deployService.test.ts index a420d1b54e..86710f14b5 100644 --- a/extensions/sql-database-projects/src/test/deploy/deployService.test.ts +++ b/extensions/sql-database-projects/src/test/deploy/deployService.test.ts @@ -11,12 +11,13 @@ import { DeployService } from '../../models/deploy/deployService'; import { Project } from '../../models/project'; import * as vscode from 'vscode'; import * as azdata from 'azdata'; -import * as childProcess from 'child_process'; import { AppSettingType, IDeployProfile } from '../../models/deploy/deployProfile'; import * as UUID from 'vscode-languageclient/lib/utils/uuid'; import * as fse from 'fs-extra'; import * as path from 'path'; import * as constants from '../../common/constants'; +import { ShellExecutionHelper } from '../../tools/shellExecutionHelper'; +import * as TypeMoq from 'typemoq'; export interface TestContext { outputChannel: vscode.OutputChannel; @@ -80,12 +81,15 @@ describe('deploy service', function (): void { }; const projFilePath = await testUtils.createTestSqlProjFile(baselines.newProjectFileBaseline); const project1 = await Project.openProject(vscode.Uri.file(projFilePath).fsPath); - const deployService = new DeployService(testContext.outputChannel); + const shellExecutionHelper = TypeMoq.Mock.ofType(ShellExecutionHelper); + shellExecutionHelper.setup(x => x.runStreamedCommand(TypeMoq.It.isAny(), + undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('id')); + const deployService = new DeployService(testContext.outputChannel, shellExecutionHelper.object); sandbox.stub(azdata.connection, 'connect').returns(Promise.resolve(mockConnectionResult)); sandbox.stub(azdata.connection, 'getUriForConnection').returns(Promise.resolve('connection')); sandbox.stub(vscode.window, 'showWarningMessage').returns(Promise.resolve(constants.yesString)); sandbox.stub(azdata.tasks, 'startBackgroundOperation').callThrough(); - sandbox.stub(childProcess, 'exec').yields(undefined, 'id'); + let connection = await deployService.deploy(deployProfile, project1); should(connection).equals('connection'); @@ -106,9 +110,12 @@ describe('deploy service', function (): void { }; const projFilePath = await testUtils.createTestSqlProjFile(baselines.newProjectFileBaseline); const project1 = await Project.openProject(vscode.Uri.file(projFilePath).fsPath); - const deployService = new DeployService(testContext.outputChannel); + const shellExecutionHelper = TypeMoq.Mock.ofType(ShellExecutionHelper); + shellExecutionHelper.setup(x => x.runStreamedCommand(TypeMoq.It.isAny(), + undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.reject('error')); + const deployService = new DeployService(testContext.outputChannel, shellExecutionHelper.object); sandbox.stub(azdata.tasks, 'startBackgroundOperation').callThrough(); - sandbox.stub(childProcess, 'exec').throws('error'); + await should(deployService.deploy(deployProfile, project1)).rejected(); }); @@ -124,13 +131,16 @@ describe('deploy service', function (): void { connectionRetryTimeout: 1 }; - const deployService = new DeployService(testContext.outputChannel); + const shellExecutionHelper = TypeMoq.Mock.ofType(ShellExecutionHelper); + shellExecutionHelper.setup(x => x.runStreamedCommand(TypeMoq.It.isAny(), + undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('id')); + const deployService = new DeployService(testContext.outputChannel, shellExecutionHelper.object); let connectionStub = sandbox.stub(azdata.connection, 'connect'); connectionStub.onFirstCall().returns(Promise.resolve(mockFailedConnectionResult)); connectionStub.onSecondCall().returns(Promise.resolve(mockConnectionResult)); sandbox.stub(azdata.connection, 'getUriForConnection').returns(Promise.resolve('connection')); sandbox.stub(azdata.tasks, 'startBackgroundOperation').callThrough(); - sandbox.stub(childProcess, 'exec').yields(undefined, 'id'); + let connection = await deployService.getConnection(localDbSettings, false, 'master'); should(connection).equals('connection'); }); @@ -178,8 +188,11 @@ describe('deploy service', function (): void { envVariableName: 'SQLConnectionString' }; - const deployService = new DeployService(testContext.outputChannel); - sandbox.stub(childProcess, 'exec').yields(undefined, 'id'); + const shellExecutionHelper = TypeMoq.Mock.ofType(ShellExecutionHelper); + shellExecutionHelper.setup(x => x.runStreamedCommand(TypeMoq.It.isAny(), + undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('id')); + const deployService = new DeployService(testContext.outputChannel, shellExecutionHelper.object); + await deployService.updateAppSettings(appInteg, deployProfile); let newContent = JSON.parse(fse.readFileSync(filePath, 'utf8')); should(newContent).deepEqual(expected); @@ -226,10 +239,13 @@ describe('deploy service', function (): void { appSettingFile: filePath, envVariableName: 'SQLConnectionString', }; - const deployService = new DeployService(testContext.outputChannel); + const shellExecutionHelper = TypeMoq.Mock.ofType(ShellExecutionHelper); + shellExecutionHelper.setup(x => x.runStreamedCommand(TypeMoq.It.isAny(), + undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve('id')); + const deployService = new DeployService(testContext.outputChannel, shellExecutionHelper.object); let connection = new azdata.connection.ConnectionProfile(); sandbox.stub(azdata.connection, 'getConnection').returns(Promise.resolve(connection)); - sandbox.stub(childProcess, 'exec').yields(undefined, 'id'); + sandbox.stub(azdata.connection, 'getConnectionString').returns(Promise.resolve('connectionString')); await deployService.updateAppSettings(appInteg, deployProfile); let newContent = JSON.parse(fse.readFileSync(filePath, 'utf8')); @@ -238,16 +254,15 @@ describe('deploy service', function (): void { it('Should clean a list of docker images successfully', async function (): Promise { const testContext = createContext(); - const deployService = new DeployService(testContext.outputChannel); - - let process = sandbox.stub(childProcess, 'exec').yields(undefined, ` - id + const shellExecutionHelper = TypeMoq.Mock.ofType(ShellExecutionHelper); + shellExecutionHelper.setup(x => x.runStreamedCommand(TypeMoq.It.isAny(), + undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny())).returns(() => Promise.resolve(`id id2 - id3`); - + id3`)); + const deployService = new DeployService(testContext.outputChannel, shellExecutionHelper.object); const ids = await deployService.getCurrentDockerContainer('label'); await deployService.cleanDockerObjects(ids, ['docker stop', 'docker rm']); - should(process.calledThrice); + shellExecutionHelper.verify(x => x.runStreamedCommand(TypeMoq.It.isAny(), undefined, TypeMoq.It.isAny(), TypeMoq.It.isAny()), TypeMoq.Times.exactly(7)); }); it('Should create docker image info correctly', () => { diff --git a/extensions/sql-database-projects/src/test/netCoreTool.test.ts b/extensions/sql-database-projects/src/test/netCoreTool.test.ts index 7d1d957ca9..90f9791311 100644 --- a/extensions/sql-database-projects/src/test/netCoreTool.test.ts +++ b/extensions/sql-database-projects/src/test/netCoreTool.test.ts @@ -62,10 +62,9 @@ describe('NetCoreTool: Net core tests', function (): void { it('should run a command successfully', async function (): Promise { const netcoreTool = new NetCoreTool(testContext.outputChannel); const dummyFile = path.join(await generateTestFolderPath(), 'dummy.dacpac'); - const outputChannel = vscode.window.createOutputChannel('db project test'); try { - await netcoreTool.runStreamedCommand('echo test > ' + getQuotedPath(dummyFile), outputChannel, undefined); + await netcoreTool.runStreamedCommand('echo test > ' + getQuotedPath(dummyFile), undefined); const text = await fs.promises.readFile(dummyFile); should(text.toString().trim()).equal('test'); } diff --git a/extensions/sql-database-projects/src/tools/autorestHelper.ts b/extensions/sql-database-projects/src/tools/autorestHelper.ts index f42331847b..bad282930d 100644 --- a/extensions/sql-database-projects/src/tools/autorestHelper.ts +++ b/extensions/sql-database-projects/src/tools/autorestHelper.ts @@ -51,7 +51,7 @@ export class AutorestHelper extends ShellExecutionHelper { if (response === constants.installGlobally) { this._outputChannel.appendLine(constants.userSelectionInstallGlobally); - await this.runStreamedCommand('npm install autorest -g', this._outputChannel); + await this.runStreamedCommand('npm install autorest -g'); return autorestCommand; } else if (response === constants.runViaNpx) { this._outputChannel.appendLine(constants.userSelectionRunNpx); @@ -99,7 +99,7 @@ export class AutorestHelper extends ShellExecutionHelper { } const command = this.constructAutorestCommand(commandExecutable, specPath, outputFolder); - const output = await this.runStreamedCommand(command, this._outputChannel); + const output = await this.runStreamedCommand(command); return output; } diff --git a/extensions/sql-database-projects/src/tools/netcoreTool.ts b/extensions/sql-database-projects/src/tools/netcoreTool.ts index ed51d12882..4c980d0524 100644 --- a/extensions/sql-database-projects/src/tools/netcoreTool.ts +++ b/extensions/sql-database-projects/src/tools/netcoreTool.ts @@ -216,7 +216,7 @@ export class NetCoreTool extends ShellExecutionHelper { const command = dotnetPath + ' ' + options.argument; try { - return await this.runStreamedCommand(command, this._outputChannel, options); + return await this.runStreamedCommand(command, options); } catch (error) { this._outputChannel.append(localize('sqlDatabaseProject.RunCommand.ErroredOut', "\t>>> {0} … errored out: {1}", command, utils.getErrorMessage(error))); //errors are localized in our code where emitted, other errors are pass through from external components that are not easily localized throw error; diff --git a/extensions/sql-database-projects/src/tools/shellExecutionHelper.ts b/extensions/sql-database-projects/src/tools/shellExecutionHelper.ts index 5dbc1c6a45..1c3cc32a18 100644 --- a/extensions/sql-database-projects/src/tools/shellExecutionHelper.ts +++ b/extensions/sql-database-projects/src/tools/shellExecutionHelper.ts @@ -22,9 +22,15 @@ export class ShellExecutionHelper { /** * spawns the shell command with arguments and redirects the error and output to ADS output channel */ - public async runStreamedCommand(command: string, outputChannel: vscode.OutputChannel, options?: ShellCommandOptions): Promise { + public async runStreamedCommand(command: string, options?: ShellCommandOptions, sensitiveData: string[] = [], timeout: number = 5 * 60 * 1000): Promise { const stdoutData: string[] = []; - outputChannel.appendLine(` > ${command}`); + + let cmdOutputMessage = command; + sensitiveData.forEach(element => { + cmdOutputMessage = cmdOutputMessage.replace(element, '***'); + }); + + this._outputChannel.appendLine(` > ${cmdOutputMessage}`); const spawnOptions = { cwd: options && options.workingDirectory, @@ -33,39 +39,53 @@ export class ShellExecutionHelper { maxBuffer: 10 * 1024 * 1024, // 10 Mb of output can be captured. shell: true, detached: false, - windowsHide: true + windowsHide: true, + timeout: timeout }; - const child = cp.spawn(command, [], spawnOptions); - outputChannel.show(); + try { + const child = cp.spawn(command, [], spawnOptions); + this._outputChannel.show(); - // Add listeners to print stdout and stderr and exit code - void child.on('exit', (code: number | null, signal: string | null) => { - if (code !== null) { - outputChannel.appendLine(localize('sqlDatabaseProjects.RunStreamedCommand.ExitedWithCode', " >>> {0} … exited with code: {1}", command, code)); - } else { - outputChannel.appendLine(localize('sqlDatabaseProjects.RunStreamedCommand.ExitedWithSignal', " >>> {0} … exited with signal: {1}", command, signal)); - } - }); + // Add listeners to print stdout and stderr and exit code + void child.on('exit', (code: number | null, signal: string | null) => { + if (code !== null) { + this._outputChannel.appendLine(localize('sqlDatabaseProjects.RunStreamedCommand.ExitedWithCode', " >>> {0} … exited with code: {1}", command, code)); + } else { + this._outputChannel.appendLine(localize('sqlDatabaseProjects.RunStreamedCommand.ExitedWithSignal', " >>> {0} … exited with signal: {1}", command, signal)); + } + }); - child.stdout!.on('data', (data: string | Buffer) => { - stdoutData.push(data.toString()); - this.outputDataChunk(data, outputChannel, localize('sqlDatabaseProjects.RunCommand.stdout', " stdout: ")); - }); + child.stdout!.on('data', (data: string | Buffer) => { + stdoutData.push(data.toString()); + ShellExecutionHelper.outputDataChunk(this._outputChannel, data, localize('sqlDatabaseProjects.RunCommand.stdout', " stdout: ")); + }); - child.stderr!.on('data', (data: string | Buffer) => { - this.outputDataChunk(data, outputChannel, localize('sqlDatabaseProjects.RunCommand.stderr', " stderr: ")); - }); + child.stderr!.on('data', (data: string | Buffer) => { + ShellExecutionHelper.outputDataChunk(this._outputChannel, data, localize('sqlDatabaseProjects.RunCommand.stderr', " stderr: ")); + }); - await child; + await child; - return stdoutData.join(''); + return stdoutData.join(''); + } + catch (err) { + // removing sensitive data from the exception + sensitiveData.forEach(element => { + err.cmd = err.cmd?.replace(element, '***'); + err.message = err.message?.replace(element, '***'); + }); + + throw err; + } } - private outputDataChunk(data: string | Buffer, outputChannel: vscode.OutputChannel, header: string): void { + private static outputDataChunk(outputChannel: vscode.OutputChannel, data: string | Buffer, header: string): void { data.toString().split(/\r?\n/) .forEach(line => { - outputChannel.appendLine(header + line); + if (outputChannel) { + outputChannel.appendLine(header + line); + } }); } }