From 9fcb95aaf6c79f167c0982567114ecc70e3e46d4 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 4 Mar 2025 11:19:49 +0100 Subject: [PATCH 1/4] fix(cli): write notices to stderr or don't write them at all On CI systems, the CDK CLI tries to avoid writing to `stderr` because there are a couple of CI systems that are commonly configured to fail if any output is written to `stderr`. That means all output, like notices, must go to `stdout`. Some commands (like `cdk synth` or `cdk bootstrap --show-template`) produce usable output on `stdout`, and these are commonly scripted, like piping their output to a file. However, because notices must go to `stdout`, these now interfere with the output of these commands. This needs a more thorough reworking of the CLI output streams, but there is a risk of affecting users who are currently relying on the fact that all output goes to `stdout`. In this PR, we are doing the first steps to solving this situation: - Notices will always go to `stderr`, so that they will never interfere with `stdout` anymore. - We try to detect what CI system we are running on, and we will completely suppress notices *unless* we determine that we are running on a CI system where it is "safe" to write to `sterr` (fail closed). "Safe" in this case means that the CI system doesn't come with an easy to toggle checkbox that makes commands fail based on what they print, instead of their exit codes. The only systems I'm aware of that have this checkbox are "Azure DevOps", and "TeamCity running PowerShell scripts". Even though we know the systems that are "unsafe", we will only show notices on systems known to be "safe". Fixes https://github.com/aws/aws-cdk/issues/33589. --- .../toolkit-lib/lib/api/io/private/codes.ts | 22 +++++ packages/aws-cdk/lib/cli/ci-systems.ts | 83 +++++++++++++++++++ packages/aws-cdk/lib/cli/cli.ts | 23 ++++- packages/aws-cdk/lib/notices.ts | 49 +++++------ packages/aws-cdk/lib/toolkit/cli-io-host.ts | 51 +++++++++++- 5 files changed, 194 insertions(+), 34 deletions(-) create mode 100644 packages/aws-cdk/lib/cli/ci-systems.ts diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts index 9fcc78d11..18f834d42 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/io/private/codes.ts @@ -206,6 +206,28 @@ export const CODES = { level: 'error', }), + // Notices + CDK_TOOLKIT_I0100: codeInfo({ + code: 'CDK_TOOLKIT_I0100', + description: 'Notices decoration (the header or footer of a list of notices)', + level: 'info', + }), + CDK_TOOLKIT_W0101: codeInfo({ + code: 'CDK_TOOLKIT_W0101', + description: 'A notice that is marked as a warning', + level: 'warn', + }), + CDK_TOOLKIT_E0101: codeInfo({ + code: 'CDK_TOOLKIT_E0101', + description: 'A notice that is marked as an error', + level: 'error', + }), + CDK_TOOLKIT_I0101: codeInfo({ + code: 'CDK_TOOLKIT_I0101', + description: 'A notice that is marked as informational', + level: 'info', + }), + // Assembly codes CDK_ASSEMBLY_I0042: codeInfo({ code: 'CDK_ASSEMBLY_I0042', diff --git a/packages/aws-cdk/lib/cli/ci-systems.ts b/packages/aws-cdk/lib/cli/ci-systems.ts new file mode 100644 index 000000000..7b0ec022a --- /dev/null +++ b/packages/aws-cdk/lib/cli/ci-systems.ts @@ -0,0 +1,83 @@ + +interface CiSystem { + /** + * What's the name? + */ + readonly name: string; + + /** + * What environment variable indicates that we are running on this system? + */ + readonly detectEnvVar: string; + + /** + * Whether or not this CI system can be configured to fail on messages written to stderr + * + * With "can be configured", what we mean is that a checkbox or configuration + * flag to enable this behavior comes out of the box with the CI system and (judgement + * call), this flag is "commonly" used. + * + * Of course every CI system can be scripted to have this behavior, but that's + * not what we mean. + */ + readonly canBeConfiguredToFailOnStdErr: boolean; +} + +const CI_SYSTEMS: CiSystem[] = [ + { + name: 'Azure DevOps', + // https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml + detectEnvVar: 'TF_BUILD', + canBeConfiguredToFailOnStdErr: true, + }, + { + name: 'TeamCity', + // https://www.jetbrains.com/help/teamcity/predefined-build-parameters.html + detectEnvVar: 'TEAMCITY_VERSION', + // Can be configured to fail on stderr, when using a PowerShell task + canBeConfiguredToFailOnStdErr: true, + }, + { + name: 'GitHub Actions', + // https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables + detectEnvVar: 'GITHUB_ACTION', + canBeConfiguredToFailOnStdErr: false, + }, + { + name: 'CodeBuild', + // https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-env-vars.html + detectEnvVar: 'CODEBUILD_BUILD_ID', + canBeConfiguredToFailOnStdErr: false, + }, + { + name: 'CircleCI', + // https://circleci.com/docs/variables/#built-in-environment-variables + detectEnvVar: 'CIRCLECI', + canBeConfiguredToFailOnStdErr: false, + }, + { + name: 'Jenkins', + // https://www.jenkins.io/doc/book/pipeline/jenkinsfile/#using-environment-variables + detectEnvVar: 'EXECUTOR_NUMBER', + canBeConfiguredToFailOnStdErr: false, + }, +]; + +export function detectCiSystem(): CiSystem | undefined { + for (const ciSystem of CI_SYSTEMS) { + if (process.env[ciSystem.detectEnvVar]) { + return ciSystem; + } + } + return undefined; +} + +/** + * Return whether the CI system we're detecting is safe to write to stderr on + * + * Returns `undefined` if the current CI system cannot be recognized. + */ +export function ciSystemIsStdErrSafe(): boolean | undefined { + const x = detectCiSystem()?.canBeConfiguredToFailOnStdErr; + return x === undefined ? undefined : !x; +} diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index d9d33ef96..b9e7b7817 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -2,6 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api'; import '@jsii/check-node/run'; import * as chalk from 'chalk'; import { CdkToolkit, AssetBuildTime } from './cdk-toolkit'; +import { ciSystemIsStdErrSafe } from './ci-systems'; import { parseCommandLineArguments } from './parse-command-line-arguments'; import { checkForPlatformWarnings } from './platform-warnings'; @@ -90,10 +91,23 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise; private readonly includeAcknowlegded: boolean; private readonly httpOptions: SdkHttpOptions; @@ -313,7 +305,6 @@ export class Notices { this.acknowledgedIssueNumbers = new Set(this.context.get('acknowledged-issue-numbers') ?? []); this.includeAcknowlegded = props.includeAcknowledged ?? false; this.output = props.output ?? 'cdk.out'; - this.shouldDisplay = props.shouldDisplay ?? true; this.httpOptions = props.httpOptions ?? {}; } @@ -339,10 +330,6 @@ export class Notices { * If context is configured to not display notices, this will no-op. */ public async refresh(options: NoticesRefreshOptions = {}) { - if (!this.shouldDisplay) { - return; - } - try { const underlyingDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.httpOptions); const dataSource = new CachedDataSource(CACHE_FILE_PATH, underlyingDataSource, options.force ?? false); @@ -357,10 +344,6 @@ export class Notices { * Display the relevant notices (unless context dictates we shouldn't). */ public display(options: NoticesPrintOptions = {}) { - if (!this.shouldDisplay) { - return; - } - const filteredNotices = NoticesFilter.filter({ data: Array.from(this.data), cliVersion: versionNumber(), @@ -369,29 +352,39 @@ export class Notices { }); if (filteredNotices.length > 0) { - info(''); - info('NOTICES (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)'); - info(''); + info({ + code: 'CDK_TOOLKIT_I0100', + message: [ + '', + 'NOTICES (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)', + '', + ].join('\n'), + }); for (const filtered of filteredNotices) { - const formatted = filtered.format(); + const formatted = filtered.format() + '\n'; switch (filtered.notice.severity) { case 'warning': - warning(formatted); + warning({ code: 'CDK_TOOLKIT_W0101', message: formatted }); break; case 'error': - error(formatted); + error({ code: 'CDK_TOOLKIT_E0101', message: formatted }); break; default: - info(formatted); + info({ code: 'CDK_TOOLKIT_I0101', message: formatted }); + break; } - info(''); } - info(`If you don’t want to see a notice anymore, use "cdk acknowledge ". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".`); + info({ + code: 'CDK_TOOLKIT_I0100', + message: `If you don’t want to see a notice anymore, use "cdk acknowledge ". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".` + }); } if (options.showTotal ?? false) { - info(''); - info(`There are ${filteredNotices.length} unacknowledged notice(s).`); + info({ + code: 'CDK_TOOLKIT_I0100', + message: `\nThere are ${filteredNotices.length} unacknowledged notice(s).`, + }); } } } diff --git a/packages/aws-cdk/lib/toolkit/cli-io-host.ts b/packages/aws-cdk/lib/toolkit/cli-io-host.ts index 8a998c05e..1289a0c24 100644 --- a/packages/aws-cdk/lib/toolkit/cli-io-host.ts +++ b/packages/aws-cdk/lib/toolkit/cli-io-host.ts @@ -170,6 +170,11 @@ export interface CliIoHostProps { readonly stackProgress?: StackActivityProgress; } +/** + * A type for configuring a target stream + */ +export type TargetStream = 'stdout' | 'stderr' | 'drop'; + /** * A simple IO host for the CLI that writes messages to the console. */ @@ -189,6 +194,14 @@ export class CliIoHost implements IIoHost { */ private static _instance: CliIoHost | undefined; + /** + * Configure the target stream for notices + * + * (Not a setter because there's no need for additional logic when this value + * is changed yet) + */ + public noticesTarget: TargetStream = 'stderr'; + // internal state for getters/setter private _currentAction: ToolkitAction; private _isCI: boolean; @@ -379,8 +392,8 @@ export class CliIoHost implements IIoHost { } const output = this.formatMessage(msg); - const stream = this.selectStream(msg.level); - stream.write(output); + const stream = this.selectStream(msg); + stream?.write(output); } /** @@ -394,10 +407,21 @@ export class CliIoHost implements IIoHost { ].includes(msg.code); } + /** + * Determines the output stream, based on message and configuration. + */ + private selectStream(msg: IoMessage): NodeJS.WriteStream | undefined { + if (isNoticesMessage(msg)) { + return targetStreamObject(this.noticesTarget); + } + + return this.selectStreamFromLevel(msg.level); + } + /** * Determines the output stream, based on message level and configuration. */ - private selectStream(level: IoMessageLevel) { + private selectStreamFromLevel(level: IoMessageLevel): NodeJS.WriteStream { // The stream selection policy for the CLI is the following: // // (1) Messages of level `result` always go to `stdout` @@ -506,7 +530,7 @@ export class CliIoHost implements IIoHost { */ private makeActivityPrinter() { const props: ActivityPrinterProps = { - stream: this.selectStream('info'), + stream: this.selectStreamFromLevel('info'), }; switch (this.stackProgress) { @@ -568,3 +592,22 @@ export function isCI(): boolean { return process.env.CI !== undefined && process.env.CI !== 'false' && process.env.CI !== '0'; } +function targetStreamObject(x: TargetStream): NodeJS.WriteStream | undefined { + switch (x) { + case 'stderr': + return process.stderr; + case 'stdout': + return process.stdout; + case 'drop': + return undefined; + } +} + +function isNoticesMessage(msg: IoMessage) { + return [ + 'CDK_TOOLKIT_I0100', + 'CDK_TOOLKIT_W0101', + 'CDK_TOOLKIT_E0101', + 'CDK_TOOLKIT_I0101', + ].includes(msg.code); +} \ No newline at end of file From 70766239ede47c2f36c1e0d8566672986e79ad26 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 4 Mar 2025 11:30:37 +0100 Subject: [PATCH 2/4] noticesTarget => noticesDestination --- packages/aws-cdk/lib/cli/cli.ts | 6 +++--- packages/aws-cdk/lib/toolkit/cli-io-host.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index b9e7b7817..ed82fb16c 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -94,7 +94,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise): NodeJS.WriteStream | undefined { if (isNoticesMessage(msg)) { - return targetStreamObject(this.noticesTarget); + return targetStreamObject(this.noticesDestination); } return this.selectStreamFromLevel(msg.level); From a8603353f0beaf08e07ea7bcdbde4ee7e2ba8540 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 4 Mar 2025 12:51:17 +0100 Subject: [PATCH 3/4] Add tests --- packages/aws-cdk/lib/cli/cli.ts | 1 + packages/aws-cdk/lib/logging.ts | 92 ++++++--- packages/aws-cdk/lib/notices.ts | 76 ++++--- packages/aws-cdk/lib/toolkit/cli-io-host.ts | 6 +- .../environment/environment-resources.test.ts | 11 +- packages/aws-cdk/test/notices.test.ts | 185 +++++++----------- packages/aws-cdk/test/util/fake-io-host.ts | 27 +++ 7 files changed, 218 insertions(+), 180 deletions(-) create mode 100644 packages/aws-cdk/test/util/fake-io-host.ts diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index ed82fb16c..6ad2e51ae 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -106,6 +106,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise string, ...args: unknown[] ): void { - // Extract message and code from input, using new default code format - const { message, code = getDefaultCode(level) } = typeof input === 'object' ? input : { message: input }; - - // Format message if args are provided - const formattedMessage = args.length > 0 - ? util.format(message, ...args) - : message; - - // Apply style if provided - const finalMessage = style ? style(formattedMessage) : formattedMessage; - - const ioHost = CliIoHost.instance(); - const ioMessage: IoMessage = { - time: new Date(), - action: ioHost.currentAction, - level, - message: finalMessage, - code, - }; - - void ioHost.notify(ioMessage); + const singletonHost = CliIoHost.instance(); + new IoHostEmitter(singletonHost, singletonHost.currentAction).emit(level, input, style, ...args); } function getDefaultCode(level: IoMessageLevel, category: IoMessageCodeCategory = 'TOOLKIT'): IoMessageCode { @@ -132,7 +115,7 @@ export const result = (input: LogInput<'I'>, ...args: unknown[]) => { * debug({ message: 'ratio: %d%%', code: 'CDK_TOOLKIT_I001' }, ratio) // specifies info code `CDK_TOOLKIT_I001` * ``` */ -export const debug = (input: LogInput<'I'>, ...args: unknown[]) => { +export const debug = (input: LogInput<'D'>, ...args: unknown[]) => { return formatMessageAndLog('debug', input, undefined, ...args); }; @@ -147,7 +130,7 @@ export const debug = (input: LogInput<'I'>, ...args: unknown[]) => { * trace({ message: 'method: %s', code: 'CDK_TOOLKIT_I001' }, name) // specifies info code `CDK_TOOLKIT_I001` * ``` */ -export const trace = (input: LogInput<'I'>, ...args: unknown[]) => { +export const trace = (input: LogInput<'D'>, ...args: unknown[]) => { return formatMessageAndLog('trace', input, undefined, ...args); }; @@ -180,3 +163,64 @@ export const success = (input: LogInput<'I'>, ...args: unknown[]) => { export const highlight = (input: LogInput<'I'>, ...args: unknown[]) => { return formatMessageAndLog('info', input, chalk.bold, ...args); }; + +/** + * Helper class to emit messages to an IoHost + * + * Messages sent to the IoHost require the current action, so this class holds on + * to an IoHost and an action. + * + * It also contains convenience methods for the various log levels, same as the global + * ones but scoped to a particular `IoHost` instead of the global singleton one. + */ +export class IoHostEmitter { + constructor(private readonly ioHost: IIoHost, private readonly action: ToolkitAction) { + } + + public error(input: LogInput<'E'>, ...args: unknown[]) { + this.emit('error', input, undefined, ...args); + } + + public warning(input: LogInput<'W'>, ...args: unknown[]) { + this.emit('warn', input, undefined, ...args); + } + + public info(input: LogInput<'I'>, ...args: unknown[]) { + this.emit('info', input, undefined, ...args); + } + + public result(input: LogInput<'I'>, ...args: unknown[]) { + this.emit('result', input, undefined, ...args); + } + + public debug(input: LogInput<'D'>, ...args: unknown[]) { + this.emit('debug', input, undefined, ...args); + } + + public trace(input: LogInput<'D'>, ...args: unknown[]) { + this.emit('trace', input, undefined, ...args); + } + + public emit(level: IoMessageLevel, input: LogInput, style?: (str: string) => string, ...args: unknown[]) { + // Extract message and code from input, using new default code format + const { message, code = getDefaultCode(level) } = typeof input === 'object' ? input : { message: input }; + + // Format message if args are provided + const formattedMessage = args.length > 0 + ? util.format(message, ...args) + : message; + + // Apply style if provided + const finalMessage = style ? style(formattedMessage) : formattedMessage; + + const ioMessage: IoMessage = { + time: new Date(), + action: this.action, + level, + message: finalMessage, + code, + }; + + void this.ioHost.notify(ioMessage); + } +} diff --git a/packages/aws-cdk/lib/notices.ts b/packages/aws-cdk/lib/notices.ts index 32285e7f7..18d38f8b9 100644 --- a/packages/aws-cdk/lib/notices.ts +++ b/packages/aws-cdk/lib/notices.ts @@ -9,7 +9,8 @@ import { SdkHttpOptions } from './api'; import { AwsCliCompatible } from './api/aws-auth/awscli-compatible'; import type { Context } from './api/context'; import { versionNumber } from './cli/version'; -import { debug, info, warning, error } from './logging'; +import { IoHostEmitter } from './logging'; +import { IIoHost } from './toolkit/cli-io-host'; import { ToolkitError } from './toolkit/error'; import { ConstructTreeNode, loadTreeFromDir } from './tree'; import { cdkCacheDir, formatErrorMessage } from './util'; @@ -40,6 +41,11 @@ export interface NoticesProps { * Options for the HTTP request */ readonly httpOptions?: SdkHttpOptions; + + /** + * Where messages are going to be sent + */ + readonly ioHost: IIoHost; } export interface NoticesPrintOptions { @@ -75,20 +81,23 @@ export interface NoticesFilterFilterOptions { readonly bootstrappedEnvironments: BootstrappedEnvironment[]; } -export abstract class NoticesFilter { - public static filter(options: NoticesFilterFilterOptions): FilteredNotice[] { +export class NoticesFilter { + constructor(private readonly ioHostEmitter: IoHostEmitter) { + } + + public filter(options: NoticesFilterFilterOptions): FilteredNotice[] { const components = [ - ...NoticesFilter.constructTreeComponents(options.outDir), - ...NoticesFilter.otherComponents(options), + ...this.constructTreeComponents(options.outDir), + ...this.otherComponents(options), ]; - return NoticesFilter.findForNamedComponents(options.data, components); + return this.findForNamedComponents(options.data, components); } /** * From a set of input options, return the notices components we are searching for */ - private static otherComponents(options: NoticesFilterFilterOptions): ActualComponent[] { + private otherComponents(options: NoticesFilterFilterOptions): ActualComponent[] { return [ // CLI { @@ -108,7 +117,7 @@ export abstract class NoticesFilter { const semverBootstrapVersion = semver.coerce(env.bootstrapStackVersion); if (!semverBootstrapVersion) { // we don't throw because notices should never crash the cli. - warning(`While filtering notices, could not coerce bootstrap version '${env.bootstrapStackVersion}' into semver`); + this.ioHostEmitter.warning(`While filtering notices, could not coerce bootstrap version '${env.bootstrapStackVersion}' into semver`); return []; } @@ -125,7 +134,7 @@ export abstract class NoticesFilter { /** * Based on a set of component names, find all notices that match one of the given components */ - private static findForNamedComponents(data: Notice[], actualComponents: ActualComponent[]): FilteredNotice[] { + private findForNamedComponents(data: Notice[], actualComponents: ActualComponent[]): FilteredNotice[] { return data.flatMap(notice => { const ors = this.resolveAliases(normalizeComponents(notice.components)); @@ -134,12 +143,12 @@ export abstract class NoticesFilter { // component can match more than one actual component for (const ands of ors) { const matched = ands.map(affected => actualComponents.filter(actual => - NoticesFilter.componentNameMatches(affected, actual) && semver.satisfies(actual.version, affected.version, { includePrerelease: true }))); + this.componentNameMatches(affected, actual) && semver.satisfies(actual.version, affected.version, { includePrerelease: true }))); // For every clause in the filter we matched one or more components if (matched.every(xs => xs.length > 0)) { const ret = new FilteredNotice(notice); - NoticesFilter.addDynamicValues(matched.flatMap(x => x), ret); + this.addDynamicValues(matched.flatMap(x => x), ret); return [ret]; } } @@ -154,7 +163,7 @@ export abstract class NoticesFilter { * The name matches if the name is exactly the same, or the name in the notice * is a prefix of the node name when the query ends in '.'. */ - private static componentNameMatches(pattern: Component, actual: ActualComponent): boolean { + private componentNameMatches(pattern: Component, actual: ActualComponent): boolean { return pattern.name.endsWith('.') ? actual.name.startsWith(pattern.name) : pattern.name === actual.name; } @@ -164,7 +173,7 @@ export abstract class NoticesFilter { * If there are multiple components with the same dynamic name, they are joined * by a comma. */ - private static addDynamicValues(comps: ActualComponent[], notice: FilteredNotice) { + private addDynamicValues(comps: ActualComponent[], notice: FilteredNotice) { const dynamicValues: Record = {}; for (const comp of comps) { if (comp.dynamicName) { @@ -183,7 +192,7 @@ export abstract class NoticesFilter { * Because it's EITHER `aws-cdk-lib` or `@aws-cdk/core`, we need to add multiple * arrays at the top level. */ - private static resolveAliases(ors: Component[][]): Component[][] { + private resolveAliases(ors: Component[][]): Component[][] { return ors.flatMap(ands => { const hasFramework = ands.find(c => c.name === 'framework'); if (!hasFramework) { @@ -200,7 +209,7 @@ export abstract class NoticesFilter { /** * Load the construct tree from the given directory and return its components */ - private static constructTreeComponents(manifestDir: string): ActualComponent[] { + private constructTreeComponents(manifestDir: string): ActualComponent[] { const tree = loadTreeFromDir(manifestDir); if (!tree) { return []; @@ -294,6 +303,7 @@ export class Notices { private readonly acknowledgedIssueNumbers: Set; private readonly includeAcknowlegded: boolean; private readonly httpOptions: SdkHttpOptions; + private readonly ioHostEmitter: IoHostEmitter; private data: Set = new Set(); @@ -306,6 +316,7 @@ export class Notices { this.includeAcknowlegded = props.includeAcknowledged ?? false; this.output = props.output ?? 'cdk.out'; this.httpOptions = props.httpOptions ?? {}; + this.ioHostEmitter = new IoHostEmitter(props.ioHost, 'notices'); } /** @@ -331,12 +342,12 @@ export class Notices { */ public async refresh(options: NoticesRefreshOptions = {}) { try { - const underlyingDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.httpOptions); - const dataSource = new CachedDataSource(CACHE_FILE_PATH, underlyingDataSource, options.force ?? false); + const underlyingDataSource = options.dataSource ?? new WebsiteNoticeDataSource(this.ioHostEmitter, this.httpOptions); + const dataSource = new CachedDataSource(this.ioHostEmitter, CACHE_FILE_PATH, underlyingDataSource, options.force ?? false); const notices = await dataSource.fetch(); this.data = new Set(this.includeAcknowlegded ? notices : notices.filter(n => !this.acknowledgedIssueNumbers.has(n.issueNumber))); } catch (e: any) { - debug(`Could not refresh notices: ${e}`); + this.ioHostEmitter.debug(`Could not refresh notices: ${e}`); } } @@ -344,7 +355,7 @@ export class Notices { * Display the relevant notices (unless context dictates we shouldn't). */ public display(options: NoticesPrintOptions = {}) { - const filteredNotices = NoticesFilter.filter({ + const filteredNotices = new NoticesFilter(this.ioHostEmitter).filter({ data: Array.from(this.data), cliVersion: versionNumber(), outDir: this.output, @@ -352,7 +363,7 @@ export class Notices { }); if (filteredNotices.length > 0) { - info({ + this.ioHostEmitter.info({ code: 'CDK_TOOLKIT_I0100', message: [ '', @@ -364,24 +375,24 @@ export class Notices { const formatted = filtered.format() + '\n'; switch (filtered.notice.severity) { case 'warning': - warning({ code: 'CDK_TOOLKIT_W0101', message: formatted }); + this.ioHostEmitter.warning({ code: 'CDK_TOOLKIT_W0101', message: formatted }); break; case 'error': - error({ code: 'CDK_TOOLKIT_E0101', message: formatted }); + this.ioHostEmitter.error({ code: 'CDK_TOOLKIT_E0101', message: formatted }); break; default: - info({ code: 'CDK_TOOLKIT_I0101', message: formatted }); + this.ioHostEmitter.info({ code: 'CDK_TOOLKIT_I0101', message: formatted }); break; } } - info({ + this.ioHostEmitter.info({ code: 'CDK_TOOLKIT_I0100', - message: `If you don’t want to see a notice anymore, use "cdk acknowledge ". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".` + message: `If you don’t want to see a notice anymore, use "cdk acknowledge ". For example, "cdk acknowledge ${filteredNotices[0].notice.issueNumber}".`, }); } if (options.showTotal ?? false) { - info({ + this.ioHostEmitter.info({ code: 'CDK_TOOLKIT_I0100', message: `\nThere are ${filteredNotices.length} unacknowledged notice(s).`, }); @@ -478,7 +489,7 @@ export interface NoticeDataSource { export class WebsiteNoticeDataSource implements NoticeDataSource { private readonly options: SdkHttpOptions; - constructor(options: SdkHttpOptions = {}) { + constructor(private readonly ioHostEmitter: IoHostEmitter, options: SdkHttpOptions = {}) { this.options = options; } @@ -515,7 +526,7 @@ export class WebsiteNoticeDataSource implements NoticeDataSource { if (!data) { throw new ToolkitError("'notices' key is missing"); } - debug('Notices refreshed'); + this.ioHostEmitter.debug('Notices refreshed'); resolve(data ?? []); } catch (e: any) { reject(new ToolkitError(`Failed to parse notices: ${formatErrorMessage(e)}`)); @@ -546,6 +557,7 @@ const TIME_TO_LIVE_ERROR = 1 * 60 * 1000; // 1 minute export class CachedDataSource implements NoticeDataSource { constructor( + private readonly ioHostEmitter: IoHostEmitter, private readonly fileName: string, private readonly dataSource: NoticeDataSource, private readonly skipCache?: boolean) { @@ -561,7 +573,7 @@ export class CachedDataSource implements NoticeDataSource { await this.save(freshData); return freshData.notices; } else { - debug(`Reading cached notices from ${this.fileName}`); + this.ioHostEmitter.debug(`Reading cached notices from ${this.fileName}`); return data; } } @@ -573,7 +585,7 @@ export class CachedDataSource implements NoticeDataSource { notices: await this.dataSource.fetch(), }; } catch (e) { - debug(`Could not refresh notices: ${e}`); + this.ioHostEmitter.debug(`Could not refresh notices: ${e}`); return { expiration: Date.now() + TIME_TO_LIVE_ERROR, notices: [], @@ -592,7 +604,7 @@ export class CachedDataSource implements NoticeDataSource { ? await fs.readJSON(this.fileName) as CachedNotices : defaultValue; } catch (e) { - debug(`Failed to load notices from cache: ${e}`); + this.ioHostEmitter.debug(`Failed to load notices from cache: ${e}`); return defaultValue; } } @@ -601,7 +613,7 @@ export class CachedDataSource implements NoticeDataSource { try { await fs.writeJSON(this.fileName, cached); } catch (e) { - debug(`Failed to store notices in the cache: ${e}`); + this.ioHostEmitter.debug(`Failed to store notices in the cache: ${e}`); } } } diff --git a/packages/aws-cdk/lib/toolkit/cli-io-host.ts b/packages/aws-cdk/lib/toolkit/cli-io-host.ts index 54d1f2940..9de5043fd 100644 --- a/packages/aws-cdk/lib/toolkit/cli-io-host.ts +++ b/packages/aws-cdk/lib/toolkit/cli-io-host.ts @@ -6,7 +6,7 @@ import { ActivityPrinterProps, CurrentActivityPrinter, HistoryActivityPrinter, I import { StackActivityProgress } from '../commands/deploy'; export type IoMessageCodeCategory = 'TOOLKIT' | 'SDK' | 'ASSETS'; -export type IoCodeLevel = 'E' | 'W' | 'I'; +export type IoCodeLevel = 'E' | 'W' | 'I' | 'D'; export type IoMessageSpecificCode = `CDK_${IoMessageCodeCategory}_${L}${number}${number}${number}${number}`; export type IoMessageCode = IoMessageSpecificCode; @@ -114,7 +114,7 @@ export interface IIoHost { * Notifies the host of a message. * The caller waits until the notification completes. */ - notify(msg: IoMessage): Promise; + notify(msg: IoMessage): Promise; /** * Notifies the host of a message that requires a response. @@ -610,4 +610,4 @@ function isNoticesMessage(msg: IoMessage) { 'CDK_TOOLKIT_E0101', 'CDK_TOOLKIT_I0101', ].includes(msg.code); -} \ No newline at end of file +} diff --git a/packages/aws-cdk/test/api/environment/environment-resources.test.ts b/packages/aws-cdk/test/api/environment/environment-resources.test.ts index 6a9e4207f..dfe601bd3 100644 --- a/packages/aws-cdk/test/api/environment/environment-resources.test.ts +++ b/packages/aws-cdk/test/api/environment/environment-resources.test.ts @@ -4,9 +4,11 @@ import { Context } from '../../../lib/api/context'; import { EnvironmentResourcesRegistry } from '../../../lib/api/environment'; import * as version from '../../../lib/cli/version'; import { CachedDataSource, Notices, NoticesFilter } from '../../../lib/notices'; -import { CliIoHost, IoMessaging } from '../../../lib/toolkit/cli-io-host'; +import { CliIoHost, IIoHost, IoMessaging } from '../../../lib/toolkit/cli-io-host'; import { MockSdk, mockBootstrapStack, mockSSMClient } from '../../util/mock-sdk'; import { MockToolkitInfo } from '../../util/mock-toolkitinfo'; +import { IoHostEmitter } from '../../../lib/logging'; +import { FakeIoHost } from '../../util/fake-io-host'; let mockSdk: MockSdk; let envRegistry: EnvironmentResourcesRegistry; @@ -102,11 +104,14 @@ describe('validateversion without bootstrap stack', () => { jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); // THEN - const notices = Notices.create({ context: new Context() }); + const ioHost = new FakeIoHost(); + const ioHostEmitter = new IoHostEmitter(ioHost, 'notices'); + const noticesFilter = new NoticesFilter(ioHostEmitter); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [] } }); await expect(envResources().validateVersion(8, '/abc')).resolves.toBeUndefined(); - const filter = jest.spyOn(NoticesFilter, 'filter'); + const filter = jest.spyOn(NoticesFilter.prototype, 'filter'); notices.display(); expect(filter).toHaveBeenCalledTimes(1); diff --git a/packages/aws-cdk/test/notices.test.ts b/packages/aws-cdk/test/notices.test.ts index b11a125b4..c97502a82 100644 --- a/packages/aws-cdk/test/notices.test.ts +++ b/packages/aws-cdk/test/notices.test.ts @@ -4,7 +4,6 @@ import * as os from 'os'; import * as path from 'path'; import * as fs from 'fs-extra'; import * as nock from 'nock'; -import * as logging from '../lib/logging'; import { CachedDataSource, Notice, @@ -18,6 +17,8 @@ import { import * as version from '../lib/cli/version'; import { Settings } from '../lib/api/settings'; import { Context } from '../lib/api/context'; +import { IoHostEmitter } from '../lib/logging'; +import { FakeIoHost } from './util/fake-io-host'; const BASIC_BOOTSTRAP_NOTICE = { title: 'Exccessive permissions on file asset publishing role', @@ -166,6 +167,15 @@ const NOTICE_FOR_APIGATEWAYV2_CFN_STAGE = { schemaVersion: '1', }; +const ioHost = new FakeIoHost(); +const ioHostEmitter = new IoHostEmitter(ioHost, 'notices'); +const noticesFilter = new NoticesFilter(ioHostEmitter); + +beforeEach(() => { + jest.restoreAllMocks(); + ioHost.clear(); +}); + describe(FilteredNotice, () => { describe('format', () => { test('resolves dynamic values', () => { @@ -227,10 +237,10 @@ describe(NoticesFilter, () => { // doesn't matter for this test because our data only has CLI notices const outDir = path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0'); - expect(NoticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.0.0' }).map(f => f.notice)).toEqual([BASIC_NOTICE]); - expect(NoticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.129.0' }).map(f => f.notice)).toEqual([MULTIPLE_AFFECTED_VERSIONS_NOTICE]); - expect(NoticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.126.0' }).map(f => f.notice)).toEqual([BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE]); - expect(NoticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.130.0' }).map(f => f.notice)).toEqual([]); + expect(noticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.0.0' }).map(f => f.notice)).toEqual([BASIC_NOTICE]); + expect(noticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.129.0' }).map(f => f.notice)).toEqual([MULTIPLE_AFFECTED_VERSIONS_NOTICE]); + expect(noticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.126.0' }).map(f => f.notice)).toEqual([BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE]); + expect(noticesFilter.filter({ data: notices, bootstrappedEnvironments: [], outDir, cliVersion: '1.130.0' }).map(f => f.notice)).toEqual([]); }); test('framework', () => { @@ -239,8 +249,8 @@ describe(NoticesFilter, () => { // doesn't matter for this test because our data only has framework notices const cliVersion = '1.0.0'; - expect(NoticesFilter.filter({ data: notices, cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0') }).map(f => f.notice)).toEqual([]); - expect(NoticesFilter.filter({ data: notices, cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'built-with-1_144_0') }).map(f => f.notice)).toEqual([FRAMEWORK_2_1_0_AFFECTED_NOTICE]); + expect(noticesFilter.filter({ data: notices, cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0') }).map(f => f.notice)).toEqual([]); + expect(noticesFilter.filter({ data: notices, cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'built-with-1_144_0') }).map(f => f.notice)).toEqual([FRAMEWORK_2_1_0_AFFECTED_NOTICE]); }); test('module', () => { @@ -248,16 +258,15 @@ describe(NoticesFilter, () => { const cliVersion = '1.0.0'; // module-level match - expect(NoticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAYV2], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module') }).map(f => f.notice)).toEqual([NOTICE_FOR_APIGATEWAYV2]); + expect(noticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAYV2], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module') }).map(f => f.notice)).toEqual([NOTICE_FOR_APIGATEWAYV2]); // no apigatewayv2 in the tree - expect(NoticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAYV2], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0') }).map(f => f.notice)).toEqual([]); - + expect(noticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAYV2], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0') }).map(f => f.notice)).toEqual([]); // module name mismatch: apigateway != apigatewayv2 - expect(NoticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAY], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module') }).map(f => f.notice)).toEqual([]); + expect(noticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAY], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module') }).map(f => f.notice)).toEqual([]); // construct-level match - expect(NoticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAYV2_CFN_STAGE], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module') }).map(f => f.notice)).toEqual([NOTICE_FOR_APIGATEWAYV2_CFN_STAGE]); + expect(noticesFilter.filter({ data: [NOTICE_FOR_APIGATEWAYV2_CFN_STAGE], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module') }).map(f => f.notice)).toEqual([NOTICE_FOR_APIGATEWAYV2_CFN_STAGE]); }); test('module with pre-release version', () => { @@ -265,7 +274,7 @@ describe(NoticesFilter, () => { const cliVersion = '1.0.0'; // module-level match - expect(NoticesFilter.filter({ data: [NOTICES_FOR_IDENTITY_POOL], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module-pre-release-semver')}).map(f => f.notice)).toEqual([NOTICES_FOR_IDENTITY_POOL]); + expect(noticesFilter.filter({ data: [NOTICES_FOR_IDENTITY_POOL], cliVersion, bootstrappedEnvironments: [], outDir: path.join(__dirname, 'cloud-assembly-trees', 'experimental-module-pre-release-semver')}).map(f => f.notice)).toEqual([NOTICES_FOR_IDENTITY_POOL]); }); test('bootstrap', () => { @@ -303,7 +312,7 @@ describe(NoticesFilter, () => { }, ]; - const filtered = NoticesFilter.filter({ + const filtered = noticesFilter.filter({ data: [BASIC_BOOTSTRAP_NOTICE], cliVersion, outDir, @@ -318,7 +327,7 @@ describe(NoticesFilter, () => { const outDir = path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0'); const cliVersion = '1.0.0'; - expect(NoticesFilter.filter({ + expect(noticesFilter.filter({ data: [BASIC_BOOTSTRAP_NOTICE], cliVersion, outDir, @@ -331,7 +340,7 @@ describe(NoticesFilter, () => { const outDir = path.join(__dirname, 'cloud-assembly-trees', 'built-with-2_12_0'); const cliVersion = '1.0.0'; - const filtered = NoticesFilter.filter({ + const filtered = noticesFilter.filter({ data: [ { title: 'matchme', @@ -403,7 +412,7 @@ describe(NoticesFilter, () => { const cliVersion = '1.0.0'; // WHEN - const filtered = NoticesFilter.filter({ + const filtered = noticesFilter.filter({ data: [ { title: 'match', @@ -440,7 +449,7 @@ function parseTestComponent(x: string): Component { describe(WebsiteNoticeDataSource, () => { - const dataSource = new WebsiteNoticeDataSource(); + const dataSource = new WebsiteNoticeDataSource(ioHostEmitter); test('returns data when download succeeds', async () => { const result = await mockCall(200, { @@ -572,16 +581,13 @@ describe(CachedDataSource, () => { test('retrieves data from the delegate when the file cannot be read', async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-test')); try { - const debugSpy = jest.spyOn(logging, 'debug'); - const dataSource = dataSourceWithDelegateReturning(freshData, `${tmpDir}/does-not-exist.json`); const notices = await dataSource.fetch(); expect(notices).toEqual(freshData); - expect(debugSpy).not.toHaveBeenCalled(); + expect(ioHost.messages).toEqual([]); - debugSpy.mockRestore(); } finally { fs.rmSync(tmpDir, { recursive: true, force: true }); } @@ -604,7 +610,7 @@ describe(CachedDataSource, () => { const delegate = { fetch: jest.fn().mockRejectedValue(new Error('fetching failed')), }; - const dataSource = new CachedDataSource(fileName, delegate, true); + const dataSource = new CachedDataSource(ioHostEmitter, fileName, delegate, true); // WHEN const notices = await dataSource.fetch(); @@ -619,7 +625,7 @@ describe(CachedDataSource, () => { }; delegate.fetch.mockResolvedValue(notices); - return new CachedDataSource(file, delegate, ignoreCache); + return new CachedDataSource(ioHostEmitter, file, delegate, ignoreCache); } }); @@ -636,7 +642,7 @@ describe(Notices, () => { describe('addBootstrapVersion', () => { test('can add multiple values', async () => { - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ context: new Context(), ioHost }); notices.addBootstrappedEnvironment({ bootstrapStackVersion: 10, environment: { account: 'account', region: 'region', name: 'env' } }); notices.addBootstrappedEnvironment({ bootstrapStackVersion: 11, environment: { account: 'account', region: 'region', name: 'env' } }); @@ -644,15 +650,13 @@ describe(Notices, () => { dataSource: { fetch: async () => [BOOTSTRAP_NOTICE_V10, BOOTSTRAP_NOTICE_V11] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenCalledWith(new FilteredNotice(BOOTSTRAP_NOTICE_V10).format()); - expect(print).toHaveBeenCalledWith(new FilteredNotice(BOOTSTRAP_NOTICE_V11).format()); + ioHost.expectMessage({ containing: new FilteredNotice(BOOTSTRAP_NOTICE_V10).format() }); + ioHost.expectMessage({ containing: new FilteredNotice(BOOTSTRAP_NOTICE_V11).format() }); }); test('deduplicates', async () => { - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); notices.addBootstrappedEnvironment({ bootstrapStackVersion: 10, environment: { account: 'account', region: 'region', name: 'env' } }); notices.addBootstrappedEnvironment({ bootstrapStackVersion: 10, environment: { account: 'account', region: 'region', name: 'env' } }); @@ -661,7 +665,7 @@ describe(Notices, () => { notices.display(); - const filter = jest.spyOn(NoticesFilter, 'filter'); + const filter = jest.spyOn(NoticesFilter.prototype, 'filter'); notices.display(); expect(filter).toHaveBeenCalledTimes(1); @@ -686,36 +690,30 @@ describe(Notices, () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, BASIC_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenCalledWith(new FilteredNotice(BASIC_NOTICE).format()); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); }); test('clears notices if empty', async () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display({ showTotal: true }); - expect(print).toHaveBeenNthCalledWith(1, ''); - expect(print).toHaveBeenNthCalledWith(2, 'There are 0 unacknowledged notice(s).'); - expect(print).toHaveBeenCalledTimes(2); + ioHost.expectMessage({ containing: 'There are 0 unacknowledged notice(s).' }); }); test('doesnt throw', async () => { - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => { @@ -725,37 +723,20 @@ describe(Notices, () => { }); }); - test('does nothing when we shouldnt display', async () => { - let refreshCalled = false; - const notices = Notices.create({ context: new Context(), shouldDisplay: false }); - await notices.refresh({ - dataSource: { - fetch: async () => { - refreshCalled = true; - return Promise.resolve([]); - }, - }, - }); - - expect(refreshCalled).toBeFalsy(); - }); - test('filters out acknowledged notices by default', async () => { // within the affected version range of both notices jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.126.0'); const context = new Context({ bag: new Settings({ 'acknowledged-issue-numbers': [MULTIPLE_AFFECTED_VERSIONS_NOTICE.issueNumber] }) }); - const notices = Notices.create({ context }); + const notices = Notices.create({ ioHost, context }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenNthCalledWith(4, new FilteredNotice(BASIC_NOTICE).format()); - expect(print).toHaveBeenNthCalledWith(6, 'If you don’t want to see a notice anymore, use \"cdk acknowledge \". For example, \"cdk acknowledge 16603\".'); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); + ioHost.expectMessage({ containing: 'If you don’t want to see a notice anymore, use \"cdk acknowledge \". For example, \"cdk acknowledge 16603\".' }); }); test('preserves acknowledged notices if requested', async () => { @@ -764,16 +745,14 @@ describe(Notices, () => { const context = new Context({ bag: new Settings({ 'acknowledged-issue-numbers': [MULTIPLE_AFFECTED_VERSIONS_NOTICE.issueNumber] }) }); - const notices = Notices.create({ context, includeAcknowledged: true }); + const notices = Notices.create({ ioHost, context, includeAcknowledged: true }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenCalledWith(new FilteredNotice(BASIC_NOTICE).format()); - expect(print).toHaveBeenCalledWith(new FilteredNotice(MULTIPLE_AFFECTED_VERSIONS_NOTICE).format()); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); + ioHost.expectMessage({ containing: new FilteredNotice(MULTIPLE_AFFECTED_VERSIONS_NOTICE).format() }); }); }); @@ -782,103 +761,77 @@ describe(Notices, () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, BASIC_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenNthCalledWith(2, 'NOTICES (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)'); - expect(print).toHaveBeenNthCalledWith(6, 'If you don’t want to see a notice anymore, use \"cdk acknowledge \". For example, \"cdk acknowledge 16603\".'); + ioHost.expectMessage({ containing: 'NOTICES (What\'s this? https://github.com/aws/aws-cdk/wiki/CLI-Notices)' }); + ioHost.expectMessage({ containing: 'If you don’t want to see a notice anymore, use \"cdk acknowledge \". For example, \"cdk acknowledge 16603\".' }); }); test('deduplicates notices', async () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, BASIC_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - - notices.display(); - expect(print).toHaveBeenNthCalledWith(4, new FilteredNotice(BASIC_NOTICE).format()); - expect(print).toHaveBeenNthCalledWith(6, 'If you don’t want to see a notice anymore, use \"cdk acknowledge \". For example, \"cdk acknowledge 16603\".'); - }); - - test('does nothing when we shouldnt display', async () => { - const notices = Notices.create({ context: new Context(), shouldDisplay: false }); - await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE] } }); - - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenCalledTimes(0); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); + ioHost.expectMessage({ containing: 'If you don’t want to see a notice anymore, use \"cdk acknowledge \". For example, \"cdk acknowledge 16603\".' }); }); test('nothing when there are no notices', async () => { - const print = jest.spyOn(logging, 'info'); - - Notices.create({ context: new Context() }).display(); - expect(print).toHaveBeenCalledTimes(0); + Notices.create({ ioHost, context: new Context() }).display(); + expect(ioHost.messages).toEqual([]); }); test('total count when show total is true', async () => { - const print = jest.spyOn(logging, 'info'); - - Notices.create({ context: new Context() }).display({ showTotal: true }); - expect(print).toHaveBeenNthCalledWith(2, 'There are 0 unacknowledged notice(s).'); + Notices.create({ ioHost, context: new Context() }).display({ showTotal: true }); + ioHost.expectMessage({ containing: 'There are 0 unacknowledged notice(s).' }); }); test('warning', async () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_WARNING_NOTICE] }, }); - const warning = jest.spyOn(logging, 'warning'); - notices.display(); - expect(warning).toHaveBeenNthCalledWith(1, new FilteredNotice(BASIC_NOTICE).format()); - expect(warning).toHaveBeenCalledTimes(1); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format(), level: 'warn' }); }); test('error', async () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_ERROR_NOTICE] }, }); - const error = jest.spyOn(logging, 'error'); - notices.display(); - expect(error).toHaveBeenNthCalledWith(1, new FilteredNotice(BASIC_NOTICE).format()); - expect(error).toHaveBeenCalledTimes(1); + ioHost.expectMessage({ level: 'error', containing: new FilteredNotice(BASIC_NOTICE).format() }); }); test('only relevant notices', async () => { // within the affected version range of the notice jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.0.0'); - const notices = Notices.create({ context: new Context() }); + const notices = Notices.create({ ioHost, context: new Context() }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenNthCalledWith(4, new FilteredNotice(BASIC_NOTICE).format()); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); }); test('only unacknowledged notices', async () => { @@ -887,15 +840,13 @@ describe(Notices, () => { const context = new Context({ bag: new Settings({ 'acknowledged-issue-numbers': [MULTIPLE_AFFECTED_VERSIONS_NOTICE.issueNumber] }) }); - const notices = Notices.create({ context }); + const notices = Notices.create({ ioHost, context }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenNthCalledWith(4, new FilteredNotice(BASIC_NOTICE).format()); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); }); test('can include acknowledged notices if requested', async () => { @@ -903,16 +854,14 @@ describe(Notices, () => { jest.spyOn(version, 'versionNumber').mockImplementation(() => '1.126.0'); const context = new Context({ bag: new Settings({ 'acknowledged-issue-numbers': [MULTIPLE_AFFECTED_VERSIONS_NOTICE.issueNumber] }) }); - const notices = Notices.create({ context, includeAcknowledged: true }); + const notices = Notices.create({ ioHost, context, includeAcknowledged: true }); await notices.refresh({ dataSource: { fetch: async () => [BASIC_NOTICE, MULTIPLE_AFFECTED_VERSIONS_NOTICE] }, }); - const print = jest.spyOn(logging, 'info'); - notices.display(); - expect(print).toHaveBeenNthCalledWith(4, new FilteredNotice(BASIC_NOTICE).format()); - expect(print).toHaveBeenNthCalledWith(6, new FilteredNotice(MULTIPLE_AFFECTED_VERSIONS_NOTICE).format()); + ioHost.expectMessage({ containing: new FilteredNotice(BASIC_NOTICE).format() }); + ioHost.expectMessage({ containing: new FilteredNotice(MULTIPLE_AFFECTED_VERSIONS_NOTICE).format() }); }); }); }); diff --git a/packages/aws-cdk/test/util/fake-io-host.ts b/packages/aws-cdk/test/util/fake-io-host.ts new file mode 100644 index 000000000..44c685750 --- /dev/null +++ b/packages/aws-cdk/test/util/fake-io-host.ts @@ -0,0 +1,27 @@ +import { IIoHost, IoMessage, IoMessageLevel, IoRequest } from "../../lib/toolkit/cli-io-host"; + +export class FakeIoHost implements IIoHost { + public messages: Array> = []; + public requestResponse: (msg: IoRequest) => Promise; + + constructor() { + this.clear(); + } + + public clear() { + this.messages.splice(0, this.messages.length); + this.requestResponse = jest.fn().mockRejectedValue(new Error('requestResponse not mocked')); + } + + public async notify(msg: IoMessage): Promise { + this.messages.push(msg); + } + + public expectMessage(m: { containing: string, level?: IoMessageLevel }) { + expect(this.messages).toContainEqual(expect.objectContaining({ + ...m.level ? { level: m.level } : undefined, + // Can be a partial string as well + message: expect.stringContaining(m.containing), + })); + } +} \ No newline at end of file From d25fd89447e2852dfe9b4c98553f5b81aece6782 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 4 Mar 2025 12:55:43 +0100 Subject: [PATCH 4/4] Tests for the CliIoHost switching --- .../aws-cdk/test/toolkit/cli-io-host.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/aws-cdk/test/toolkit/cli-io-host.test.ts b/packages/aws-cdk/test/toolkit/cli-io-host.test.ts index e88951aaf..a0932ee1e 100644 --- a/packages/aws-cdk/test/toolkit/cli-io-host.test.ts +++ b/packages/aws-cdk/test/toolkit/cli-io-host.test.ts @@ -97,6 +97,38 @@ describe('CliIoHost', () => { }); }); + describe('notices stream selection', () => { + const NOTICES_MSG: IoMessage = { + time: new Date(), + level: 'info', + action: 'notices', + code: 'CDK_TOOLKIT_I0100', + message: 'MESSAGE', + }; + + test('can send notices to stdout', async () => { + ioHost.noticesDestination = 'stdout'; + await ioHost.notify(NOTICES_MSG); + // THEN + expect(mockStdout).toHaveBeenCalledWith(expect.stringContaining('MESSAGE')); + }); + + test('can send notices to stderr', async () => { + ioHost.noticesDestination = 'stderr'; + await ioHost.notify(NOTICES_MSG); + // THEN + expect(mockStderr).toHaveBeenCalledWith(expect.stringContaining('MESSAGE')); + }); + + test('can drop notices', async () => { + ioHost.noticesDestination = 'drop'; + await ioHost.notify(NOTICES_MSG); + // THEN + expect(mockStdout).not.toHaveBeenCalled(); + expect(mockStderr).not.toHaveBeenCalled(); + }); + }); + describe('message formatting', () => { beforeEach(() => { ioHost.isTTY = true;