Skip to content

Commit

Permalink
fix(cli): standard log messages are sent to stderr when CI=true (#20957)
Browse files Browse the repository at this point in the history
The CLI currently sends almost all logs to `stderr`, even success
messages. Based on the linked issue, this was done to make terminal
colors easier to see. While this might make it easier for users that are
running the CLI from their local machine, it causes issues on some CI
systems that will exit if anything is written to `stderr` (even when the
exit code is 0).

This PR updates all of the logging mechanisms to recognize the `ci`
argument. If `--ci` is passed, or the environment variable `CI=true`
then all logs (except for error logs) will be sent to `stdout`.
Currently the `ci` argument was only available on the `deploy` command,
but this PR moves that to the global arguments list so that it will
apply to all commands.

I tested this manually on a CDK app by using a script to capture
`stderr` and `stdout`.

```bash

export CI=true
key="$1"

cmd="npx cdk deploy"

errlog=$(mktemp)
stdlog=$(mktemp)
$cmd 1>> "$stdlog" 2> "$errlog"

echo "-------------------errlog---------------------"
cat "$errlog"
echo "-------------------stdlog---------------------"
cat "$stdlog"
rm -f "$errlog"
rm -f "$stdlog"
```

I also added new unit and integration tests that validate the change.

fixes #7717


----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
corymhall authored Jul 4, 2022
1 parent 1061e4f commit 277340d
Show file tree
Hide file tree
Showing 10 changed files with 111 additions and 9 deletions.
13 changes: 13 additions & 0 deletions packages/aws-cdk/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ Command | Description
[`cdk acknowledge`](#cdk-acknowledge) | Acknowledge (and hide) a notice by issue number
[`cdk notices`](#cdk-notices) | List all relevant notices for the application

- [Bundling](#bundling)
- [MFA Support](#mfa-support)
- [SSO Support](#sso-support)
- [Configuration](#configuration)
- [Running in CI](#running-in-ci)


This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project.

## Commands
Expand Down Expand Up @@ -714,3 +721,9 @@ The following environment variables affect aws-cdk:

- `CDK_DISABLE_VERSION_CHECK`: If set, disable automatic check for newer versions.
- `CDK_NEW_BOOTSTRAP`: use the modern bootstrapping stack.

### Running in CI

The CLI will attempt to detect whether it is being run in CI by looking for the presence of an
environment variable `CI=true`. This can be forced by passing the `--ci` flag. By default the CLI
sends most of its logs to `stderr`, but when `ci=true` it will send the logs to `stdout` instead.
2 changes: 2 additions & 0 deletions packages/aws-cdk/lib/api/cloudformation-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ export interface DestroyStackOptions {
roleArn?: string;
quiet?: boolean;
force?: boolean;
ci?: boolean;
}

export interface StackExistsOptions {
Expand Down Expand Up @@ -382,6 +383,7 @@ export class CloudFormationDeployments {
stack: options.stack,
deployName: options.deployName,
quiet: options.quiet,
ci: options.ci,
});
}

Expand Down
6 changes: 5 additions & 1 deletion packages/aws-cdk/lib/api/deploy-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ async function prepareAndExecuteChangeSet(
resourcesTotal: cloudFormationStack.exists ? changeSetLength + 1 : changeSetLength,
progress: options.progress,
changeSetCreationTime: changeSetDescription.CreationTime,
ci: options.ci,
}).start();
debug('Execution of changeset %s on stack %s has started; waiting for the update to complete...', changeSet.Id, deployName);
try {
Expand Down Expand Up @@ -497,6 +498,7 @@ export interface DestroyStackOptions {
roleArn?: string;
deployName?: string;
quiet?: boolean;
ci?: boolean;
}

export async function destroyStack(options: DestroyStackOptions) {
Expand All @@ -507,7 +509,9 @@ export async function destroyStack(options: DestroyStackOptions) {
if (!currentStack.exists) {
return;
}
const monitor = options.quiet ? undefined : StackActivityMonitor.withDefaultPrinter(cfn, deployName, options.stack).start();
const monitor = options.quiet ? undefined : StackActivityMonitor.withDefaultPrinter(cfn, deployName, options.stack, {
ci: options.ci,
}).start();

try {
await cfn.deleteStack({ StackName: deployName, RoleARN: options.roleArn }).promise();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export class StackActivityMonitor {
cfn: aws.CloudFormation,
stackName: string,
stackArtifact: cxapi.CloudFormationStackArtifact, options: WithDefaultPrinterProps = {}) {
const stream = process.stderr;
const stream = options.ci ? process.stdout : process.stderr;

const props: PrinterProps = {
resourceTypeColumnWidth: calcMaxResourceTypeLength(stackArtifact.template),
Expand Down
9 changes: 9 additions & 0 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ export class CdkToolkit {
force: true,
roleArn: options.roleArn,
fromDeploy: true,
ci: options.ci,
});
}
continue;
Expand Down Expand Up @@ -460,6 +461,7 @@ export class CdkToolkit {
stack,
deployName: stack.stackName,
roleArn: options.roleArn,
ci: options.ci,
});
success(`\n ✅ %s: ${action}ed`, chalk.blue(stack.displayName));
} catch (e) {
Expand Down Expand Up @@ -980,6 +982,13 @@ export interface DestroyOptions {
* Whether the destroy request came from a deploy.
*/
fromDeploy?: boolean

/**
* Whether we are on a CI system
*
* @default false
*/
readonly ci?: boolean;
}

/**
Expand Down
10 changes: 8 additions & 2 deletions packages/aws-cdk/lib/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import { realHandler as docs } from '../lib/commands/docs';
import { realHandler as doctor } from '../lib/commands/doctor';
import { RequireApproval } from '../lib/diff';
import { availableInitLanguages, cliInit, printAvailableTemplates } from '../lib/init';
import { data, debug, error, print, setLogLevel } from '../lib/logging';
import { data, debug, error, print, setLogLevel, setCI } from '../lib/logging';
import { displayNotices, refreshNotices } from '../lib/notices';
import { Command, Configuration, Settings } from '../lib/settings';
import * as version from '../lib/version';
Expand Down Expand Up @@ -79,6 +79,7 @@ async function parseCommandLineArguments() {
.option('output', { type: 'string', alias: 'o', desc: 'Emits the synthesized cloud assembly into a directory (default: cdk.out)', requiresArg: true })
.option('notices', { type: 'boolean', desc: 'Show relevant notices' })
.option('no-color', { type: 'boolean', desc: 'Removes colors and other style from console output', default: false })
.option('ci', { type: 'boolean', desc: 'Force CI detection. If CI=true then logs will be sent to stdout instead of stderr', default: process.env.CI !== undefined })
.command(['list [STACKS..]', 'ls [STACKS..]'], 'Lists all stacks in the app', (yargs: Argv) => yargs
.option('long', { type: 'boolean', default: false, alias: 'l', desc: 'Display environment information for each stack' }),
)
Expand Down Expand Up @@ -108,7 +109,6 @@ async function parseCommandLineArguments() {
.option('build-exclude', { type: 'array', alias: 'E', nargs: 1, desc: 'Do not rebuild asset with the given ID. Can be specified multiple times', default: [] })
.option('exclusively', { type: 'boolean', alias: 'e', desc: 'Only deploy requested stacks, don\'t include dependencies' })
.option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'What security-sensitive changes need manual approval' })
.option('ci', { type: 'boolean', desc: 'Force CI detection', default: process.env.CI !== undefined })
.option('notification-arns', { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', nargs: 1, requiresArg: true })
// @deprecated(v2) -- tags are part of the Cloud Assembly and tags specified here will be overwritten on the next deployment
.option('tags', { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly (deprecated)', nargs: 1, requiresArg: true })
Expand Down Expand Up @@ -269,6 +269,10 @@ async function initCommandLine() {
if (argv.verbose) {
setLogLevel(argv.verbose);
}

if (argv.ci) {
setCI(true);
}
debug('CDK toolkit version:', version.DISPLAY_VERSION);
debug('Command line arguments:', argv);

Expand Down Expand Up @@ -413,6 +417,7 @@ async function initCommandLine() {
contextLines: args.contextLines,
securityOnly: args.securityOnly,
fail: args.fail || !enableDiffNoFail,
stream: args.ci ? process.stdout : undefined,
});

case 'bootstrap':
Expand Down Expand Up @@ -514,6 +519,7 @@ async function initCommandLine() {
exclusively: args.exclusively,
force: args.force,
roleArn: args.roleArn,
ci: args.ci,
});

case 'synthesize':
Expand Down
14 changes: 10 additions & 4 deletions packages/aws-cdk/lib/logging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,30 @@ export enum LogLevel {


export let logLevel = LogLevel.DEFAULT;
export let CI = false;

export function setLogLevel(newLogLevel: LogLevel) {
logLevel = newLogLevel;
}

export function setCI(newCI: boolean) {
CI = newCI;
}

export function increaseVerbosity() {
logLevel += 1;
}

const _debug = logger(stderr, [chalk.gray]);
const stream = () => CI ? stdout : stderr;
const _debug = (fmt: string, ...args: any) => logger(stream(), [chalk.gray])(fmt, ...args);

export const trace = (fmt: string, ...args: any) => logLevel >= LogLevel.TRACE && _debug(fmt, ...args);
export const debug = (fmt: string, ...args: any[]) => logLevel >= LogLevel.DEBUG && _debug(fmt, ...args);
export const error = logger(stderr, [chalk.red]);
export const warning = logger(stderr, [chalk.yellow]);
export const success = logger(stderr, [chalk.green]);
export const highlight = logger(stderr, [chalk.bold]);
export const print = logger(stderr);
export const success = (fmt: string, ...args: any) => logger(stream(), [chalk.green])(fmt, ...args);
export const highlight = (fmt: string, ...args: any) => logger(stream(), [chalk.bold])(fmt, ...args);
export const print = (fmt: string, ...args: any) => logger(stream())(fmt, ...args);
export const data = logger(stdout);

export type LoggerFunction = (fmt: string, ...args: any[]) => void;
Expand Down
25 changes: 25 additions & 0 deletions packages/aws-cdk/test/api/deploy-stack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { deployStack, DeployStackOptions, ToolkitInfo } from '../../lib/api';
import { tryHotswapDeployment } from '../../lib/api/hotswap-deployments';
import { DEFAULT_FAKE_TEMPLATE, testStack } from '../util';
import { MockedObject, mockResolvedEnvironment, MockSdk, MockSdkProvider, SyncHandlerSubsetOf } from '../util/mock-sdk';
import { setCI } from '../../lib/logging';

jest.mock('../../lib/api/hotswap-deployments');

Expand Down Expand Up @@ -29,9 +30,13 @@ const FAKE_STACK_TERMINATION_PROTECTION = testStack({
let sdk: MockSdk;
let sdkProvider: MockSdkProvider;
let cfnMocks: MockedObject<SyncHandlerSubsetOf<AWS.CloudFormation>>;
let stderrMock: jest.SpyInstance;
let stdoutMock: jest.SpyInstance;

beforeEach(() => {
jest.resetAllMocks();
stderrMock = jest.spyOn(process.stderr, 'write').mockImplementation(() => { return true; });
stdoutMock = jest.spyOn(process.stdout, 'write').mockImplementation(() => { return true; });

sdkProvider = new MockSdkProvider();
sdk = new MockSdk();
Expand Down Expand Up @@ -193,6 +198,26 @@ test('reuse previous parameters if requested', async () => {
}));
});

describe('ci=true', () => {
beforeEach(() => {
setCI(true);
});
afterEach(() => {
setCI(false);
});
test('output written to stdout', async () => {
// GIVEN

await deployStack({
...standardDeployStackArguments(),
});

// THEN
expect(stderrMock.mock.calls).toEqual([]);
expect(stdoutMock.mock.calls).not.toEqual([]);
});
});

test('do not reuse previous parameters if not requested', async () => {
// GIVEN
givenStackExists({
Expand Down
27 changes: 27 additions & 0 deletions packages/aws-cdk/test/integ/cli/cli.integtest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,33 @@ import { integTest } from '../helpers/test-helpers';

jest.setTimeout(600_000);

describe('ci', () => {
integTest('output to stderr', withDefaultFixture(async (fixture) => {
const deployOutput = await fixture.cdkDeploy('test-2', { captureStderr: true, onlyStderr: true });
const diffOutput = await fixture.cdk(['diff', fixture.fullStackName('test-2')], { captureStderr: true, onlyStderr: true });
const destroyOutput = await fixture.cdkDestroy('test-2', { captureStderr: true, onlyStderr: true });
expect(deployOutput).not.toEqual('');
expect(destroyOutput).not.toEqual('');
expect(diffOutput).not.toEqual('');
}));
describe('ci=true', () => {
beforeEach(() => {
process.env.CI = 'true';
});
afterEach(() => {
process.env.CI = undefined;
});
integTest('output to stdout', withDefaultFixture(async (fixture) => {
const deployOutput = await fixture.cdkDeploy('test-2', { captureStderr: true, onlyStderr: true });
const diffOutput = await fixture.cdk(['diff', fixture.fullStackName('test-2')], { captureStderr: true, onlyStderr: true });
const destroyOutput = await fixture.cdkDestroy('test-2', { captureStderr: true, onlyStderr: true });
expect(deployOutput).toEqual('');
expect(destroyOutput).toEqual('');
expect(diffOutput).toEqual('');
}));
});
});

integTest('VPC Lookup', withDefaultFixture(async (fixture) => {
fixture.log('Making sure we are clean before starting.');
await fixture.cdkDestroy('define-vpc', { modEnv: { ENABLE_VPC_TESTING: 'DEFINE' } });
Expand Down
12 changes: 11 additions & 1 deletion packages/aws-cdk/test/integ/helpers/cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,14 @@ export interface ShellOptions extends child_process.SpawnOptions {
* Pass output here
*/
output?: NodeJS.WritableStream;

/**
* Only return stderr. For example, this is used to validate
* that when CI=true, all logs are sent to stdout.
*
* @default false
*/
onlyStderr?: boolean;
}

export interface CdkCliOptions extends ShellOptions {
Expand Down Expand Up @@ -629,7 +637,9 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom
child.once('error', reject);

child.once('close', code => {
const output = (Buffer.concat(stdout).toString('utf-8') + Buffer.concat(stderr).toString('utf-8')).trim();
const stderrOutput = Buffer.concat(stderr).toString('utf-8');
const stdoutOutput = Buffer.concat(stdout).toString('utf-8');
const output = (options.onlyStderr ? stderrOutput : stdoutOutput + stderrOutput).trim();
if (code === 0 || options.allowErrExit) {
resolve(output);
} else {
Expand Down

0 comments on commit 277340d

Please sign in to comment.