fix(cli): cdk list pollutes stdout with synthesis time in CI#1663
fix(cli): cdk list pollutes stdout with synthesis time in CI#1663sai-ray wants to merge 9 commits into
cdk list pollutes stdout with synthesis time in CI#1663Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| // In CI, info-level output (incl. the synth-time line, I1000) routes to stdout next to the | ||
| // listing and breaks parsers. With --json, stdout is an explicit machine-readable contract. | ||
| // In both cases, suppress I1000 so stdout carries only the listing. | ||
| if (options.json || this.ioHost.isCI) { |
There was a problem hiding this comment.
Just disable them for any case please. Let us not get confused by reports saying this happens in CI. It happens everywhere.
There was a problem hiding this comment.
Converted to draft while I rework this.
The current approach raises the log level to warn during listing. It works, but it's pretty blunt, it mutates shared ioHost state and ends up swallowing other info output during the call too.
So I was thinking of giving the "Including dependency stacks" line its own message code. Then I'd suppress that and the synth-time line by code on the list path. Feels self contained and matches what we already do. Does that sound right to you, would you go about it differently?
| // listing and breaks parsers. With --json, stdout is an explicit machine-readable contract. | ||
| // In both cases, suppress I1000 so stdout carries only the listing. | ||
| if (options.json || this.ioHost.isCI) { | ||
| this.ioHost.once(IO.CDK_TOOLKIT_I1000, () => ({ preventDefault: true })); |
There was a problem hiding this comment.
I am CERTAIN there is a second message we need to silence as well.
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
mrgrain
left a comment
mrgrain
left a comment
There was a problem hiding this comment.
I struggle understanding what we are trying to achieve here. Can you please structure the PR to show relevant screenshots of the pre #1603 output (have one for every mode you are interested in) and than include screenshots of today's output.
This way it will be abundantly clear which messages got introduced and how this PR is fixing it.
| this.ioHost.once(IO.CDK_TOOLKIT_I1000, () => ({ preventDefault: true })); | ||
| // Only the listing should reach stdout; drop info status lines (synth time, dependency notes). | ||
| const previousLevel = this.ioHost.logLevel; | ||
| this.ioHost.logLevel = 'warn'; |
There was a problem hiding this comment.
this doesn't seem like the right approach
4 participants
cdk lsprints status lines like✨ Synthesis time: X.XXsandIncluding dependency stacks: ...alongside the stack listing. In CI these go to stdout, next to the listing, which breaks scripts that parse the output (issue aws/aws-cdk#38165).#1637 tried to fix this by suppressing the synth-time message under
--json, but plaincdk lsin CI was still affected, and the dependency line was never handled.These status lines are info-level. While listing, the log level is raised to
warnso only the listing (aresultmessage) and errors get through. This applies in all cases, not just CI or--json.Tests
warnduring listing and restored afterwards, including when listing throws.cdk lsandcdk ls <stack-with-dependency>in CI produce only the stack listing, no status lines.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license