Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): standard log messages are sent to stderr when CI=true #20957

Merged
merged 2 commits into from
Jul 4, 2022
Merged

fix(cli): standard log messages are sent to stderr when CI=true #20957

merged 2 commits into from
Jul 4, 2022

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Jul 1, 2022

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.

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:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • 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

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
@gitpod-io
Copy link

gitpod-io bot commented Jul 1, 2022

@github-actions github-actions bot added bug This issue is a bug. effort/large Large work item – several weeks of effort p1 labels Jul 1, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team July 1, 2022 19:23
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 1, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 4, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: afdb3d8
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 277340d into aws:main Jul 4, 2022
@mergify
Copy link
Contributor

mergify bot commented Jul 4, 2022

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

daschaa pushed a commit to daschaa/aws-cdk that referenced this pull request Jul 9, 2022
…20957)

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 aws#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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/large Large work item – several weeks of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Standard CDK log messages are written to the StandardError stream
3 participants