fix(cli): write notices to stderr or don't write them at all #188
+435
−205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
rix0rrr
commented
Mar 4, 2025
| * | ||
| * @default true | ||
| */ | ||
| readonly shouldDisplay?: boolean; |
There was a problem hiding this comment.
Messages are now always emitted -- whether they are displayed or not is up to the IoHost.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #188 +/- ##
========================================
Coverage 85.02% 85.02%
========================================
Files 207 208 +1
Lines 35726 35919 +193
Branches 4605 4626 +21
========================================
+ Hits 30375 30540 +165
- Misses 5197 5232 +35
+ Partials 154 147 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On CI systems, the CDK CLI tries to avoid writing to
stderrbecause there are a couple of CI systems that are commonly configured to fail if any output is written tostderr. That means all output, like notices, must go tostdout.Some commands (like
cdk synthorcdk bootstrap --show-template) produce usable output onstdout, 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:
stderr, so that they will never interfere withstdoutanymore.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".
To make the existing
Noticescode play nice with theIoHost, I needed to do a couple of things:IoHostEmitter, to format messages that are sent to the IoHost. This converts strings or structured messages into anIoMessage.NoticesFilterclass a proper class with anioHostEmittermember.FakeIoHostclass for tests, to check for the messages we're emitting (as opposed to spying onstdoutor spying on theloggingmembers)Fixes aws/aws-cdk#33589.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license