From 8ebb8f7275e53d40bf14cc95b4af3a191f0af086 Mon Sep 17 00:00:00 2001 From: Jan Richter Date: Wed, 14 Nov 2018 10:33:34 +0100 Subject: [PATCH 1/6] Propagate errors from odo --- src/odo.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/odo.ts b/src/odo.ts index c6d5a364f..f75eb519e 100644 --- a/src/odo.ts +++ b/src/odo.ts @@ -207,7 +207,7 @@ export class OdoImpl implements Odo { return clusters; } - public async getClustersWithOc(): Promise { + private async getClustersWithOc(): Promise { let clusters: OpenShiftObject[] = []; const result: cliInstance.CliExitData = await this.execute(`oc version`); clusters = result.stdout.trim().split('\n').filter((value) => { @@ -219,7 +219,7 @@ export class OdoImpl implements Odo { return clusters; } - public async getClustersWithOdo(): Promise { + private async getClustersWithOdo(): Promise { let clusters: OpenShiftObject[] = []; const result: cliInstance.CliExitData = await this.execute( `odo version && odo project list` @@ -300,7 +300,13 @@ export class OdoImpl implements Odo { return OdoImpl.cli.execute( toolLocation ? command.replace(cmd, `"${toolLocation}"`).replace(new RegExp(`&& ${cmd}`, 'g'), `&& "${toolLocation}"`) : command, cwd ? {cwd} : { } - ); + ).then(async (result) => { + if (result.error) { + return Promise.reject(result.error); + } else { + return result; + } + }); } public async requireLogin(): Promise { From b8e48e79081ac6e0cb621573981cbdd6baa1b9c7 Mon Sep 17 00:00:00 2001 From: Denis Golovin Date: Mon, 26 Nov 2018 23:18:34 -0800 Subject: [PATCH 2/6] Do not download oc again in case of error --- src/cli.ts | 11 ++--------- src/odo.ts | 8 ++++---- src/tools.ts | 18 +++++++----------- src/util/progress.ts | 32 +++++++++++++++++++------------- test/util/progress.test.ts | 14 +++++++++----- 5 files changed, 41 insertions(+), 42 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index a7c8c1525..e4b9d0063 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -36,15 +36,8 @@ export class Cli implements ICli { childProcess.exec(cmd, opts, (error: ExecException, stdout: string, stderr: string) => { this.odoChannel.print(stdout); this.odoChannel.print(stderr); - if (error) { - if (error.code === 1 && stderr.trim().length === 0) { - resolve({ error, stdout: stdout.replace(/---[\s\S]*---/g, '').trim(), stderr }); - } else { - reject(error); - } - } else { - resolve({ error, stdout: stdout.replace(/---[\s\S]*---/g, '').trim(), stderr }); - } + // we don't reject it here, because caller need in some cases access to error and streams + resolve({ error, stdout: stdout.replace(/---[\s\S]*---/g, '').trim(), stderr }); }); }); } diff --git a/src/odo.ts b/src/odo.ts index f75eb519e..85b5a00b3 100644 --- a/src/odo.ts +++ b/src/odo.ts @@ -111,7 +111,7 @@ export interface Odo { getServiceTemplatePlans(svc: string): Promise; getServices(application: OpenShiftObject): Promise; getApplicationChildren(application: OpenShiftObject): Promise; - execute(command: string, cwd?: string): Promise; + execute(command: string, cwd?: string, fail?: boolean): Promise; requireLogin(): Promise; } @@ -209,7 +209,7 @@ export class OdoImpl implements Odo { private async getClustersWithOc(): Promise { let clusters: OpenShiftObject[] = []; - const result: cliInstance.CliExitData = await this.execute(`oc version`); + const result: cliInstance.CliExitData = await this.execute(`oc version`, process.cwd(), false); clusters = result.stdout.trim().split('\n').filter((value) => { return value.indexOf('Server ') !== -1; }).map((value) => { @@ -294,14 +294,14 @@ export class OdoImpl implements Odo { terminal.show(); } - public async execute(command: string, cwd?: string): Promise { + public async execute(command: string, cwd?: string, fail: boolean = true): Promise { const cmd = command.split(' ')[0]; const toolLocation = await ToolsConfig.detectOrDownload(cmd); return OdoImpl.cli.execute( toolLocation ? command.replace(cmd, `"${toolLocation}"`).replace(new RegExp(`&& ${cmd}`, 'g'), `&& "${toolLocation}"`) : command, cwd ? {cwd} : { } ).then(async (result) => { - if (result.error) { + if (result.error && fail) { return Promise.reject(result.error); } else { return result; diff --git a/src/tools.ts b/src/tools.ts index f1207f3db..7d042cbf6 100644 --- a/src/tools.ts +++ b/src/tools.ts @@ -162,18 +162,14 @@ export class ToolsConfig { let detectedVersion: string; if (fs.existsSync(location)) { const version = new RegExp(`${cmd} v([\\d\\.]+)`); - try { - const result = await Cli.getInstance().execute(`"${location}" version`); - if (!result.error) { - const toolVersion: string[] = result.stdout.trim().split('\n').filter((value) => { - return value.match(version); - }).map((value)=>version.exec(value)[1]); - if (toolVersion.length) { - detectedVersion = toolVersion[0]; - } + const result = await Cli.getInstance().execute(`"${location}" version`); + if (result.stdout) { + const toolVersion: string[] = result.stdout.trim().split('\n').filter((value) => { + return value.match(version); + }).map((value)=>version.exec(value)[1]); + if (toolVersion.length) { + detectedVersion = toolVersion[0]; } - } catch (ignore) { - // if `${tool} version` failed, then there is no tool at specified location } } return detectedVersion; diff --git a/src/util/progress.ts b/src/util/progress.ts index b38bf71c6..ffa83909a 100644 --- a/src/util/progress.ts +++ b/src/util/progress.ts @@ -7,6 +7,8 @@ import * as vscode from 'vscode'; import * as odoctl from '../odo'; +import { CliExitData } from '../cli'; +import { ExecException } from 'child_process'; export interface Step { command: string; @@ -20,30 +22,34 @@ export class Progress { (progress: vscode.Progress<{increment: number, message: string}>, token: vscode.CancellationToken) => { const calls: (()=>Promise)[] = []; steps.reduce((previous: Step, current: Step, currentIndex: number, steps: Step[])=> { + current.total = previous.total + current.increment; calls.push(()=> { - let result: Promise = Promise.resolve(); - progress.report({ - increment: previous.increment, - message: `${previous.total}%` - }); - result = odo.execute(current.command); - current.total = previous.total + current.increment; - return currentIndex+1 === steps.length ? result.then(()=> { + return new Promise(async (resolve,reject) => { progress.report({ increment: previous.increment, message: `${previous.total}%` }); - }) : result; + + const result: CliExitData = await odo.execute(current.command); + if(result.error) { + reject(result.error); + } else { + if (currentIndex+1 === steps.length) { + progress.report({ + increment: current.increment, + message: `${current.total}%` + }); + } + resolve(); + } + }); }); return current; }, {increment: 0, command: "", total: 0}); return calls.reduce>((previous: Promise, current: ()=>Promise, index: number, calls: (()=>Promise)[])=> { return previous.then(current); - }, Promise.resolve()).catch((error) => { - vscode.window.showErrorMessage(`${error}`); - return; - }); + }, Promise.resolve()); }); } } \ No newline at end of file diff --git a/test/util/progress.test.ts b/test/util/progress.test.ts index d96011d49..4ccc474aa 100644 --- a/test/util/progress.test.ts +++ b/test/util/progress.test.ts @@ -30,7 +30,6 @@ suite('Progress Utility', () => { setup(() => { sandbox = sinon.createSandbox(); - execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves(); }); teardown(() => { @@ -38,6 +37,7 @@ suite('Progress Utility', () => { }); test('calls cli commands in sequence', async () => { + execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves({ error: undefined, stdout: "", stderr: "" }); await Progress.execWithProgress(options, steps, OdoImpl.getInstance()); expect(execStub).calledTwice; @@ -46,6 +46,7 @@ suite('Progress Utility', () => { }); test('calls progress with given options', async () => { + execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves({ error: undefined, stdout: "", stderr: "" }); const spy = sandbox.spy(vscode.window, 'withProgress'); await Progress.execWithProgress(options, steps, OdoImpl.getInstance()); @@ -53,10 +54,13 @@ suite('Progress Utility', () => { }); test('shows an error message if a command fails', async () => { - execStub.rejects(errorMessage); + let error = new Error(errorMessage); + execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves({ error, stdout: "", stderr: "" }); const spy = sandbox.spy(vscode.window, 'showErrorMessage'); - await Progress.execWithProgress(options, steps, OdoImpl.getInstance()); - - expect(spy).calledOnceWith(errorMessage); + try { + await Progress.execWithProgress(options, steps, OdoImpl.getInstance()); + } catch(err) { + expect(error).equals(error); + } }); }); \ No newline at end of file From a440ddeb88442d74477056738386eeb0a36d74c0 Mon Sep 17 00:00:00 2001 From: Denis Golovin Date: Tue, 27 Nov 2018 01:52:54 -0800 Subject: [PATCH 3/6] Fix comment for the change --- src/cli.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cli.ts b/src/cli.ts index e4b9d0063..3446c04c0 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -36,7 +36,8 @@ export class Cli implements ICli { childProcess.exec(cmd, opts, (error: ExecException, stdout: string, stderr: string) => { this.odoChannel.print(stdout); this.odoChannel.print(stderr); - // we don't reject it here, because caller need in some cases access to error and streams + // do not reject it here, because caller in some cases need the error and the streams + // to make a decision resolve({ error, stdout: stdout.replace(/---[\s\S]*---/g, '').trim(), stderr }); }); }); From dde531afbe20ff5e84e529d21d9682e32d5f87e0 Mon Sep 17 00:00:00 2001 From: Denis Golovin Date: Tue, 27 Nov 2018 02:00:43 -0800 Subject: [PATCH 4/6] Fix test for exception being thrown --- test/util/progress.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/util/progress.test.ts b/test/util/progress.test.ts index 4ccc474aa..525ed6da9 100644 --- a/test/util/progress.test.ts +++ b/test/util/progress.test.ts @@ -53,14 +53,15 @@ suite('Progress Utility', () => { expect(spy).calledOnceWith(options, sinon.match.func); }); - test('shows an error message if a command fails', async () => { + test('throw an error if a command fails', async () => { let error = new Error(errorMessage); execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves({ error, stdout: "", stderr: "" }); const spy = sandbox.spy(vscode.window, 'showErrorMessage'); try { await Progress.execWithProgress(options, steps, OdoImpl.getInstance()); + expect.fail(false,true, 'no errors were detected'); } catch(err) { - expect(error).equals(error); + expect(err).equals(error); } }); }); \ No newline at end of file From 7d93d956a368c3fef067b0e28895f8424296417e Mon Sep 17 00:00:00 2001 From: Denis Golovin Date: Tue, 27 Nov 2018 20:16:35 -0800 Subject: [PATCH 5/6] Fix error flow --- src/util/progress.ts | 40 +++++++++++++++----------------------- test/util/progress.test.ts | 14 ++++++++----- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/src/util/progress.ts b/src/util/progress.ts index ffa83909a..80a994c0d 100644 --- a/src/util/progress.ts +++ b/src/util/progress.ts @@ -19,37 +19,29 @@ export interface Step { export class Progress { static execWithProgress(options, steps: Step[], odo: odoctl.Odo): Thenable { return vscode.window.withProgress(options, - (progress: vscode.Progress<{increment: number, message: string}>, token: vscode.CancellationToken) => { - const calls: (()=>Promise)[] = []; - steps.reduce((previous: Step, current: Step, currentIndex: number, steps: Step[])=> { - current.total = previous.total + current.increment; - calls.push(()=> { - return new Promise(async (resolve,reject) => { - progress.report({ - increment: previous.increment, - message: `${previous.total}%` - }); - - const result: CliExitData = await odo.execute(current.command); - if(result.error) { - reject(result.error); - } else { + (progress: vscode.Progress<{increment: number, message: string}>, token: vscode.CancellationToken) => { + const calls: (()=>Promise)[] = []; + steps.reduce((previous: Step, current: Step, currentIndex: number, steps: Step[])=> { + current.total = previous.total + current.increment; + calls.push (() => { + return Promise.resolve() + .then(() => progress.report({increment: previous.increment, message: `${previous.total}%` })) + .then(() => odo.execute(current.command)) + .then(() => { if (currentIndex+1 === steps.length) { progress.report({ increment: current.increment, message: `${current.total}%` }); } - resolve(); - } + }); }); - }); - return current; - }, {increment: 0, command: "", total: 0}); + return current; + }, {increment: 0, command: "", total: 0}); - return calls.reduce>((previous: Promise, current: ()=>Promise, index: number, calls: (()=>Promise)[])=> { - return previous.then(current); - }, Promise.resolve()); - }); + return calls.reduce>((previous: Promise, current: ()=>Promise, index: number, calls: (()=>Promise)[])=> { + return previous.then(current); + }, Promise.resolve()); + }); } } \ No newline at end of file diff --git a/test/util/progress.test.ts b/test/util/progress.test.ts index 525ed6da9..e1527b414 100644 --- a/test/util/progress.test.ts +++ b/test/util/progress.test.ts @@ -54,14 +54,18 @@ suite('Progress Utility', () => { }); test('throw an error if a command fails', async () => { - let error = new Error(errorMessage); - execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves({ error, stdout: "", stderr: "" }); + const error = new Error(errorMessage); + execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves(error); const spy = sandbox.spy(vscode.window, 'showErrorMessage'); + let e; try { await Progress.execWithProgress(options, steps, OdoImpl.getInstance()); - expect.fail(false,true, 'no errors were detected'); - } catch(err) { - expect(err).equals(error); + } catch (err) { + e = err; + expect(err.message).equals(errorMessage); + } + if (!e) { + expect.fail('no error thrown'); } }); }); \ No newline at end of file From 01c4f84ab124d0d672ac7d514fbc4c22a0a4628b Mon Sep 17 00:00:00 2001 From: Denis Golovin Date: Tue, 27 Nov 2018 20:31:11 -0800 Subject: [PATCH 6/6] Fix test error --- test/util/progress.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/util/progress.test.ts b/test/util/progress.test.ts index e1527b414..b5bb48597 100644 --- a/test/util/progress.test.ts +++ b/test/util/progress.test.ts @@ -55,7 +55,7 @@ suite('Progress Utility', () => { test('throw an error if a command fails', async () => { const error = new Error(errorMessage); - execStub = sandbox.stub(OdoImpl.prototype, 'execute').resolves(error); + execStub = sandbox.stub(OdoImpl.prototype, 'execute').rejects(error); const spy = sandbox.spy(vscode.window, 'showErrorMessage'); let e; try {