Skip to content

Commit

Permalink
refactor(toolkit): new message level result (#33234)
Browse files Browse the repository at this point in the history
### Reason for this change

The CDK CLI has certain semantic rules around which messages are written to which output stream (`stdout` or `stderr`). These rules are mostly generalizable based on the level of a message. However until now there was one major exception: The main results of a CLI command should always be written to `stdout`, regardless of level. In practice these messages were always attributed to the `info` level.

With the recent refactorings towards a `CliIoHost`, we needed to include an additional property `forceStdout` on `IoMessages`. This always was a bad crutch and unintended to stay.

### Description of changes

Removal of the `forceStdout` message property, in favor of a new explicit message level: `result`.
In terms of priority, a `result` is lower than an `error`, but higher than a `warn`. This is intuitive: A user that wants to ignore all warnings, will still want to see results.

Use result in the CLU and the new Toolkit class.

### Describe any new or updated permissions being added

n/a

### Description of how you validated changes

Existing and extended tests.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
mrgrain authored Jan 30, 2025
1 parent c40a9e2 commit 3b2846e
Show file tree
Hide file tree
Showing 14 changed files with 125 additions and 128 deletions.
3 changes: 2 additions & 1 deletion packages/@aws-cdk/toolkit/lib/api/io/io-message.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { ToolkitAction } from '../../toolkit';
* The reporting level of the message.
* All messages are always reported, it's up to the IoHost to decide what to log.
*/
export type IoMessageLevel = 'error' | 'warn' | 'info' | 'debug' | 'trace';
export type IoMessageLevel = 'error'| 'result' | 'warn' | 'info' | 'debug' | 'trace';

/**
* Valid reporting categories for messages.
Expand Down Expand Up @@ -89,6 +89,7 @@ const levels = [
'debug',
'info',
'warn',
'result',
'error',
] as const;

Expand Down
31 changes: 17 additions & 14 deletions packages/@aws-cdk/toolkit/lib/api/io/private/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export const prompt = <T, U>(code: VALID_CODE, message: string, defaultResponse:
};

/**
* Logs an error level message.
* Creates an error level message.
*/
export const error = <T>(message: string, code?: VALID_CODE, payload?: T) => {
return formatMessage({
Expand All @@ -76,34 +76,37 @@ export const error = <T>(message: string, code?: VALID_CODE, payload?: T) => {
};

/**
* Logs an warning level message.
* Creates a result level message and represents the most important message for a given action.
*
* They should be used sparsely, with an action usually having no or exactly one result.
* However actions that operate on Cloud Assemblies might include a result per Stack.
* Unlike other messages, results must always have a code and a payload.
*/
export const warn = <T>(message: string, code?: VALID_CODE, payload?: T) => {
export const result = <T>(message: string, code: VALID_CODE, payload: T) => {
return formatMessage({
level: 'warn',
level: 'result',
code,
message,
data: payload,
});
};

/**
* Logs an info level message.
* Creates a warning level message.
*/
export const info = <T>(message: string, code?: VALID_CODE, payload?: T) => {
export const warn = <T>(message: string, code?: VALID_CODE, payload?: T) => {
return formatMessage({
level: 'info',
level: 'warn',
code,
message,
data: payload,
});
};

/**
* Logs an info level message to stdout.
* @deprecated
* Creates an info level message.
*/
export const data = <T>(message: string, code?: VALID_CODE, payload?: T) => {
export const info = <T>(message: string, code?: VALID_CODE, payload?: T) => {
return formatMessage({
level: 'info',
code,
Expand All @@ -113,7 +116,7 @@ export const data = <T>(message: string, code?: VALID_CODE, payload?: T) => {
};

/**
* Logs a debug level message.
* Creates a debug level message.
*/
export const debug = <T>(message: string, code?: VALID_CODE, payload?: T) => {
return formatMessage({
Expand All @@ -125,7 +128,7 @@ export const debug = <T>(message: string, code?: VALID_CODE, payload?: T) => {
};

/**
* Logs a trace level message.
* Creates a trace level message.
*/
export const trace = <T>(message: string, code?: VALID_CODE, payload?: T) => {
return formatMessage({
Expand All @@ -137,7 +140,7 @@ export const trace = <T>(message: string, code?: VALID_CODE, payload?: T) => {
};

/**
* Logs an info level success message in green text.
* Creates an info level success message in green text.
* @deprecated
*/
export const success = <T>(message: string, code?: VALID_CODE, payload?: T) => {
Expand All @@ -150,7 +153,7 @@ export const success = <T>(message: string, code?: VALID_CODE, payload?: T) => {
};

/**
* Logs an info level message in bold text.
* Creates an info level message in bold text.
* @deprecated
*/
export const highlight = <T>(message: string, code?: VALID_CODE, payload?: T) => {
Expand Down
10 changes: 5 additions & 5 deletions packages/@aws-cdk/toolkit/lib/toolkit/toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { CachedCloudAssemblySource, IdentityCloudAssemblySource, StackAssembly,
import { ALL_STACKS, CloudAssemblySourceBuilder } from '../api/cloud-assembly/private';
import { ToolkitError } from '../api/errors';
import { IIoHost, IoMessageCode, IoMessageLevel } from '../api/io';
import { asSdkLogger, withAction, Timer, confirm, error, highlight, info, success, warn, ActionAwareIoHost, debug } from '../api/io/private';
import { asSdkLogger, withAction, Timer, confirm, error, highlight, info, success, warn, ActionAwareIoHost, debug, result } from '../api/io/private';

/**
* The current action being performed by the CLI. 'none' represents the absence of an action.
Expand Down Expand Up @@ -149,7 +149,7 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
const firstStack = stacks.firstStack!;
const template = firstStack.template;
const obscuredTemplate = obscureTemplate(template);
await ioHost.notify(info(message, 'CDK_TOOLKIT_I0001', {
await ioHost.notify(result(message, 'CDK_TOOLKIT_I0001', {
...assemblyData,
stack: {
stackName: firstStack.stackName,
Expand All @@ -161,7 +161,7 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
}));
} else {
// not outputting template to stdout, let's explain things to the user a little bit...
await ioHost.notify(success(message, 'CDK_TOOLKIT_I0002', assemblyData));
await ioHost.notify(result(chalk.green(message), 'CDK_TOOLKIT_I0002', assemblyData));
await ioHost.notify(info(`Supply a stack id (${stacks.stackArtifacts.map((s) => chalk.green(s.hierarchicalId)).join(', ')}) to display its template.`));
}

Expand Down Expand Up @@ -650,15 +650,15 @@ export class Toolkit extends CloudAssemblySourceBuilder implements AsyncDisposab
const startRollbackTime = Timer.start();
const deployments = await this.deploymentsForAction('rollback');
try {
const result = await deployments.rollbackStack({
const stackResult = await deployments.rollbackStack({
stack,
roleArn: options.roleArn,
toolkitStackName: this.toolkitStackName,
force: options.orphanFailedResources,
validateBootstrapStackVersion: options.validateBootstrapStackVersion,
orphanLogicalIds: options.orphanLogicalIds,
});
if (!result.notInRollbackableState) {
if (!stackResult.notInRollbackableState) {
anyRollbackable = true;
}
const elapsedRollbackTime = startRollbackTime.end();
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/toolkit/test/actions/synth.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe('synth', () => {
// THEN
expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
action: 'synth',
level: 'info',
level: 'result',
message: expect.stringContaining('Successfully synthesized'),
}));
});
Expand All @@ -31,7 +31,7 @@ describe('synth', () => {
// THEN
expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
action: 'synth',
level: 'info',
level: 'result',
message: expect.stringContaining('Successfully synthesized'),
}));
});
Expand All @@ -44,7 +44,7 @@ describe('synth', () => {
// THEN
expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
action: 'synth',
level: 'info',
level: 'result',
code: 'CDK_TOOLKIT_I0001',
message: expect.stringContaining('Successfully synthesized'),
data: expect.objectContaining({
Expand All @@ -65,7 +65,7 @@ describe('synth', () => {
// THEN
expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({
action: 'synth',
level: 'info',
level: 'result',
code: 'CDK_TOOLKIT_I0002',
message: expect.stringContaining('Successfully synthesized'),
data: expect.objectContaining({
Expand Down
12 changes: 7 additions & 5 deletions packages/@aws-cdk/toolkit/test/api/io/io-message.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import { isMessageRelevantForLevel } from '../../../lib/api/io/io-message';

describe('IoMessageLevel', () => {
test.each`
msgLevel | logLevel | isRelevant
${'error'} | ${'trace'} | ${true}
${'info'} | ${'trace'} | ${true}
${'info'} | ${'warn'} | ${false}
${'trace'} | ${'error'} | ${false}
msgLevel | logLevel | isRelevant
${'error'} | ${'trace'} | ${true}
${'info'} | ${'trace'} | ${true}
${'result'} | ${'warn'} | ${true}
${'info'} | ${'warn'} | ${false}
${'trace'} | ${'error'} | ${false}
${'warn'} | ${'result'} | ${false}
`('with msgLevel=$msgLevel and logLevel=$msgLevel, logging should be $shouldLog', async ({ msgLevel, logLevel, isRelevant }) => {
expect(isMessageRelevantForLevel({ level: msgLevel }, logLevel)).toBe(isRelevant);
});
Expand Down
8 changes: 4 additions & 4 deletions packages/aws-cdk/lib/cli/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import {
import { printSecurityDiff, printStackDiff, RequireApproval } from '../diff';
import { ResourceImporter, removeNonImportResources } from '../import';
import { listStacks } from '../list-stacks';
import { data, debug, error, highlight, info, success, warning } from '../logging';
import { result as logResult, debug, error, highlight, info, success, warning } from '../logging';
import { ResourceMigrator } from '../migrator';
import { deserializeStructure, obscureTemplate, serializeStructure } from '../serialize';
import { CliIoHost } from '../toolkit/cli-io-host';
Expand Down Expand Up @@ -526,7 +526,7 @@ export class CdkToolkit {

info('Stack ARN:');

data(deployResult.stackArn);
logResult(deployResult.stackArn);
} catch (e: any) {
// It has to be exactly this string because an integration test tests for
// "bold(stackname) failed: ResourceNotReady: <error>"
Expand Down Expand Up @@ -891,7 +891,7 @@ export class CdkToolkit {

// just print stack IDs
for (const stack of stacks) {
data(stack.id);
logResult(stack.id);
}

return 0; // exit-code
Expand Down Expand Up @@ -1275,7 +1275,7 @@ export class CdkToolkit {
* Print a serialized object (YAML or JSON) to stdout.
*/
function printSerializedObject(obj: any, json: boolean) {
data(serializeStructure(obj, json));
logResult(serializeStructure(obj, json));
}

export interface DiffOptions {
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk/lib/cli/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { docs } from '../commands/docs';
import { doctor } from '../commands/doctor';
import { getMigrateScanType } from '../commands/migrate';
import { cliInit, printAvailableTemplates } from '../init';
import { data, debug, error, info } from '../logging';
import { result, debug, error, info } from '../logging';
import { Notices } from '../notices';
import { Command, Configuration } from './user-configuration';
import { IoMessageLevel, CliIoHost } from '../toolkit/cli-io-host';
Expand Down Expand Up @@ -490,7 +490,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
});
case 'version':
ioHost.currentAction = 'version';
return data(version.DISPLAY_VERSION);
return result(version.DISPLAY_VERSION);

default:
throw new ToolkitError('Unknown command: ' + command);
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk/lib/commands/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { minimatch } from 'minimatch';
import { Context } from '../api/context';
import { PROJECT_CONFIG, PROJECT_CONTEXT, USER_DEFAULTS } from '../cli/user-configuration';
import * as version from '../cli/version';
import { error, warning, info, data } from '../logging';
import { error, warning, info, result } from '../logging';
import { ToolkitError } from '../toolkit/error';
import { renderTable } from '../util';

Expand Down Expand Up @@ -58,7 +58,7 @@ export async function contextHandler(options: ContextOptions): Promise<number> {
if (options.json) {
/* istanbul ignore next */
const contextValues = options.context.all;
data(JSON.stringify(contextValues, undefined, 2));
result(JSON.stringify(contextValues, undefined, 2));
} else {
listContext(options.context);
}
Expand Down
30 changes: 14 additions & 16 deletions packages/aws-cdk/lib/logging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { IoMessageLevel, IoMessage, CliIoHost, IoMessageSpecificCode, IoMessageC
*/
function formatMessageAndLog(
level: IoMessageLevel,
forceStdout: boolean,
input: LogInput<IoCodeLevel>,
style?: (str: string) => string,
...args: unknown[]
Expand All @@ -32,7 +31,6 @@ function formatMessageAndLog(
level,
message: finalMessage,
code,
forceStdout,
};

void ioHost.notify(ioMessage);
Expand Down Expand Up @@ -75,7 +73,7 @@ type LogInput<L extends IoCodeLevel> = string | LogParams<L>;
* ```
*/
export const error = (input: LogInput<'E'>, ...args: unknown[]) => {
return formatMessageAndLog('error', false, input, undefined, ...args);
return formatMessageAndLog('error', input, undefined, ...args);
};

/**
Expand All @@ -90,7 +88,7 @@ export const error = (input: LogInput<'E'>, ...args: unknown[]) => {
* ```
*/
export const warning = (input: LogInput<'W'>, ...args: unknown[]) => {
return formatMessageAndLog('warn', false, input, undefined, ...args);
return formatMessageAndLog('warn', input, undefined, ...args);
};

/**
Expand All @@ -105,22 +103,22 @@ export const warning = (input: LogInput<'W'>, ...args: unknown[]) => {
* ```
*/
export const info = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('info', false, input, undefined, ...args);
return formatMessageAndLog('info', input, undefined, ...args);
};

/**
* Logs an info level message to stdout.
* Logs an result. In the CLI, this always goes to stdout.
*
* Can be used in multiple ways:
* ```ts
* data(`${JSON.stringify(stats)}`) // infers default info code `CDK_TOOLKIT_I000`
* data('{"count": %d}', count) // infers default info code `CDK_TOOLKIT_I000`
* data({ message: 'stats: %j', code: 'CDK_DATA_I001' }) // specifies info code `CDK_DATA_I001`
* data({ message: 'stats: %j', code: 'CDK_DATA_I001' }, stats) // specifies info code `CDK_DATA_I001`
* result(`${JSON.stringify(stats)}`) // infers default info code `CDK_TOOLKIT_I000`
* result('{"count": %d}', count) // infers default info code `CDK_TOOLKIT_I000`
* result({ message: 'stats: %j', code: 'CDK_DATA_I001' }) // specifies info code `CDK_DATA_I001`
* result({ message: 'stats: %j', code: 'CDK_DATA_I001' }, stats) // specifies info code `CDK_DATA_I001`
* ```
*/
export const data = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('info', true, input, undefined, ...args);
export const result = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('result', input, undefined, ...args);
};

/**
Expand All @@ -135,7 +133,7 @@ export const data = (input: LogInput<'I'>, ...args: unknown[]) => {
* ```
*/
export const debug = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('debug', false, input, undefined, ...args);
return formatMessageAndLog('debug', input, undefined, ...args);
};

/**
Expand All @@ -150,7 +148,7 @@ export const debug = (input: LogInput<'I'>, ...args: unknown[]) => {
* ```
*/
export const trace = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('trace', false, input, undefined, ...args);
return formatMessageAndLog('trace', input, undefined, ...args);
};

/**
Expand All @@ -165,7 +163,7 @@ export const trace = (input: LogInput<'I'>, ...args: unknown[]) => {
* ```
*/
export const success = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('info', false, input, chalk.green, ...args);
return formatMessageAndLog('info', input, chalk.green, ...args);
};

/**
Expand All @@ -180,5 +178,5 @@ export const success = (input: LogInput<'I'>, ...args: unknown[]) => {
* ```
*/
export const highlight = (input: LogInput<'I'>, ...args: unknown[]) => {
return formatMessageAndLog('info', false, input, chalk.bold, ...args);
return formatMessageAndLog('info', input, chalk.bold, ...args);
};
Loading

0 comments on commit 3b2846e

Please sign in to comment.