From e7056ed2eeeb912fc6214b96b747cdc2bda1a607 Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Mon, 21 Nov 2022 16:58:57 -0800 Subject: [PATCH 1/7] (src/clients): added error handling --- src/clients.ts | 35 +++++++++++++++++++++++++++-------- src/update-secrets.ts | 2 +- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/clients.ts b/src/clients.ts index 0fe1da01..3b56c058 100644 --- a/src/clients.ts +++ b/src/clients.ts @@ -44,8 +44,11 @@ function log(text: string = '') { } function getRepositoryName(): string { - const output = spawnSync('gh', ['repo', 'view', '--json', 'nameWithOwner'], { stdio: ['ignore', 'pipe', 'inherit'] }).stdout.toString('utf-8'); - + const spawn = spawnSync('gh', ['repo', 'view', '--json', 'nameWithOwner'], { stdio: ['ignore', 'pipe', 'inherit'] }); + if (spawn.stderr) { + throw new Error(`Failed to get repository name: ${spawn.stderr}`); + } + const output = spawn.stdout.toString('utf-8'); try { const repo = JSON.parse(output); return repo.nameWithOwner; @@ -56,18 +59,27 @@ function getRepositoryName(): string { function storeSecret(repository: string, name: string, value: string): void { const args = ['secret', 'set', '--repo', repository, name]; - spawnSync('gh', args, { input: value, stdio: ['pipe', 'inherit', 'inherit'] }); + const spawn = spawnSync('gh', args, { input: value, stdio: ['pipe', 'inherit', 'pipe'] }); + if (spawn.stderr) { + throw new Error(`Failed to store secret '${name}' in repository '${repository}': ${spawn.stderr}`); + } } function listSecrets(repository: string): string[] { const args = ['secret', 'list', '--repo', repository]; - const stdout = spawnSync('gh', args, { stdio: ['ignore', 'pipe', 'inherit'] }).stdout.toString('utf-8').trim(); - return stdout.split('\n').map(line => line.split('\t')[0]); + const spawn = spawnSync('gh', args, { stdio: ['ignore', 'pipe', 'pipe'] }); + if (spawn.stderr) { + throw new Error(`Failed to list secrets in repository '${repository}': ${spawn.stderr}`); + } + return spawn.stdout.toString('utf-8').trim().split('\n').map(line => line.split('\t')[0]); } function removeSecret(repository: string, key: string): void { const args = ['secret', 'remove', '--repo', repository, key]; - spawnSync('gh', args, { stdio: ['ignore', 'inherit', 'inherit'] }); + const spawn = spawnSync('gh', args, { stdio: ['ignore', 'inherit', 'pipe'] }); + if (spawn.stderr) { + throw new Error(`Failed to remove secret '${key}' from repository '${repository}': ${spawn.stderr}`); + } } function confirmPrompt(): Promise { @@ -96,7 +108,14 @@ async function getSecret(secretId: string, options: SecretOptions = {}): Promise customUserAgent: `${PKG.name}/${PKG.version}`, }); - const result = await client.getSecretValue({ SecretId: secretId }).promise(); + let result; + try { + result = await client.getSecretValue({ SecretId: secretId }).promise(); + } catch (error) { + throw new Error(`Failed to retrieve secret '${secretId}' from SecretsManager: ${error}`); + } + + let json; try { json = JSON.parse(result.SecretString!); @@ -113,4 +132,4 @@ async function getSecret(secretId: string, options: SecretOptions = {}): Promise export interface Secret { readonly json: Record; readonly arn: string; -} \ No newline at end of file +} diff --git a/src/update-secrets.ts b/src/update-secrets.ts index fc153d83..545b7eb4 100644 --- a/src/update-secrets.ts +++ b/src/update-secrets.ts @@ -124,7 +124,7 @@ export async function updateSecrets(options: UpdateSecretsOptions) { c.log(`FROM : ${secret.arn}`); c.log(`REPO : ${repository}`); - c.log(`UDPATE: ${keys.join(',')}`); + c.log(`UPDATE: ${keys.join(',')}`); if (pruneCandidates.length > 0) { if (prune) { From 940d926d51abbbbda1447d692efff24824e60e37 Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Mon, 21 Nov 2022 17:08:37 -0800 Subject: [PATCH 2/7] =?UTF-8?q?missed=20a=20=E2=80=98pipe=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/clients.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clients.ts b/src/clients.ts index 3b56c058..8d48dd5a 100644 --- a/src/clients.ts +++ b/src/clients.ts @@ -44,7 +44,7 @@ function log(text: string = '') { } function getRepositoryName(): string { - const spawn = spawnSync('gh', ['repo', 'view', '--json', 'nameWithOwner'], { stdio: ['ignore', 'pipe', 'inherit'] }); + const spawn = spawnSync('gh', ['repo', 'view', '--json', 'nameWithOwner'], { stdio: ['ignore', 'pipe', 'pipe'] }); if (spawn.stderr) { throw new Error(`Failed to get repository name: ${spawn.stderr}`); } From 8c92344fa782f043158d84701b8ac3233c535567 Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Mon, 21 Nov 2022 17:11:03 -0800 Subject: [PATCH 3/7] spacing --- src/clients.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/clients.ts b/src/clients.ts index 8d48dd5a..e7be65d2 100644 --- a/src/clients.ts +++ b/src/clients.ts @@ -115,7 +115,6 @@ async function getSecret(secretId: string, options: SecretOptions = {}): Promise throw new Error(`Failed to retrieve secret '${secretId}' from SecretsManager: ${error}`); } - let json; try { json = JSON.parse(result.SecretString!); From 878eec9b97c52a78bb99dc0b7422746dbf6b8efd Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Mon, 21 Nov 2022 19:36:46 -0800 Subject: [PATCH 4/7] tests --- src/clients.ts | 20 ++++++++++++-------- test/clients.test.ts | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 test/clients.test.ts diff --git a/src/clients.ts b/src/clients.ts index e7be65d2..a9a4d16d 100644 --- a/src/clients.ts +++ b/src/clients.ts @@ -45,8 +45,9 @@ function log(text: string = '') { function getRepositoryName(): string { const spawn = spawnSync('gh', ['repo', 'view', '--json', 'nameWithOwner'], { stdio: ['ignore', 'pipe', 'pipe'] }); - if (spawn.stderr) { - throw new Error(`Failed to get repository name: ${spawn.stderr}`); + const stderr = spawn.stderr?.toString(); + if (stderr) { + throw new Error(`Failed to get repository name: ${stderr}`); } const output = spawn.stdout.toString('utf-8'); try { @@ -60,16 +61,18 @@ function getRepositoryName(): string { function storeSecret(repository: string, name: string, value: string): void { const args = ['secret', 'set', '--repo', repository, name]; const spawn = spawnSync('gh', args, { input: value, stdio: ['pipe', 'inherit', 'pipe'] }); - if (spawn.stderr) { - throw new Error(`Failed to store secret '${name}' in repository '${repository}': ${spawn.stderr}`); + const stderr = spawn.stderr?.toString(); + if (stderr) { + throw new Error(`Failed to store secret '${name}' in repository '${repository}': ${stderr}`); } } function listSecrets(repository: string): string[] { const args = ['secret', 'list', '--repo', repository]; const spawn = spawnSync('gh', args, { stdio: ['ignore', 'pipe', 'pipe'] }); - if (spawn.stderr) { - throw new Error(`Failed to list secrets in repository '${repository}': ${spawn.stderr}`); + const stderr = spawn.stderr?.toString(); + if (stderr) { + throw new Error(`Failed to list secrets in repository '${repository}': ${stderr}`); } return spawn.stdout.toString('utf-8').trim().split('\n').map(line => line.split('\t')[0]); } @@ -77,8 +80,9 @@ function listSecrets(repository: string): string[] { function removeSecret(repository: string, key: string): void { const args = ['secret', 'remove', '--repo', repository, key]; const spawn = spawnSync('gh', args, { stdio: ['ignore', 'inherit', 'pipe'] }); - if (spawn.stderr) { - throw new Error(`Failed to remove secret '${key}' from repository '${repository}': ${spawn.stderr}`); + const stderr = spawn.stderr?.toString(); + if (stderr) { + throw new Error(`Failed to remove secret '${key}' from repository '${repository}': ${stderr}`); } } diff --git a/test/clients.test.ts b/test/clients.test.ts new file mode 100644 index 00000000..dbe45f23 --- /dev/null +++ b/test/clients.test.ts @@ -0,0 +1,38 @@ +import { DEFAULTS as clients } from '../src/clients'; + +jest.mock('child_process', () => { + const actual = jest.requireActual('child_process'); + return { + spawnSync: (_a: any, _b: any, options: any) => { + return actual.spawnSync('node', ['--eval', "throw new Error('Nope');"], options); + }, + }; +}); + +afterEach(() => { + jest.resetAllMocks(); +}); + +test('throws error when, getRepositoryName() sub process fails', async () => { + expect( + () => clients.getRepositoryName(), + ).toThrowError(/Failed to get repository name: .*/); +}); + +test('throws error when, storeSecret() sub process fails', async () => { + expect( + () => clients.storeSecret('repo', 'name', 'value'), + ).toThrowError(/Failed to store secret 'name' in repository 'repo': .*/); +}); + +test('throws error when, listSecrets() sub process fails', async () => { + expect( + () => clients.listSecrets('repo'), + ).toThrowError(/Failed to list secrets in repository 'repo': .*/); +}); + +test('throws error when, removeSecret() sub process fails', async () => { + expect( + () => clients.removeSecret('repo', 'key'), + ).toThrowError(/Failed to remove secret 'key' from repository 'repo': .*/); +}); From 217ebcd3b91f6a5cc2fcae12a9db6aa476ac3de4 Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Tue, 22 Nov 2022 09:17:11 -0800 Subject: [PATCH 5/7] grammar! --- test/clients.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/clients.test.ts b/test/clients.test.ts index dbe45f23..8b5fa84e 100644 --- a/test/clients.test.ts +++ b/test/clients.test.ts @@ -13,25 +13,25 @@ afterEach(() => { jest.resetAllMocks(); }); -test('throws error when, getRepositoryName() sub process fails', async () => { +test('throws error, when getRepositoryName() sub process fails', async () => { expect( () => clients.getRepositoryName(), ).toThrowError(/Failed to get repository name: .*/); }); -test('throws error when, storeSecret() sub process fails', async () => { +test('throws error, when storeSecret() sub process fails', async () => { expect( () => clients.storeSecret('repo', 'name', 'value'), ).toThrowError(/Failed to store secret 'name' in repository 'repo': .*/); }); -test('throws error when, listSecrets() sub process fails', async () => { +test('throws error, when listSecrets() sub process fails', async () => { expect( () => clients.listSecrets('repo'), ).toThrowError(/Failed to list secrets in repository 'repo': .*/); }); -test('throws error when, removeSecret() sub process fails', async () => { +test('throws error, when removeSecret() sub process fails', async () => { expect( () => clients.removeSecret('repo', 'key'), ).toThrowError(/Failed to remove secret 'key' from repository 'repo': .*/); From 7d82724ad475fdec197bb47b6a9f3b867ab23eb1 Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Tue, 22 Nov 2022 14:37:23 -0800 Subject: [PATCH 6/7] throw if process exits non 0, added test for .getSecret() --- src/clients.ts | 20 +++++------ test/clients.test.ts | 86 +++++++++++++++++++++++++++++--------------- 2 files changed, 65 insertions(+), 41 deletions(-) diff --git a/src/clients.ts b/src/clients.ts index a9a4d16d..84ef41c1 100644 --- a/src/clients.ts +++ b/src/clients.ts @@ -45,9 +45,8 @@ function log(text: string = '') { function getRepositoryName(): string { const spawn = spawnSync('gh', ['repo', 'view', '--json', 'nameWithOwner'], { stdio: ['ignore', 'pipe', 'pipe'] }); - const stderr = spawn.stderr?.toString(); - if (stderr) { - throw new Error(`Failed to get repository name: ${stderr}`); + if (spawn.status !== 0) { + throw new Error(`Failed to get repository name: ${spawn.stderr?.toString()}`); } const output = spawn.stdout.toString('utf-8'); try { @@ -61,18 +60,16 @@ function getRepositoryName(): string { function storeSecret(repository: string, name: string, value: string): void { const args = ['secret', 'set', '--repo', repository, name]; const spawn = spawnSync('gh', args, { input: value, stdio: ['pipe', 'inherit', 'pipe'] }); - const stderr = spawn.stderr?.toString(); - if (stderr) { - throw new Error(`Failed to store secret '${name}' in repository '${repository}': ${stderr}`); + if (spawn.status !== 0) { + throw new Error(`Failed to store secret '${name}' in repository '${repository}': ${spawn.stderr?.toString()}`); } } function listSecrets(repository: string): string[] { const args = ['secret', 'list', '--repo', repository]; const spawn = spawnSync('gh', args, { stdio: ['ignore', 'pipe', 'pipe'] }); - const stderr = spawn.stderr?.toString(); - if (stderr) { - throw new Error(`Failed to list secrets in repository '${repository}': ${stderr}`); + if (spawn.status !== 0) { + throw new Error(`Failed to list secrets in repository '${repository}': ${spawn.stderr?.toString()}`); } return spawn.stdout.toString('utf-8').trim().split('\n').map(line => line.split('\t')[0]); } @@ -80,9 +77,8 @@ function listSecrets(repository: string): string[] { function removeSecret(repository: string, key: string): void { const args = ['secret', 'remove', '--repo', repository, key]; const spawn = spawnSync('gh', args, { stdio: ['ignore', 'inherit', 'pipe'] }); - const stderr = spawn.stderr?.toString(); - if (stderr) { - throw new Error(`Failed to remove secret '${key}' from repository '${repository}': ${stderr}`); + if (spawn.status !== 0) { + throw new Error(`Failed to remove secret '${key}' from repository '${repository}': ${spawn.stderr?.toString()}`); } } diff --git a/test/clients.test.ts b/test/clients.test.ts index 8b5fa84e..11c54883 100644 --- a/test/clients.test.ts +++ b/test/clients.test.ts @@ -1,38 +1,66 @@ +import { spawnSync } from 'child_process'; +import { SecretsManager } from 'aws-sdk'; import { DEFAULTS as clients } from '../src/clients'; -jest.mock('child_process', () => { - const actual = jest.requireActual('child_process'); - return { - spawnSync: (_a: any, _b: any, options: any) => { +jest.mock('child_process', () => ({ + spawnSync: jest.fn(), +})); +jest.mock('aws-sdk', () => ({ + SecretsManager: jest.fn(), +})); + + +describe('Error handling', () => { + beforeEach(() => { + // @ts-expect-error We've mocked this submodule at top of file. + spawnSync.mockImplementation(function (_a: any, _b: any, options: any) { + const actual = jest.requireActual('child_process'); return actual.spawnSync('node', ['--eval', "throw new Error('Nope');"], options); - }, - }; -}); + }); -afterEach(() => { - jest.resetAllMocks(); -}); + // @ts-expect-error We've mocked this submodule at top of file. + SecretsManager.mockImplementation(function () { + return { + getSecretValue: function () { + return { + promise: async () => Promise.reject(new Error('Nope')), + }; + }, + }; + }); + }); -test('throws error, when getRepositoryName() sub process fails', async () => { - expect( - () => clients.getRepositoryName(), - ).toThrowError(/Failed to get repository name: .*/); -}); + afterEach(() => { + jest.resetAllMocks(); + }); -test('throws error, when storeSecret() sub process fails', async () => { - expect( - () => clients.storeSecret('repo', 'name', 'value'), - ).toThrowError(/Failed to store secret 'name' in repository 'repo': .*/); -}); + test('throws error, when .getRepositoryName() sub-process throws an error', () => { + expect( + () => clients.getRepositoryName(), + ).toThrowError('Failed to get repository name'); + }); -test('throws error, when listSecrets() sub process fails', async () => { - expect( - () => clients.listSecrets('repo'), - ).toThrowError(/Failed to list secrets in repository 'repo': .*/); -}); + test('throws error, when .storeSecret() sub-process throws an error', () => { + expect( + () => clients.storeSecret('repo', 'name', 'value'), + ).toThrowError("Failed to store secret 'name' in repository 'repo'"); + }); + + test('throws error, when .listSecrets() sub-process throws an error', () => { + expect( + () => clients.listSecrets('repo'), + ).toThrowError("Failed to list secrets in repository 'repo'"); + }); + + test('throws error, when .removeSecret() sub-process throws an error', () => { + expect( + () => clients.removeSecret('repo', 'key'), + ).toThrowError("Failed to remove secret 'key' from repository 'repo'"); + }); -test('throws error, when removeSecret() sub process fails', async () => { - expect( - () => clients.removeSecret('repo', 'key'), - ).toThrowError(/Failed to remove secret 'key' from repository 'repo': .*/); + test('throws error, when SecretsManager.removeSecret() throws an error', async () => { + return expect(async () => { + return clients.getSecret('secretId'); + }).rejects.toThrow("Failed to retrieve secret 'secretId' from SecretsManager: Error: Nope"); + }); }); From 2df810c3c3a1c2aefb73a60567e0ae996dfe1797 Mon Sep 17 00:00:00 2001 From: Ryan Parker Date: Tue, 22 Nov 2022 14:42:04 -0800 Subject: [PATCH 7/7] typo in test --- test/clients.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/clients.test.ts b/test/clients.test.ts index 11c54883..b4fa168d 100644 --- a/test/clients.test.ts +++ b/test/clients.test.ts @@ -58,7 +58,7 @@ describe('Error handling', () => { ).toThrowError("Failed to remove secret 'key' from repository 'repo'"); }); - test('throws error, when SecretsManager.removeSecret() throws an error', async () => { + test('throws error, when SecretsManager.getSecretValue() throws an error', async () => { return expect(async () => { return clients.getSecret('secretId'); }).rejects.toThrow("Failed to retrieve secret 'secretId' from SecretsManager: Error: Nope");