Skip to content

Commit be378de

Browse files
rix0rrrgithub-actions
and
github-actions
authored
fix(cli): write notices to stderr or don't write them at all (#221)
(Replaces #188). 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 aws/aws-cdk#33589. Also in this PR, because this grew. * Introduce `IoDefaultMessages` in the CLI package, which helps migrate "legacy" logging code to just emit default warning/info/etc messages to the IoHost. * Removed the ability to log with a `{ message: 'asdf' }` object to the global logger functions. This wasn't being used anywhere other than tests, and it's sort of pointless: if you know the code you should be using the `MessageMaker` to make a message object; if you don't know the code you can emit a string. There is no need to look up the right code given a level and a message object. * Make it possible for result types to be any type, not just object types. This is necessary to cover the "result" from legacy logging, where the result is just a string. * Updated many tests in a test file (`cli-io-host.test.ts`) that failed type checking, but succeeded running, and therefore didn't fail the build of #220. * Centralized `TestIoHost` into the helper package, and renamed it to `MockIoHost`. * Introducing a `FakeIoHost` in the CLI package to assert on messages emitted to an `IoHost`. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license --------- Signed-off-by: github-actions <[email protected]> Co-authored-by: github-actions <[email protected]>
1 parent fe143da commit be378de

34 files changed

+590
-409
lines changed
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
export * from './cloud-assembly';
22
export * from './io';
33
export * from './toolkit-error';
4+
export * from './require-approval';

packages/@aws-cdk/tmp-toolkit-helpers/src/api/io/private/index.ts

+2
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,5 @@ export * from './span';
44
export * from './message-maker';
55
export * from './messages';
66
export * from './types';
7+
export * from './io-default-messages';
8+
export * from './testing';
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
import * as util from 'util';
2+
import type { ActionLessMessage, ActionLessRequest, IoHelper } from './io-helper';
3+
import type { IoMessageMaker } from './message-maker';
4+
import { IO } from './messages';
5+
6+
/**
7+
* Helper class to emit standard log messages to an IoHost
8+
*
9+
* It wraps an `IoHelper`, and adds convenience methods to emit default messages
10+
* for the various log levels.
11+
*/
12+
export class IoDefaultMessages {
13+
constructor(private readonly ioHelper: IoHelper) {
14+
}
15+
16+
public notify(msg: ActionLessMessage<unknown>): Promise<void> {
17+
return this.ioHelper.notify(msg);
18+
}
19+
20+
public requestResponse<T, U>(msg: ActionLessRequest<T, U>): Promise<U> {
21+
return this.ioHelper.requestResponse(msg);
22+
}
23+
24+
public error(input: string, ...args: unknown[]) {
25+
this.emitMessage(IO.DEFAULT_TOOLKIT_ERROR, input, ...args);
26+
}
27+
28+
public warn(input: string, ...args: unknown[]) {
29+
this.emitMessage(IO.DEFAULT_TOOLKIT_WARN, input, ...args);
30+
}
31+
32+
public warning(input: string, ...args: unknown[]) {
33+
this.emitMessage(IO.DEFAULT_TOOLKIT_WARN, input, ...args);
34+
}
35+
36+
public info(input: string, ...args: unknown[]) {
37+
this.emitMessage(IO.DEFAULT_TOOLKIT_INFO, input, ...args);
38+
}
39+
40+
public debug(input: string, ...args: unknown[]) {
41+
this.emitMessage(IO.DEFAULT_TOOLKIT_DEBUG, input, ...args);
42+
}
43+
44+
public trace(input: string, ...args: unknown[]) {
45+
this.emitMessage(IO.DEFAULT_TOOLKIT_TRACE, input, ...args);
46+
}
47+
48+
public result(input: string, ...args: unknown[]) {
49+
const message = args.length > 0 ? util.format(input, ...args) : input;
50+
// This is just the default "info" message but with a level of "result"
51+
void this.ioHelper.notify({
52+
time: new Date(),
53+
code: IO.DEFAULT_TOOLKIT_INFO.code,
54+
level: 'result',
55+
message,
56+
data: undefined,
57+
});
58+
}
59+
60+
private emitMessage(maker: IoMessageMaker<void>, input: string, ...args: unknown[]) {
61+
// Format message if args are provided
62+
const message = args.length > 0 ? util.format(input, ...args) : input;
63+
void this.ioHelper.notify(maker.msg(message));
64+
}
65+
}

packages/@aws-cdk/tmp-toolkit-helpers/src/api/io/private/message-maker.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ export const debug = <T = AbsentData>(details: CodeInfoMaybeInterface<T>) => mes
100100
export const info = <T = AbsentData>(details: CodeInfoMaybeInterface<T>) => message<T>('info', details);
101101
export const warn = <T = AbsentData>(details: CodeInfoMaybeInterface<T>) => message<T>('warn', details);
102102
export const error = <T = AbsentData>(details: CodeInfoMaybeInterface<T>) => message<T>('error', details);
103-
export const result = <T extends object = ImpossibleType>(details: Required<CodeInfo>) => message<T extends object ? T : undefined>('result', details);
103+
export const result = <T extends object = ImpossibleType>(details: Required<CodeInfo>) => message<T>('result', details);
104104

105105
interface RequestInfo<U> extends CodeInfo {
106106
readonly defaultResponse: U;

packages/@aws-cdk/tmp-toolkit-helpers/src/api/io/private/messages.ts

+26
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,14 @@ export const IO = {
3535
code: 'CDK_TOOLKIT_W0000',
3636
description: 'Default warning messages emitted from the Toolkit',
3737
}),
38+
DEFAULT_TOOLKIT_ERROR: make.error({
39+
code: 'CDK_TOOLKIT_E0000',
40+
description: 'Default error messages emitted from the Toolkit',
41+
}),
42+
DEFAULT_TOOLKIT_TRACE: make.trace({
43+
code: 'CDK_TOOLKIT_I0000',
44+
description: 'Default trace messages emitted from the Toolkit',
45+
}),
3846

3947
// 1: Synth (1xxx)
4048
CDK_TOOLKIT_I1000: make.info<Duration>({
@@ -331,6 +339,24 @@ export const IO = {
331339
interface: 'ErrorPayload',
332340
}),
333341

342+
// Notices
343+
CDK_TOOLKIT_I0100: make.info({
344+
code: 'CDK_TOOLKIT_I0100',
345+
description: 'Notices decoration (the header or footer of a list of notices)',
346+
}),
347+
CDK_TOOLKIT_W0101: make.warn({
348+
code: 'CDK_TOOLKIT_W0101',
349+
description: 'A notice that is marked as a warning',
350+
}),
351+
CDK_TOOLKIT_E0101: make.error({
352+
code: 'CDK_TOOLKIT_E0101',
353+
description: 'A notice that is marked as an error',
354+
}),
355+
CDK_TOOLKIT_I0101: make.info({
356+
code: 'CDK_TOOLKIT_I0101',
357+
description: 'A notice that is marked as informational',
358+
}),
359+
334360
// Assembly codes
335361
CDK_ASSEMBLY_I0010: make.debug({
336362
code: 'CDK_ASSEMBLY_I0010',
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
import type { IIoHost } from '../../io-host';
2+
import type { IoMessage, IoMessageLevel, IoRequest } from '../../io-message';
3+
4+
/**
5+
* An implementation of `IIoHost` that records messages and lets you assert on what was logged
6+
*
7+
* It's like `TestIoHost`, but comes with a predefined implementation for `notify`
8+
* that appends all messages to an in-memory array, and comes with a helper function
9+
* `expectMessage()` to test for the existence of a function in that array.
10+
*
11+
* Has a public mock for `requestResponse` that you configure like any
12+
* other mock function.
13+
*
14+
* # How to use
15+
*
16+
* Either create a new instance of this class for every test, or call `clear()`
17+
* on it between runs.
18+
*/
19+
export class FakeIoHost implements IIoHost {
20+
public messages: Array<IoMessage<unknown>> = [];
21+
public requestResponse!: <T, U>(msg: IoRequest<T, U>) => Promise<U>;
22+
23+
constructor() {
24+
this.clear();
25+
}
26+
27+
public clear() {
28+
this.messages.splice(0, this.messages.length);
29+
this.requestResponse = jest.fn().mockRejectedValue(new Error('requestResponse not mocked'));
30+
}
31+
32+
public async notify(msg: IoMessage<unknown>): Promise<void> {
33+
this.messages.push(msg);
34+
}
35+
36+
public expectMessage(m: { containing: string; level?: IoMessageLevel }) {
37+
expect(this.messages).toContainEqual(expect.objectContaining({
38+
...m.level ? { level: m.level } : undefined,
39+
// Can be a partial string as well
40+
message: expect.stringContaining(m.containing),
41+
}));
42+
}
43+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
export * from './test-io-host';
2+
export * from './fake-io-host';

packages/@aws-cdk/toolkit-lib/test/_helpers/test-io-host.ts renamed to packages/@aws-cdk/tmp-toolkit-helpers/src/api/io/private/testing/test-io-host.ts

+14-6
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,19 @@
1-
import type { IIoHost, IoMessage, IoMessageLevel, IoRequest } from '../../lib';
2-
import { RequireApproval } from '../../lib';
3-
import { isMessageRelevantForLevel } from '../../lib/api/shared-private';
1+
import { RequireApproval } from '../../../require-approval';
2+
import type { IIoHost } from '../../io-host';
3+
import type { IoMessage, IoMessageLevel, IoRequest } from '../../io-message';
4+
import { isMessageRelevantForLevel } from '../level-priority';
45

56
/**
6-
* A test implementation of IIoHost that does nothing but can by spied on.
7-
* Optionally set a level to filter out all irrelevant messages.
8-
* Optionally set a approval level.
7+
* A test implementation of IIoHost that does nothing but can be spied on.
8+
*
9+
* Includes a level to filter out irrelevant messages, defaults to `info`.
10+
*
11+
* Optionally set an approval level for code `CDK_TOOLKIT_I5060`.
12+
*
13+
* # How to use
14+
*
15+
* Configure and reset the `notifySpy` and `requestSpy` members as you would any
16+
* mock function.
917
*/
1018
export class TestIoHost implements IIoHost {
1119
public readonly notifySpy: jest.Mock<any, any, any>;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/**
2+
* @deprecated
3+
*/
4+
export enum RequireApproval {
5+
/**
6+
* Never require any security approvals
7+
*/
8+
NEVER = 'never',
9+
/**
10+
* Any security changes require an approval
11+
*/
12+
ANY_CHANGE = 'any-change',
13+
/**
14+
* Require approval only for changes that are access broadening
15+
*/
16+
BROADENING = 'broadening',
17+
}

packages/@aws-cdk/toolkit-lib/docs/message-registry.md

+6
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ group: Documents
99
| `CDK_TOOLKIT_I0000` | Default info messages emitted from the Toolkit | `info` | n/a |
1010
| `CDK_TOOLKIT_I0000` | Default debug messages emitted from the Toolkit | `debug` | n/a |
1111
| `CDK_TOOLKIT_W0000` | Default warning messages emitted from the Toolkit | `warn` | n/a |
12+
| `CDK_TOOLKIT_E0000` | Default error messages emitted from the Toolkit | `error` | n/a |
13+
| `CDK_TOOLKIT_I0000` | Default trace messages emitted from the Toolkit | `trace` | n/a |
1214
| `CDK_TOOLKIT_I1000` | Provides synthesis times. | `info` | {@link Duration} |
1315
| `CDK_TOOLKIT_I1001` | Cloud Assembly synthesis is starting | `trace` | {@link StackSelectionDetails} |
1416
| `CDK_TOOLKIT_I1901` | Provides stack data | `result` | {@link StackAndAssemblyData} |
@@ -64,6 +66,10 @@ group: Documents
6466
| `CDK_TOOLKIT_I9100` | Bootstrap progress | `info` | {@link BootstrapEnvironmentProgress} |
6567
| `CDK_TOOLKIT_I9900` | Bootstrap results on success | `result` | [cxapi.Environment](https://docs.aws.amazon.com/cdk/api/v2/docs/@aws-cdk_cx-api.Environment.html) |
6668
| `CDK_TOOLKIT_E9900` | Bootstrap failed | `error` | {@link ErrorPayload} |
69+
| `CDK_TOOLKIT_I0100` | Notices decoration (the header or footer of a list of notices) | `info` | n/a |
70+
| `CDK_TOOLKIT_W0101` | A notice that is marked as a warning | `warn` | n/a |
71+
| `CDK_TOOLKIT_E0101` | A notice that is marked as an error | `error` | n/a |
72+
| `CDK_TOOLKIT_I0101` | A notice that is marked as informational | `info` | n/a |
6773
| `CDK_ASSEMBLY_I0010` | Generic environment preparation debug messages | `debug` | n/a |
6874
| `CDK_ASSEMBLY_W0010` | Emitted if the found framework version does not support context overflow | `warn` | n/a |
6975
| `CDK_ASSEMBLY_I0042` | Writing updated context | `debug` | {@link UpdatedContext} |

packages/@aws-cdk/toolkit-lib/lib/actions/deploy/index.ts

+1-18
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { BaseDeployOptions } from './private/deploy-options';
22
import type { Tag } from '../../api/aws-cdk';
3+
import type { RequireApproval } from '../../api/shared-public';
34

45
export type DeploymentMethod = DirectDeploymentMethod | ChangeSetDeploymentMethod;
56

@@ -48,24 +49,6 @@ export enum AssetBuildTime {
4849
JUST_IN_TIME = 'just-in-time',
4950
}
5051

51-
/**
52-
* @deprecated
53-
*/
54-
export enum RequireApproval {
55-
/**
56-
* Never require any security approvals
57-
*/
58-
NEVER = 'never',
59-
/**
60-
* Any security changes require an approval
61-
*/
62-
ANY_CHANGE = 'any-change',
63-
/**
64-
* Require approval only for changes that are access broadening
65-
*/
66-
BROADENING = 'broadening',
67-
}
68-
6952
export enum HotswapMode {
7053
/**
7154
* Will fall back to CloudFormation when a non-hotswappable change is detected

packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type { ToolkitServices } from './private';
77
import { assemblyFromSource } from './private';
88
import type { BootstrapEnvironments, BootstrapOptions } from '../actions/bootstrap';
99
import { BootstrapSource } from '../actions/bootstrap';
10-
import { AssetBuildTime, type DeployOptions, RequireApproval } from '../actions/deploy';
10+
import { AssetBuildTime, type DeployOptions } from '../actions/deploy';
1111
import { type ExtendedDeployOptions, buildParameterMap, createHotswapPropertyOverrides, removePublishedAssets } from '../actions/deploy/private';
1212
import { type DestroyOptions } from '../actions/destroy';
1313
import { determinePermissionType } from '../actions/diff/private';
@@ -28,7 +28,7 @@ import { IO, SPAN, asSdkLogger, withoutColor, withoutEmojis, withTrimmedWhitespa
2828
import type { IoHelper } from '../api/shared-private';
2929
import { asIoHelper } from '../api/shared-private';
3030
import type { AssemblyData, StackDetails, ToolkitAction } from '../api/shared-public';
31-
import { ToolkitError } from '../api/shared-public';
31+
import { RequireApproval, ToolkitError } from '../api/shared-public';
3232
import { obscureTemplate, serializeStructure, validateSnsTopicArn, formatTime, formatErrorMessage } from '../private/util';
3333
import { pLimit } from '../util/concurrency';
3434

packages/@aws-cdk/toolkit-lib/test/_helpers/index.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { AssemblyDirectoryProps, Toolkit } from '../../lib';
44
import { ToolkitError } from '../../lib';
55
import { determineOutputDirectory } from '../../lib/api/cloud-assembly/private';
66

7-
export * from './test-io-host';
7+
export * from '../../lib/api/shared-private';
88
export * from './test-cloud-assembly-source';
99

1010
function fixturePath(...parts: string[]): string {
+82
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
interface CiSystem {
2+
/**
3+
* What's the name?
4+
*/
5+
readonly name: string;
6+
7+
/**
8+
* What environment variable indicates that we are running on this system?
9+
*/
10+
readonly detectEnvVar: string;
11+
12+
/**
13+
* Whether or not this CI system can be configured to fail on messages written to stderr
14+
*
15+
* With "can be configured", what we mean is that a checkbox or configuration
16+
* flag to enable this behavior comes out of the box with the CI system and (judgement
17+
* call), this flag is "commonly" used.
18+
*
19+
* Of course every CI system can be scripted to have this behavior, but that's
20+
* not what we mean.
21+
*/
22+
readonly canBeConfiguredToFailOnStdErr: boolean;
23+
}
24+
25+
const CI_SYSTEMS: CiSystem[] = [
26+
{
27+
name: 'Azure DevOps',
28+
// https://learn.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml
29+
detectEnvVar: 'TF_BUILD',
30+
canBeConfiguredToFailOnStdErr: true,
31+
},
32+
{
33+
name: 'TeamCity',
34+
// https://www.jetbrains.com/help/teamcity/predefined-build-parameters.html
35+
detectEnvVar: 'TEAMCITY_VERSION',
36+
// Can be configured to fail on stderr, when using a PowerShell task
37+
canBeConfiguredToFailOnStdErr: true,
38+
},
39+
{
40+
name: 'GitHub Actions',
41+
// https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#default-environment-variables
42+
detectEnvVar: 'GITHUB_ACTION',
43+
canBeConfiguredToFailOnStdErr: false,
44+
},
45+
{
46+
name: 'CodeBuild',
47+
// https://docs.aws.amazon.com/codebuild/latest/userguide/build-env-ref-env-vars.html
48+
detectEnvVar: 'CODEBUILD_BUILD_ID',
49+
canBeConfiguredToFailOnStdErr: false,
50+
},
51+
{
52+
name: 'CircleCI',
53+
// https://circleci.com/docs/variables/#built-in-environment-variables
54+
detectEnvVar: 'CIRCLECI',
55+
canBeConfiguredToFailOnStdErr: false,
56+
},
57+
{
58+
name: 'Jenkins',
59+
// https://www.jenkins.io/doc/book/pipeline/jenkinsfile/#using-environment-variables
60+
detectEnvVar: 'EXECUTOR_NUMBER',
61+
canBeConfiguredToFailOnStdErr: false,
62+
},
63+
];
64+
65+
export function detectCiSystem(): CiSystem | undefined {
66+
for (const ciSystem of CI_SYSTEMS) {
67+
if (process.env[ciSystem.detectEnvVar]) {
68+
return ciSystem;
69+
}
70+
}
71+
return undefined;
72+
}
73+
74+
/**
75+
* Return whether the CI system we're detecting is safe to write to stderr on
76+
*
77+
* Returns `undefined` if the current CI system cannot be recognized.
78+
*/
79+
export function ciSystemIsStdErrSafe(): boolean | undefined {
80+
const x = detectCiSystem()?.canBeConfiguredToFailOnStdErr;
81+
return x === undefined ? undefined : !x;
82+
}

0 commit comments

Comments
 (0)