diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry-with-errors.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry-with-errors.integtest.ts index 6b0631268..3f005dcf9 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry-with-errors.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry-with-errors.integtest.ts @@ -32,7 +32,6 @@ integTest( json: false, debug: false, staging: true, - notices: true, ['no-color']: false, ci: expect.anything(), // changes based on where this is called validation: true, @@ -81,7 +80,6 @@ integTest( json: false, debug: false, staging: true, - notices: true, ['no-color']: false, ci: expect.anything(), // changes based on where this is called validation: true, diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry.integtest.ts index b694a2d8e..577168125 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/synth/cdk-synth-telemetry.integtest.ts @@ -24,7 +24,6 @@ integTest( json: false, debug: false, staging: true, - notices: true, ['no-color']: false, ci: expect.anything(), // changes based on where this is called validation: true, @@ -74,7 +73,6 @@ integTest( json: false, debug: false, staging: true, - notices: true, ['no-color']: false, ci: expect.anything(), // changes based on where this is called validation: true, diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/notices/notices.ts b/packages/@aws-cdk/toolkit-lib/lib/api/notices/notices.ts index cd5bea6c1..5afb83df6 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/notices/notices.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/notices/notices.ts @@ -150,7 +150,7 @@ export class Notices { * Refresh the list of notices this instance is aware of. * * This method throws an error if the data source fails to fetch notices. - * When using, consider if execution should halt immdiately or if catching the rror and continuing is more appropriate + * When using, consider if execution should halt immdiately or if catching the error and continuing is more appropriate * * @throws on failure to refresh the data source */ diff --git a/packages/aws-cdk/lib/cli/cli-config.ts b/packages/aws-cdk/lib/cli/cli-config.ts index edfc827b8..d0951a957 100644 --- a/packages/aws-cdk/lib/cli/cli-config.ts +++ b/packages/aws-cdk/lib/cli/cli-config.ts @@ -39,7 +39,7 @@ export async function makeConfig(): Promise { 'role-arn': { type: 'string', alias: 'r', desc: 'ARN of Role to use when invoking CloudFormation', default: undefined, requiresArg: true }, 'staging': { type: 'boolean', desc: 'Copy assets to the output directory (use --no-staging to disable the copy of assets which allows local debugging via the SAM CLI to reference the original source files)', default: true }, 'output': { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true }, - 'notices': { type: 'boolean', desc: 'Show relevant notices', default: YARGS_HELPERS.shouldDisplayNotices() }, + 'notices': { type: 'boolean', desc: 'Show relevant notices' }, 'no-color': { type: 'boolean', desc: 'Removes colors and other style from console output', default: false }, 'ci': { type: 'boolean', desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr', default: YARGS_HELPERS.isCI() }, 'unstable': { type: 'array', desc: 'Opt in to unstable features. The flag indicates that the scope and API of a feature might still change. Otherwise the feature is generally production ready and fully supported. Can be specified multiple times.', default: [] }, diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts index 566167f56..f28cfba6b 100644 --- a/packages/aws-cdk/lib/cli/cli.ts +++ b/packages/aws-cdk/lib/cli/cli.ts @@ -4,6 +4,7 @@ import type { ChangeSetDeployment, DeploymentMethod, DirectDeployment } from '@a import { ToolkitError, Toolkit } from '@aws-cdk/toolkit-lib'; import * as chalk from 'chalk'; import { CdkToolkit, AssetBuildTime } from './cdk-toolkit'; +import { ciSystemIsStdErrSafe } from './ci-systems'; import { displayVersionMessage } from './display-version'; import type { IoMessageLevel } from './io-host'; import { CliIoHost } from './io-host'; @@ -33,6 +34,7 @@ import type { StackSelector, Synthesizer } from '../cxapp'; import { ProxyAgentProvider } from './proxy-agent'; import { cdkCliErrorName } from './telemetry/error'; import type { ErrorDetails } from './telemetry/schema'; +import { isCI } from './util/ci'; import { isDeveloperBuildVersion, versionWithBuild, versionNumber } from './version'; import { getLanguageFromAlias } from '../commands/language'; @@ -112,7 +114,34 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise): any { requiresArg: true, }) .option('notices', { - default: helpers.shouldDisplayNotices(), + default: undefined, type: 'boolean', desc: 'Show relevant notices', }) diff --git a/packages/aws-cdk/lib/cli/util/yargs-helpers.ts b/packages/aws-cdk/lib/cli/util/yargs-helpers.ts index 713547093..7cfb548a4 100644 --- a/packages/aws-cdk/lib/cli/util/yargs-helpers.ts +++ b/packages/aws-cdk/lib/cli/util/yargs-helpers.ts @@ -1,5 +1,3 @@ -import { ciSystemIsStdErrSafe } from '../ci-systems'; -import { isCI } from '../util/ci'; import { versionWithBuild } from '../version'; export { isCI } from '../util/ci'; @@ -48,16 +46,3 @@ export function browserForPlatform(): string { return 'xdg-open %u'; } } - -/** - * The default value for displaying (and refreshing) notices on all commands. - * - * If the user didn't supply either `--notices` or `--no-notices`, we do - * autodetection. The autodetection currently is: do write notices if we are - * not on CI, or are on a CI system where we know that writing to stderr is - * safe. We fail "closed"; that is, we decide to NOT print for unknown CI - * systems, even though technically we maybe could. - */ -export function shouldDisplayNotices(): boolean { - return !isCI() || Boolean(ciSystemIsStdErrSafe()); -} diff --git a/packages/aws-cdk/test/cli/cli-arguments.test.ts b/packages/aws-cdk/test/cli/cli-arguments.test.ts index cdf2b809a..1a8adfdc3 100644 --- a/packages/aws-cdk/test/cli/cli-arguments.test.ts +++ b/packages/aws-cdk/test/cli/cli-arguments.test.ts @@ -33,7 +33,7 @@ describe('yargs', () => { lookups: true, trace: undefined, unstable: [], - notices: expect.any(Boolean), + notices: undefined, output: undefined, }, deploy: { diff --git a/packages/aws-cdk/test/cli/cli.test.ts b/packages/aws-cdk/test/cli/cli.test.ts index 77f9f87d4..57bf6d824 100644 --- a/packages/aws-cdk/test/cli/cli.test.ts +++ b/packages/aws-cdk/test/cli/cli.test.ts @@ -1,3 +1,4 @@ +import { Notices } from '../../lib/api/notices'; import * as cdkToolkitModule from '../../lib/cli/cdk-toolkit'; import { exec } from '../../lib/cli/cli'; import { CliIoHost } from '../../lib/cli/io-host'; @@ -31,35 +32,46 @@ const actualUserConfig = jest.requireActual('../../lib/cli/user-configuration'); Configuration.fromArgs = jest.fn().mockImplementation(() => actualUserConfig.Configuration.fromArgs(ioHelper)); Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => actualUserConfig.Configuration.fromArgs(ioHelper)); -jest.mock('../../lib/api/notices', () => ({ - Notices: { - create: jest.fn().mockReturnValue({ - refresh: jest.fn().mockResolvedValue(undefined), - display: jest.fn(), - }), - }, -})); - jest.mock('../../lib/cli/parse-command-line-arguments', () => ({ parseCommandLineArguments: jest.fn().mockImplementation((args) => { + let result = {}; + + // Handle commands if (args.includes('version')) { - return Promise.resolve({ - _: ['version'], - verbose: args.includes('-v') - ? args.filter((arg: string) => arg === '-v').length - : args.includes('--verbose') - ? parseInt(args[args.indexOf('--verbose') + 1]) || true - : undefined, - }); - } - if (args.includes('migrate')) { - return Promise.resolve({ + result = { ...result, _: ['version'] }; + } else if (args.includes('migrate')) { + result = { + ...result, '_': ['migrate'], 'language': 'typescript', 'stack-name': 'sampleStack', - }); + }; + + // Handle language aliases for migrate command + if (args.includes('ts')) { + result = { ...result, language: 'typescript' }; + } } - return Promise.resolve({ _: [] }); + + // Handle notices flags + if (args.includes('--notices')) { + result = { ...result, notices: true }; + } else if (args.includes('--no-notices')) { + result = { ...result, notices: false }; + } + + // Handle verbose flags + const verboseCount = args.filter((arg: string) => arg === '-v').length; + if (verboseCount > 0) { + result = { ...result, verbose: verboseCount }; + } + + const verboseIndex = args.findIndex((arg: string) => arg === '--verbose'); + if (verboseIndex !== -1 && args[verboseIndex + 1]) { + result = { ...result, verbose: parseInt(args[verboseIndex + 1], 10) }; + } + + return Promise.resolve(result); }), })); @@ -111,14 +123,307 @@ describe('exec verbose flag tests', () => { }); }); -test('should convert language alias to full language name', async () => { - const migrateSpy = jest.spyOn(cdkToolkitModule.CdkToolkit.prototype, 'migrate').mockResolvedValue(); +describe('notices configuration tests', () => { + let mockNoticesCreate: jest.SpyInstance; + + beforeEach(() => { + jest.clearAllMocks(); + + // Mock the Notices.create method + mockNoticesCreate = jest.spyOn(Notices, 'create').mockReturnValue({ + refresh: jest.fn().mockResolvedValue(undefined), + display: jest.fn(), + } as any); + + // Set up version module for our tests + jest.mock('../../lib/cli/version', () => ({ + ...originalVersion, + DISPLAY_VERSION: 'test-version', + displayVersionMessage: jest.fn().mockResolvedValue(undefined), + })); + }); - await exec(['migrate', '--language', 'ts', '--stack-name', 'sampleStack']); + afterEach(() => { + mockNoticesCreate.mockRestore(); + // Restore the version module to its original state + jest.resetModules(); + jest.setMock('../../lib/cli/version', originalVersion); + }); + + test('should send notices to "stderr" when passing --notices flag in CLI', async () => { + await exec(['--notices', 'version']); - expect(migrateSpy).toHaveBeenCalledWith( - expect.objectContaining({ - language: 'typescript', - }), - ); + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: 'stderr', + }), + }), + ); + }); + + test('should send notices to "drop" when passing --no-notices in CLI', async () => { + await exec(['--no-notices', 'version']); + + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: 'drop', + }), + }), + ); + }); + + test('should send notices to "drop" when notices: false in settings and no CLI flag is provided', async () => { + // Mock configuration to return notices: false + const mockConfig = { + loadConfigFiles: jest.fn().mockResolvedValue(undefined), + settings: { + get: jest.fn().mockImplementation((key: string[]) => { + if (key[0] === 'notices') return false; + return undefined; + }), + }, + context: { + get: jest.fn().mockReturnValue([]), + }, + }; + + (Configuration as any).mockImplementation(() => mockConfig); + Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig); + + await exec(['version']); + + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: 'drop', + }), + }), + ); + }); + + test.each([ + { + envVar: 'TEAMCITY_VERSION', + description: 'TeamCity', + scenarios: [ + { + name: 'should send notices to "drop" by default when no settings or CLI flags are provided', + configNotices: undefined, + cliArgs: ['version'], + expectedDestination: 'drop', + }, + { + name: 'should send notices to "stderr" when config setting notices=true', + configNotices: true, + cliArgs: ['version'], + expectedDestination: 'stderr', + }, + { + name: 'should send notices to "stderr" when passing --notices CLI flag', + configNotices: undefined, + cliArgs: ['--notices', 'version'], + expectedDestination: 'stderr', + }, + { + name: 'should send notices to "drop" when passing --no-notices CLI flag, even when config has notices=true', + configNotices: true, + cliArgs: ['--no-notices', 'version'], + expectedDestination: 'drop', + }, + ], + }, + { + envVar: 'TF_BUILD', + description: 'Azure DevOps', + scenarios: [ + { + name: 'should send notices to "drop" when no settings or CLI flags are provided', + configNotices: undefined, + cliArgs: ['version'], + expectedDestination: 'drop', + }, + { + name: 'should send notices to "stderr" config setting notices=true', + configNotices: true, + cliArgs: ['version'], + expectedDestination: 'stderr', + }, + { + name: 'should send notices to "stderr" --notices CLI flag', + configNotices: undefined, + cliArgs: ['--notices', 'version'], + expectedDestination: 'stderr', + }, + { + name: 'should send notices to "drop" when passing --no-notices CLI flag, even when config has notices=true', + configNotices: true, + cliArgs: ['--no-notices', 'version'], + expectedDestination: 'drop', + }, + ], + }, + ])('CI environment with $description', async ({ envVar, scenarios }) => { + for (const scenario of scenarios) { + // Store original environment variables + const originalCI = process.env.CI; + const originalEnvVar = process.env[envVar]; + + // Set CI environment variables + process.env.CI = '1'; + process.env[envVar] = '1'; + + try { + // Mock configuration + const mockConfig = { + loadConfigFiles: jest.fn().mockResolvedValue(undefined), + settings: { + get: jest.fn().mockImplementation((key: string[]) => { + if (key[0] === 'notices') return scenario.configNotices; + return undefined; + }), + }, + context: { + get: jest.fn().mockReturnValue([]), + }, + }; + + (Configuration as any).mockImplementation(() => mockConfig); + Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig); + + await exec(scenario.cliArgs); + + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: scenario.expectedDestination, + }), + }), + ); + } finally { + // Restore original environment variables + if (originalCI !== undefined) { + process.env.CI = originalCI; + } else { + delete process.env.CI; + } + if (originalEnvVar !== undefined) { + process.env[envVar] = originalEnvVar; + } else { + delete process.env[envVar]; + } + } + } + }); + + test('should read notices=true setting from configuration', async () => { + // Mock configuration to return notices: true + const mockConfig = { + loadConfigFiles: jest.fn().mockResolvedValue(undefined), + settings: { + get: jest.fn().mockImplementation((key: string) => { + if (key[0] === 'notices') return true; + return undefined; + }), + }, + context: { + get: jest.fn().mockReturnValue([]), + }, + }; + + (Configuration as any).mockImplementation(() => mockConfig); + Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig); + + await exec(['version']); + + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: 'stderr', + }), + }), + ); + }); + + test('should send notices to "drop" when passing --no-notices in CLI and config set to notices: false', async () => { + // Mock configuration to return notices: true, but CLI flag should override + const mockConfig = { + loadConfigFiles: jest.fn().mockResolvedValue(undefined), + settings: { + get: jest.fn().mockImplementation((key: string) => { + if (key[0] === 'notices') return true; + return undefined; + }), + }, + context: { + get: jest.fn().mockReturnValue([]), + }, + }; + + (Configuration as any).mockImplementation(() => mockConfig); + Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig); + + await exec(['--no-notices', 'version']); + + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: 'drop', + }), + }), + ); + }); + + test.each([ + { value: undefined, expected: 'stderr', description: 'undefined (autodetection)' }, + { value: false, expected: 'drop', description: 'boolean false' }, + { value: true, expected: 'stderr', description: 'boolean true' }, + // support string "false" as false + { value: 'false', expected: 'drop', description: 'string "false"' }, + { value: 'truthy', expected: 'stderr', description: 'string "truthy"' }, + { value: 0, expected: 'drop', description: 'numeric 0' }, + { value: 1, expected: 'stderr', description: 'numeric 1' }, + { value: '', expected: 'drop', description: 'empty string' }, + { value: null, expected: 'drop', description: 'null' }, + ])('should send notices to "$expected" config value: $description', async ({ value, expected }) => { + // Mock configuration to return the test value + const mockConfig = { + loadConfigFiles: jest.fn().mockResolvedValue(undefined), + settings: { + get: jest.fn().mockImplementation((key: string[]) => { + if (key[0] === 'notices') return value; + return undefined; + }), + }, + context: { + get: jest.fn().mockReturnValue([]), + }, + }; + + (Configuration as any).mockImplementation(() => mockConfig); + Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig); + + await exec(['version']); + + expect(mockNoticesCreate).toHaveBeenCalledWith( + expect.objectContaining({ + ioHost: expect.objectContaining({ + noticesDestination: expected, + }), + }), + ); + }); + + test('should convert language alias to full language name', async () => { + const migrateSpy = jest.spyOn(cdkToolkitModule.CdkToolkit.prototype, 'migrate').mockResolvedValue(); + + await exec(['migrate', '--language', 'ts', '--stack-name', 'sampleStack']); + + expect(migrateSpy).toHaveBeenCalledWith( + expect.objectContaining({ + language: 'typescript', + }), + ); + }); });