From d458083987e28e84418b274909c3f9487a7b6ecd Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Thu, 18 May 2023 16:51:00 -0400 Subject: [PATCH 1/7] feat(cli): logging can be locked --- packages/aws-cdk/lib/cdk-toolkit.ts | 9 +++++- packages/aws-cdk/lib/logging.ts | 15 +++++++++ packages/cdk-assets/lib/publishing.ts | 46 ++++++--------------------- 3 files changed, 32 insertions(+), 38 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 8e56480e9f4c5..d46f9cfa74263 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -17,7 +17,7 @@ import { CloudWatchLogEventMonitor } from './api/logs/logs-monitor'; import { StackActivityProgress } from './api/util/cloudformation/stack-activity-monitor'; import { printSecurityDiff, printStackDiff, RequireApproval } from './diff'; import { ResourceImporter } from './import'; -import { data, debug, error, highlight, print, success, warning } from './logging'; +import { data, debug, error, highlight, print, success, warning, LOG_LOCK } from './logging'; import { deserializeStructure, serializeStructure } from './serialize'; import { Configuration, PROJECT_CONFIG } from './settings'; import { numberFromBool, partition } from './util'; @@ -238,6 +238,8 @@ export class CdkToolkit { if (requireApproval !== RequireApproval.Never) { const currentTemplate = await this.props.deployments.readCurrentTemplate(stack); if (printSecurityDiff(currentTemplate, stack, requireApproval)) { + // Lock the logger from logging temporarily + LOG_LOCK[0] = true; // only talk to user if STDIN is a terminal (otherwise, fail) if (!process.stdin.isTTY) { @@ -254,6 +256,11 @@ export class CdkToolkit { } const confirmed = await promptly.confirm('Do you wish to deploy these changes (y/n)?'); + + LOG_LOCK[0] = false; + // Calls the logger and prints any logs that happened during the lock + print(''); + if (!confirmed) { throw new Error('Aborted by user'); } } } diff --git a/packages/aws-cdk/lib/logging.ts b/packages/aws-cdk/lib/logging.ts index ceade56caed6d..74879054c935f 100644 --- a/packages/aws-cdk/lib/logging.ts +++ b/packages/aws-cdk/lib/logging.ts @@ -7,6 +7,11 @@ const { stdout, stderr } = process; type WritableFactory = () => Writable; +// LOGGER_LOCKED is an array rather than a boolean because it needs to be modified by other +// parts of the CLI and imported variables are always immutable. +export const LOG_LOCK = [false]; +const logBuffer: string[] = []; + const logger = (stream: Writable | WritableFactory, styles?: StyleFn[], timestamp?: boolean) => (fmt: string, ...args: unknown[]) => { const ts = timestamp ? `[${formatTime(new Date())}] ` : ''; @@ -15,8 +20,18 @@ const logger = (stream: Writable | WritableFactory, styles?: StyleFn[], timestam str = styles.reduce((a, style) => style(a), str); } + // Logger is currently locked, so we store the message to be printed + // later when we are unlocked. + if (LOG_LOCK[0]) { + logBuffer.push(str); + return; + } const realStream = typeof stream === 'function' ? stream() : stream; + if (logBuffer.length > 0) { + logBuffer.forEach((l) => realStream.write(l + '\n')); + logBuffer.splice(0); + } realStream.write(str + '\n'); }; diff --git a/packages/cdk-assets/lib/publishing.ts b/packages/cdk-assets/lib/publishing.ts index c35a0254e55ed..9e38308cd7e66 100644 --- a/packages/cdk-assets/lib/publishing.ts +++ b/packages/cdk-assets/lib/publishing.ts @@ -82,9 +82,6 @@ export class AssetPublishing implements IPublishProgress { private readonly publishInParallel: boolean; private readonly buildAssets: boolean; private readonly publishAssets: boolean; - private readonly startMessagePrefix: string; - private readonly successMessagePrefix: string; - private readonly errorMessagePrefix: string; private readonly handlerCache = new Map(); constructor(private readonly manifest: AssetManifest, private readonly options: AssetPublishingOptions) { @@ -94,34 +91,6 @@ export class AssetPublishing implements IPublishProgress { this.buildAssets = options.buildAssets ?? true; this.publishAssets = options.publishAssets ?? true; - const getMessages = () => { - if (this.buildAssets && this.publishAssets) { - return { - startMessagePrefix: 'Building and publishing', - successMessagePrefix: 'Built and published', - errorMessagePrefix: 'Error building and publishing', - }; - } else if (this.buildAssets) { - return { - startMessagePrefix: 'Building', - successMessagePrefix: 'Built', - errorMessagePrefix: 'Error building', - }; - } else { - return { - startMessagePrefix: 'Publishing', - successMessagePrefix: 'Published', - errorMessagePrefix: 'Error publishing', - }; - } - }; - - const messages = getMessages(); - - this.startMessagePrefix = messages.startMessagePrefix; - this.successMessagePrefix = messages.successMessagePrefix; - this.errorMessagePrefix = messages.errorMessagePrefix; - const self = this; this.handlerHost = { aws: this.options.aws, @@ -146,7 +115,7 @@ export class AssetPublishing implements IPublishProgress { } if ((this.options.throwOnError ?? true) && this.failures.length > 0) { - throw new Error(`${this.errorMessagePrefix}: ${this.failures.map(e => e.error.message)}`); + throw new Error(`Error publishing: ${this.failures.map(e => e.error.message)}`); } } @@ -155,7 +124,7 @@ export class AssetPublishing implements IPublishProgress { */ public async buildEntry(asset: IManifestEntry) { try { - if (this.progressEvent(EventType.START, `${this.startMessagePrefix} ${asset.id}`)) { return false; } + if (this.progressEvent(EventType.START, `Building ${asset.id}`)) { return false; } const handler = this.assetHandler(asset); await handler.build(); @@ -163,6 +132,9 @@ export class AssetPublishing implements IPublishProgress { if (this.aborted) { throw new Error('Aborted'); } + + this.completedOperations++; + if (this.progressEvent(EventType.SUCCESS, `Built ${asset.id}`)) { return false; } } catch (e: any) { this.failures.push({ asset, error: e }); this.completedOperations++; @@ -177,7 +149,7 @@ export class AssetPublishing implements IPublishProgress { */ public async publishEntry(asset: IManifestEntry) { try { - if (this.progressEvent(EventType.UPLOAD, `${this.startMessagePrefix} ${asset.id}`)) { return false; } + if (this.progressEvent(EventType.START, `Publishing ${asset.id}`)) { return false; } const handler = this.assetHandler(asset); await handler.publish(); @@ -187,7 +159,7 @@ export class AssetPublishing implements IPublishProgress { } this.completedOperations++; - if (this.progressEvent(EventType.SUCCESS, `${this.successMessagePrefix} ${asset.id}`)) { return false; } + if (this.progressEvent(EventType.SUCCESS, `Published ${asset.id}`)) { return false; } } catch (e: any) { this.failures.push({ asset, error: e }); this.completedOperations++; @@ -212,7 +184,7 @@ export class AssetPublishing implements IPublishProgress { */ private async publishAsset(asset: IManifestEntry) { try { - if (this.progressEvent(EventType.START, `${this.startMessagePrefix} ${asset.id}`)) { return false; } + if (this.progressEvent(EventType.START, `Publishing ${asset.id}`)) { return false; } const handler = this.assetHandler(asset); @@ -229,7 +201,7 @@ export class AssetPublishing implements IPublishProgress { } this.completedOperations++; - if (this.progressEvent(EventType.SUCCESS, `${this.successMessagePrefix} ${asset.id}`)) { return false; } + if (this.progressEvent(EventType.SUCCESS, `Published ${asset.id}`)) { return false; } } catch (e: any) { this.failures.push({ asset, error: e }); this.completedOperations++; From 2ab6fa1d08b01f5f400e3bde87fd8f1879f5abf8 Mon Sep 17 00:00:00 2001 From: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com> Date: Thu, 18 May 2023 17:01:52 -0400 Subject: [PATCH 2/7] Update packages/aws-cdk/lib/logging.ts --- packages/aws-cdk/lib/logging.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/logging.ts b/packages/aws-cdk/lib/logging.ts index 74879054c935f..f539c6a014f8e 100644 --- a/packages/aws-cdk/lib/logging.ts +++ b/packages/aws-cdk/lib/logging.ts @@ -7,7 +7,7 @@ const { stdout, stderr } = process; type WritableFactory = () => Writable; -// LOGGER_LOCKED is an array rather than a boolean because it needs to be modified by other +// LOG_LOCK is an array rather than a boolean because it needs to be modified by other // parts of the CLI and imported variables are always immutable. export const LOG_LOCK = [false]; const logBuffer: string[] = []; From 3cd1fd939e4a2b2aafa5ad0df8bf64d43687b08a Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Thu, 18 May 2023 17:17:46 -0400 Subject: [PATCH 3/7] better way to lock --- packages/aws-cdk/lib/cdk-toolkit.ts | 10 +++++----- packages/aws-cdk/lib/logging.ts | 11 +++++++---- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index d46f9cfa74263..64e996353b22c 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -17,7 +17,7 @@ import { CloudWatchLogEventMonitor } from './api/logs/logs-monitor'; import { StackActivityProgress } from './api/util/cloudformation/stack-activity-monitor'; import { printSecurityDiff, printStackDiff, RequireApproval } from './diff'; import { ResourceImporter } from './import'; -import { data, debug, error, highlight, print, success, warning, LOG_LOCK } from './logging'; +import { data, debug, error, highlight, print, success, warning, setLogLock } from './logging'; import { deserializeStructure, serializeStructure } from './serialize'; import { Configuration, PROJECT_CONFIG } from './settings'; import { numberFromBool, partition } from './util'; @@ -239,7 +239,7 @@ export class CdkToolkit { const currentTemplate = await this.props.deployments.readCurrentTemplate(stack); if (printSecurityDiff(currentTemplate, stack, requireApproval)) { // Lock the logger from logging temporarily - LOG_LOCK[0] = true; + setLogLock(true); // only talk to user if STDIN is a terminal (otherwise, fail) if (!process.stdin.isTTY) { @@ -256,10 +256,10 @@ export class CdkToolkit { } const confirmed = await promptly.confirm('Do you wish to deploy these changes (y/n)?'); - - LOG_LOCK[0] = false; + + setLogLock(false); // Calls the logger and prints any logs that happened during the lock - print(''); + print(''); if (!confirmed) { throw new Error('Aborted by user'); } } diff --git a/packages/aws-cdk/lib/logging.ts b/packages/aws-cdk/lib/logging.ts index f539c6a014f8e..cfb4964e35e89 100644 --- a/packages/aws-cdk/lib/logging.ts +++ b/packages/aws-cdk/lib/logging.ts @@ -7,9 +7,12 @@ const { stdout, stderr } = process; type WritableFactory = () => Writable; -// LOG_LOCK is an array rather than a boolean because it needs to be modified by other -// parts of the CLI and imported variables are always immutable. -export const LOG_LOCK = [false]; +let LOG_LOCK = false; + +export function setLogLock(lock: boolean) { + LOG_LOCK = lock; +} + const logBuffer: string[] = []; const logger = (stream: Writable | WritableFactory, styles?: StyleFn[], timestamp?: boolean) => (fmt: string, ...args: unknown[]) => { @@ -22,7 +25,7 @@ const logger = (stream: Writable | WritableFactory, styles?: StyleFn[], timestam // Logger is currently locked, so we store the message to be printed // later when we are unlocked. - if (LOG_LOCK[0]) { + if (LOG_LOCK) { logBuffer.push(str); return; } From 1c4d834732a056aa799352d5246851e3053d2316 Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Fri, 19 May 2023 10:27:51 -0400 Subject: [PATCH 4/7] update unit test --- packages/cdk-assets/test/docker-images.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cdk-assets/test/docker-images.test.ts b/packages/cdk-assets/test/docker-images.test.ts index 688828a68c021..18b713947c365 100644 --- a/packages/cdk-assets/test/docker-images.test.ts +++ b/packages/cdk-assets/test/docker-images.test.ts @@ -247,7 +247,7 @@ describe('with a complete manifest', () => { test('Displays an error if the ECR repository cannot be found', async () => { aws.mockEcr.describeImages = mockedApiFailure('RepositoryNotFoundException', 'Repository not Found'); - await expect(pub.publish()).rejects.toThrow('Error building and publishing: Repository not Found'); + await expect(pub.publish()).rejects.toThrow('Error publishing: Repository not Found'); }); test('successful run does not need to query account ID', async () => { From d06e81efd8e23237620ac4edf2240e7942fcbb8b Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Fri, 19 May 2023 10:59:52 -0400 Subject: [PATCH 5/7] pr feedback --- packages/aws-cdk/lib/cdk-toolkit.ts | 44 +++++++++++++---------------- packages/aws-cdk/lib/logging.ts | 43 +++++++++++++++++++--------- 2 files changed, 49 insertions(+), 38 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 64e996353b22c..7cf0ccc6b42f1 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -17,7 +17,7 @@ import { CloudWatchLogEventMonitor } from './api/logs/logs-monitor'; import { StackActivityProgress } from './api/util/cloudformation/stack-activity-monitor'; import { printSecurityDiff, printStackDiff, RequireApproval } from './diff'; import { ResourceImporter } from './import'; -import { data, debug, error, highlight, print, success, warning, setLogLock } from './logging'; +import { data, debug, error, highlight, print, success, warning, withCorkedLogging } from './logging'; import { deserializeStructure, serializeStructure } from './serialize'; import { Configuration, PROJECT_CONFIG } from './settings'; import { numberFromBool, partition } from './util'; @@ -238,30 +238,24 @@ export class CdkToolkit { if (requireApproval !== RequireApproval.Never) { const currentTemplate = await this.props.deployments.readCurrentTemplate(stack); if (printSecurityDiff(currentTemplate, stack, requireApproval)) { - // Lock the logger from logging temporarily - setLogLock(true); - - // only talk to user if STDIN is a terminal (otherwise, fail) - if (!process.stdin.isTTY) { - throw new Error( - '"--require-approval" is enabled and stack includes security-sensitive updates, ' + - 'but terminal (TTY) is not attached so we are unable to get a confirmation from the user'); - } - - // only talk to user if concurrency is 1 (otherwise, fail) - if (concurrency > 1) { - throw new Error( - '"--require-approval" is enabled and stack includes security-sensitive updates, ' + - 'but concurrency is greater than 1 so we are unable to get a confirmation from the user'); - } - - const confirmed = await promptly.confirm('Do you wish to deploy these changes (y/n)?'); - - setLogLock(false); - // Calls the logger and prints any logs that happened during the lock - print(''); - - if (!confirmed) { throw new Error('Aborted by user'); } + await withCorkedLogging(async () => { + // only talk to user if STDIN is a terminal (otherwise, fail) + if (!process.stdin.isTTY) { + throw new Error( + '"--require-approval" is enabled and stack includes security-sensitive updates, ' + + 'but terminal (TTY) is not attached so we are unable to get a confirmation from the user'); + } + + // only talk to user if concurrency is 1 (otherwise, fail) + if (concurrency > 1) { + throw new Error( + '"--require-approval" is enabled and stack includes security-sensitive updates, ' + + 'but concurrency is greater than 1 so we are unable to get a confirmation from the user'); + } + + const confirmed = await promptly.confirm('Do you wish to deploy these changes (y/n)?'); + if (!confirmed) { throw new Error('Aborted by user'); } + }); } } diff --git a/packages/aws-cdk/lib/logging.ts b/packages/aws-cdk/lib/logging.ts index cfb4964e35e89..120869ac748ce 100644 --- a/packages/aws-cdk/lib/logging.ts +++ b/packages/aws-cdk/lib/logging.ts @@ -7,13 +7,32 @@ const { stdout, stderr } = process; type WritableFactory = () => Writable; -let LOG_LOCK = false; +export async function withCorkedLogging(block: () => Promise): Promise { + corkLogging(); + try { + return await block(); + } finally { + uncorkLogging(); + } +} + +let CORK_COUNTER = 0; +const logBuffer: [Writable, string][] = []; -export function setLogLock(lock: boolean) { - LOG_LOCK = lock; +function corked() { + return CORK_COUNTER !== 0; } -const logBuffer: string[] = []; +export function corkLogging() { + CORK_COUNTER += 1; +} + +export function uncorkLogging() { + CORK_COUNTER -= 1; + if (!corked()) { + logBuffer.forEach(([stream, str]) => stream.write(str + '\n')); + } +} const logger = (stream: Writable | WritableFactory, styles?: StyleFn[], timestamp?: boolean) => (fmt: string, ...args: unknown[]) => { const ts = timestamp ? `[${formatTime(new Date())}] ` : ''; @@ -23,21 +42,19 @@ const logger = (stream: Writable | WritableFactory, styles?: StyleFn[], timestam str = styles.reduce((a, style) => style(a), str); } - // Logger is currently locked, so we store the message to be printed - // later when we are unlocked. - if (LOG_LOCK) { - logBuffer.push(str); + const realStream = typeof stream === 'function' ? stream() : stream; + + // Logger is currently corked, so we store the message to be printed + // later when we are uncorked. + if (corked()) { + logBuffer.push([realStream, str]); return; } - const realStream = typeof stream === 'function' ? stream() : stream; - if (logBuffer.length > 0) { - logBuffer.forEach((l) => realStream.write(l + '\n')); - logBuffer.splice(0); - } realStream.write(str + '\n'); }; + function formatTime(d: Date) { return `${lpad(d.getHours(), 2)}:${lpad(d.getMinutes(), 2)}:${lpad(d.getSeconds(), 2)}`; From bc0fd1c79bd24c3c502e328f2080bad486c6156b Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Fri, 19 May 2023 11:03:13 -0400 Subject: [PATCH 6/7] unexport two functions --- packages/aws-cdk/lib/logging.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk/lib/logging.ts b/packages/aws-cdk/lib/logging.ts index 120869ac748ce..3c606add465c8 100644 --- a/packages/aws-cdk/lib/logging.ts +++ b/packages/aws-cdk/lib/logging.ts @@ -23,11 +23,11 @@ function corked() { return CORK_COUNTER !== 0; } -export function corkLogging() { +function corkLogging() { CORK_COUNTER += 1; } -export function uncorkLogging() { +function uncorkLogging() { CORK_COUNTER -= 1; if (!corked()) { logBuffer.forEach(([stream, str]) => stream.write(str + '\n')); From b86faeffa8eff147d99e5820c7e049fe3320f794 Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Fri, 19 May 2023 11:04:22 -0400 Subject: [PATCH 7/7] clear logbuffer; --- packages/aws-cdk/lib/logging.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/aws-cdk/lib/logging.ts b/packages/aws-cdk/lib/logging.ts index 3c606add465c8..50f739f185216 100644 --- a/packages/aws-cdk/lib/logging.ts +++ b/packages/aws-cdk/lib/logging.ts @@ -31,6 +31,7 @@ function uncorkLogging() { CORK_COUNTER -= 1; if (!corked()) { logBuffer.forEach(([stream, str]) => stream.write(str + '\n')); + logBuffer.splice(0); } }