From 1a6c8aa79612d1282055b2b90066317931e9f52a Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 10:36:53 +0100 Subject: [PATCH 1/7] Error handling for request with proper timeouts --- packages/cdktf-cli/lib/checkpoint.ts | 12 ++++---- packages/cdktf-cli/package.json | 17 +++++------ packages/cdktf-cli/test/checkpoint.test.ts | 33 ++++++++++++++++++++++ yarn.lock | 22 +++++++++++++++ 4 files changed, 70 insertions(+), 14 deletions(-) create mode 100644 packages/cdktf-cli/test/checkpoint.test.ts diff --git a/packages/cdktf-cli/lib/checkpoint.ts b/packages/cdktf-cli/lib/checkpoint.ts index 8012edf811..8a5b2ca286 100644 --- a/packages/cdktf-cli/lib/checkpoint.ts +++ b/packages/cdktf-cli/lib/checkpoint.ts @@ -28,7 +28,7 @@ async function post(url: string, data: string) { 'Content-Length': data.length, 'User-Agent': 'HashiCorp/cdktf-cli' }, - method: 'POST' + method: 'POST', }, res => { if (res.statusCode) { const statusCode = res.statusCode; @@ -38,16 +38,16 @@ async function post(url: string, data: string) { } const data = new Array(); res.on('data', chunk => data.push(chunk)); - - res.once('error', err => ko(err)); - res.once('end', () => { + res.on('error', err => ko(err)); + res.on('end', () => { return ok(); }); }); + req.setTimeout(1000, () => ko((new Error('request timeout')))); req.write(data); - req.end(); + req.on('error', err => ko(err)); }) } @@ -82,7 +82,7 @@ export async function ReportRequest(reportParams: ReportParams): Promise { processLogger(e.message) } -} +} diff --git a/packages/cdktf-cli/package.json b/packages/cdktf-cli/package.json index 32104bd115..13ca4a922e 100644 --- a/packages/cdktf-cli/package.json +++ b/packages/cdktf-cli/package.json @@ -27,17 +27,12 @@ }, "license": "MPL-2.0", "dependencies": { - "@types/archiver": "^5.1.0", - "@types/node": "^14.0.26", - "@types/readline-sync": "^1.4.3", - "@types/stream-buffers": "^3.0.3", - "@types/uuid": "^8.3.0", + "@skorfmann/terraform-cloud": "^1.7.1", "archiver": "^5.1.0", "cdktf": "0.0.0", "chalk": "^4.1.0", "codemaker": "^0.22.0", "constructs": "^3.0.0", - "eslint-plugin-react": "^7.20.0", "fs-extra": "^8.1.0", "ink": "^2.7.1", "ink-confirm-input": "^2.0.0", @@ -50,7 +45,6 @@ "semver": "^7.3.2", "sscaff": "^1.2.0", "stream-buffers": "^3.0.2", - "@skorfmann/terraform-cloud": "^1.7.1", "uuid": "^8.3.0", "yargs": "^15.1.0" }, @@ -90,7 +84,14 @@ "eslint": "^6.8.0", "ink-testing-library": "^2.0.0", "jest": "^26.6.3", + "nock": "^13.0.7", "ts-jest": "^26.4.4", - "typescript": "^3.9.7" + "typescript": "^3.9.7", + "@types/nock": "^11.1.0", + "@types/archiver": "^5.1.0", + "@types/readline-sync": "^1.4.3", + "@types/stream-buffers": "^3.0.3", + "@types/uuid": "^8.3.0", + "eslint-plugin-react": "^7.20.0" } } diff --git a/packages/cdktf-cli/test/checkpoint.test.ts b/packages/cdktf-cli/test/checkpoint.test.ts new file mode 100644 index 0000000000..95fbe27a29 --- /dev/null +++ b/packages/cdktf-cli/test/checkpoint.test.ts @@ -0,0 +1,33 @@ +jest.mock('../lib/logging', () => ( + { + ...(jest.requireActual('../lib/logging') as {}), + processLogger: jest.fn() + } +)) +import { processLogger } from '../lib/logging' +import { ReportRequest, ReportParams } from '../lib/checkpoint'; +import nock from 'nock'; + + +describe('ReportRequest', () => { + const reportParams: ReportParams = { command: 'foo', product: 'cdktf', version: '0.1', dateTime: new Date(), payload: {}, language: 'typescript' }; + + it('handles request errors', async () => { + nock('https://checkpoint-api.hashicorp.com') + .post(new RegExp('/v1/.*')) + .replyWithError('some request error happened'); + + await ReportRequest(reportParams) + }); + + it('handles timeouts', async () => { + nock('https://checkpoint-api.hashicorp.com') + .post(new RegExp('/v1/.*')) + .delayConnection(1001) + .reply(201, '') + + + await ReportRequest(reportParams) + expect(processLogger).toBeCalledWith('request timeout') + }); +}); diff --git a/yarn.lock b/yarn.lock index 3e27566d4a..26dae7c43d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1597,6 +1597,13 @@ dependencies: "@types/node" "*" +"@types/nock@^11.1.0": + version "11.1.0" + resolved "https://registry.yarnpkg.com/@types/nock/-/nock-11.1.0.tgz#0a8c1056a31ba32a959843abccf99626dd90a538" + integrity sha512-jI/ewavBQ7X5178262JQR0ewicPAcJhXS/iFaNJl0VHLfyosZ/kwSrsa6VNQNSO8i9d8SqdRgOtZSOKJ/+iNMw== + dependencies: + nock "*" + "@types/node@*", "@types/node@>= 8": version "14.0.13" resolved "https://registry.yarnpkg.com/@types/node/-/node-14.0.13.tgz#ee1128e881b874c371374c1f72201893616417c9" @@ -6280,6 +6287,16 @@ nice-try@^1.0.4: resolved "https://registry.yarnpkg.com/nice-try/-/nice-try-1.0.5.tgz#a3378a7696ce7d223e88fc9b764bd7ef1089e366" integrity sha512-1nh45deeb5olNY7eX82BkPO7SSxR5SSYJiPTrTdFUVYwAl8CKMA5N9PjTYkHiRjisVcxcQ1HXdLhx2qxxJzLNQ== +nock@*, nock@^13.0.7: + version "13.0.7" + resolved "https://registry.yarnpkg.com/nock/-/nock-13.0.7.tgz#9bc718c66bd0862dfa14601a9ba678a406127910" + integrity sha512-WBz73VYIjdbO6BwmXODRQLtn7B5tldA9pNpWJe5QTtTEscQlY5KXU4srnGzBOK2fWakkXj69gfTnXGzmrsaRWw== + dependencies: + debug "^4.1.0" + json-stringify-safe "^5.0.1" + lodash.set "^4.3.2" + propagate "^2.0.0" + node-fetch-npm@^2.0.2: version "2.0.4" resolved "https://registry.yarnpkg.com/node-fetch-npm/-/node-fetch-npm-2.0.4.tgz#6507d0e17a9ec0be3bec516958a497cec54bf5a4" @@ -7016,6 +7033,11 @@ prop-types@^15.5.10, prop-types@^15.6.2, prop-types@^15.7.2: object-assign "^4.1.1" react-is "^16.8.1" +propagate@^2.0.0: + version "2.0.1" + resolved "https://registry.yarnpkg.com/propagate/-/propagate-2.0.1.tgz#40cdedab18085c792334e64f0ac17256d38f9a45" + integrity sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag== + proto-list@~1.2.1: version "1.2.4" resolved "https://registry.yarnpkg.com/proto-list/-/proto-list-1.2.4.tgz#212d5bfe1318306a420f6402b8e26ff39647a849" From 475e5a1574ec81649f4d27e00c96f8856294824e Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 10:38:15 +0100 Subject: [PATCH 2/7] Delete unused code --- packages/cdktf-cli/lib/util.ts | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/packages/cdktf-cli/lib/util.ts b/packages/cdktf-cli/lib/util.ts index eb0a7e0ca3..4d7fab7e5a 100644 --- a/packages/cdktf-cli/lib/util.ts +++ b/packages/cdktf-cli/lib/util.ts @@ -1,5 +1,3 @@ -import * as http from 'http'; -import * as https from 'https'; import { spawn, SpawnOptions } from 'child_process'; import * as fs from 'fs-extra'; import * as os from 'os'; @@ -35,31 +33,6 @@ export async function mkdtemp(closure: (dir: string) => Promise) { } } -async function get(url: string, protocol: typeof http | typeof https = https): Promise { - return new Promise((ok, ko) => { - const req = protocol.get(url, res => { - if (res.statusCode !== 200) { - throw new Error(`${res.statusMessage}: ${url}`); - } - const data = new Array(); - res.on('data', chunk => data.push(chunk)); - res.once('end', () => ok(Buffer.concat(data).toString('utf-8'))); - res.once('error', ko); - }); - - req.once('error', ko); - req.end(); - }); -} - -export async function httpGet(url: string): Promise { - return get(url, http) -} - -export async function httpsGet(url: string): Promise { - return get(url) -} - export const exec = async (command: string, args: string[], options: SpawnOptions, stdout?: (chunk: Buffer) => any): Promise => { return new Promise((ok, ko) => { const child = spawn(command, args, options); From cb6f1974bf7bebea32329b73c513f53520ce2a05 Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 11:19:22 +0100 Subject: [PATCH 3/7] More explicit mock --- packages/cdktf-cli/test/checkpoint.test.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/cdktf-cli/test/checkpoint.test.ts b/packages/cdktf-cli/test/checkpoint.test.ts index 95fbe27a29..c517e877a3 100644 --- a/packages/cdktf-cli/test/checkpoint.test.ts +++ b/packages/cdktf-cli/test/checkpoint.test.ts @@ -1,14 +1,14 @@ +const mockedLogger = jest.fn() jest.mock('../lib/logging', () => ( { ...(jest.requireActual('../lib/logging') as {}), - processLogger: jest.fn() + processLogger: mockedLogger } )) -import { processLogger } from '../lib/logging' + import { ReportRequest, ReportParams } from '../lib/checkpoint'; import nock from 'nock'; - describe('ReportRequest', () => { const reportParams: ReportParams = { command: 'foo', product: 'cdktf', version: '0.1', dateTime: new Date(), payload: {}, language: 'typescript' }; @@ -21,13 +21,14 @@ describe('ReportRequest', () => { }); it('handles timeouts', async () => { + mockedLogger.mockReset() + nock('https://checkpoint-api.hashicorp.com') .post(new RegExp('/v1/.*')) - .delayConnection(1001) - .reply(201, '') - + .delayConnection(1010) + .reply() await ReportRequest(reportParams) - expect(processLogger).toBeCalledWith('request timeout') + expect(mockedLogger).toBeCalledWith('request timeout') }); }); From 2d03552745cb682f22365e94d4b38137b2a39d97 Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 11:20:03 +0100 Subject: [PATCH 4/7] Limit test run to actual packages --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index aca211752d..2b0777f71e 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "examples:build": "lerna run --parallel --scope @examples/* --ignore @examples/java* build && lerna run --stream --scope @examples/java* build --concurrency 1", "examples:synth": "lerna run --parallel --scope @examples/* --ignore @examples/java* synth && lerna run --stream --scope @examples/java* synth --concurrency 1", "examples:integration": "test/run-against-dist yarn examples:reinstall && yarn examples:build && yarn examples:synth", - "test": "lerna run test", + "test": "lerna run --scope cdktf* test", "watch": "lerna run --parallel --stream --scope cdktf* watch-preserve-output", "link-packages": "lerna exec --scope cdktf* yarn link", "integration": "test/run-against-dist test/test-all.sh", From 64b3b1a528285204a82345e2a58cb0781f97ec71 Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 11:43:38 +0100 Subject: [PATCH 5/7] Drop mock test --- packages/cdktf-cli/test/checkpoint.test.ts | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/packages/cdktf-cli/test/checkpoint.test.ts b/packages/cdktf-cli/test/checkpoint.test.ts index c517e877a3..208ead07c3 100644 --- a/packages/cdktf-cli/test/checkpoint.test.ts +++ b/packages/cdktf-cli/test/checkpoint.test.ts @@ -1,11 +1,3 @@ -const mockedLogger = jest.fn() -jest.mock('../lib/logging', () => ( - { - ...(jest.requireActual('../lib/logging') as {}), - processLogger: mockedLogger - } -)) - import { ReportRequest, ReportParams } from '../lib/checkpoint'; import nock from 'nock'; @@ -19,16 +11,4 @@ describe('ReportRequest', () => { await ReportRequest(reportParams) }); - - it('handles timeouts', async () => { - mockedLogger.mockReset() - - nock('https://checkpoint-api.hashicorp.com') - .post(new RegExp('/v1/.*')) - .delayConnection(1010) - .reply() - - await ReportRequest(reportParams) - expect(mockedLogger).toBeCalledWith('request timeout') - }); }); From 593a2c207bbf4305b2a9f5e2c77be28120d97423 Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 12:52:11 +0100 Subject: [PATCH 6/7] `@types/node` is used for generating module / provider bindings --- packages/cdktf-cli/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cdktf-cli/package.json b/packages/cdktf-cli/package.json index 13ca4a922e..232430b4e3 100644 --- a/packages/cdktf-cli/package.json +++ b/packages/cdktf-cli/package.json @@ -27,6 +27,7 @@ }, "license": "MPL-2.0", "dependencies": { + "@types/node": "^14.0.26", "@skorfmann/terraform-cloud": "^1.7.1", "archiver": "^5.1.0", "cdktf": "0.0.0", @@ -76,7 +77,6 @@ "@types/ink-spinner": "^3.0.0", "@types/jest": "^25.1.2", "@types/json-schema": "^7.0.4", - "@types/node": "^14.0.26", "@types/react": "^16.9.35", "@types/semver": "^7.1.0", "@typescript-eslint/eslint-plugin": "^2.20.0", From ea8017ad3c7450d5677785562a286e814a10889f Mon Sep 17 00:00:00 2001 From: Sebastian Korfmann Date: Mon, 8 Feb 2021 16:19:18 +0100 Subject: [PATCH 7/7] Adds unit test for CHECKPOINT_DISABLE This replaces an integration test removed in 74b21865b --- packages/cdktf-cli/test/checkpoint.test.ts | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/cdktf-cli/test/checkpoint.test.ts b/packages/cdktf-cli/test/checkpoint.test.ts index 208ead07c3..c66b99d9f0 100644 --- a/packages/cdktf-cli/test/checkpoint.test.ts +++ b/packages/cdktf-cli/test/checkpoint.test.ts @@ -11,4 +11,37 @@ describe('ReportRequest', () => { await ReportRequest(reportParams) }); + + describe('CHECKPOINT_DISABLE', () => { + let checkPointDisable: any; + + beforeEach(() => { + checkPointDisable = process.env.CHECKPOINT_DISABLE + }) + + afterEach(() => { + process.env.CHECKPOINT_DISABLE = checkPointDisable; + }) + + it('does not perform request when disabled via ENV', async () => { + process.env.CHECKPOINT_DISABLE = 'truthy' + + const scope = nock('https://checkpoint-api.hashicorp.com') + .post(new RegExp('/v1/.*')) + .reply() + + await ReportRequest(reportParams) + expect(scope.isDone()).toBeFalsy() + }) + + it('does perform request by default', async () => { + delete process.env.CHECKPOINT_DISABLE + const scope = nock('https://checkpoint-api.hashicorp.com') + .post(new RegExp('/v1/*')) + .reply(201, '') + + await ReportRequest(reportParams) + expect(scope.isDone).toBeTruthy() + }) + }) });