refactor(cli): stack/security diff no longer prints directly to a stream #264

kaizencc · 2025-03-19T22:27:49Z

part of https://github.com/orgs/aws/projects/257

this refactor removes direct prints to stderr or stdout for the diff cli command. the diff result will instead be printed through the global CliIoHost.

the diff format is determined by the Formatter class, which takes in a stream that previously was stdout (or stderr). now, we are sending our own stream into Formatter, capturing what was previously immediately printed and instead returning it as a string. we then print the resulting string through CliIoHost. it should have no functional change to how the diff cli works today.

this effort will help support diff in the toolkit lib because it allows the IoHost governance of the diff print.

as this is a pure refactor, successful existing tests is enough to ensure that this works.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

packages/aws-cdk/lib/commands/diff.ts

packages/aws-cdk/test/commands/diff.test.ts

mrgrain · 2025-03-21T14:15:52Z

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

+        diffs = numberFromBool(printSecurityDiff(template, stacks.firstStack, RequireApproval.Broadening, quiet));
+      } else {
+        const { stackDiffCount, printableStackDiff } = printStackDiff(template, stacks.firstStack, strict, contextLines, quiet, undefined, undefined, false);


I'd strongly advise you rename these functions since they don't print anything anymore.

mrgrain

This doesn't touch printSecurityDiff yet, right?

packages/aws-cdk/lib/commands/diff.ts

mrgrain · 2025-03-21T19:03:23Z

packages/aws-cdk/lib/api/deployments/cfn-api.ts

 import type { Deployments } from './deployments';
 import { ToolkitError } from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api';
 import { IO, type IoHelper } from '../../../../@aws-cdk/tmp-toolkit-helpers/src/api/io/private';
+import { info } from '../../cli/messages';


codecov-commenter · 2025-03-21T19:07:29Z

Codecov Report

Attention: Patch coverage is 73.48066% with 48 lines in your changes missing coverage. Please review.

Project coverage is 85.24%. Comparing base (ee0d87d) to head (c9463d0).

Files with missing lines	Patch %	Lines
packages/aws-cdk/lib/cli/cdk-toolkit.ts	48.97%	25 Missing ⚠️
packages/aws-cdk/lib/commands/diff.ts	85.15%	19 Missing ⚠️
packages/aws-cdk/lib/api/deployments/cfn-api.ts	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #264      +/-   ##
==========================================
+ Coverage   85.18%   85.24%   +0.05%     
==========================================
  Files         221      221              
  Lines       36512    36606      +94     
  Branches     4445     4450       +5     
==========================================
+ Hits        31104    31205     +101     
+ Misses       5307     5306       -1     
+ Partials      101       95       -6

Flag	Coverage Δ
suite.unit	`85.24% <73.48%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Fixes #302 In #264, we capture the output of the Formatter into a stream, and that is then converted to a string. This diff only gets printed if the diff isn't empty, but by some quirk `formatStackDiff` used to print the stack name _outside_ of the stream, thus creating the (correct) behavior. #264 made sure that `format` doesn't print, so we need to make sure that the consumer of `formatStackDiff` prints the relevant info. This PR makes sure that the stack name is returned as the formatted diff even if the actual diff is empty. We will print the right information when we print the formatted diff now. I also made similar modifications to `formatSecurityDiff`, as we should move away from `format` methods printing anything at all. BEFORE (cdk 2.1006.0): <img width="326" alt="Screenshot 2025-04-02 at 9 04 04 AM" src="https://github.com/user-attachments/assets/03b92517-e475-4c27-a4c1-52217be824c8" /> AFTER (my local cdk): <img width="322" alt="Screenshot 2025-04-02 at 9 03 11 AM" src="https://github.com/user-attachments/assets/53a7d3e0-2e62-45ca-898b-93c923028346" /> --- 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 <github-actions@github.com> Co-authored-by: github-actions <github-actions@github.com>

refactor(cli): diff no longer prints directly to a stream

24f68da

kaizencc had a problem deploying to integ-approval March 19, 2025 22:27 — with GitHub Actions Failure

aws-cdk-automation requested a review from a team March 19, 2025 22:28

github-actions bot added p2 labels Mar 19, 2025

kaizencc added 2 commits March 20, 2025 14:05

merge

82b2e37

fix tests

ba86ac0

kaizencc had a problem deploying to integ-approval March 20, 2025 20:53 — with GitHub Actions Failure

Merge branch 'main' into conroy/diff-refactor

4557318

kaizencc had a problem deploying to integ-approval March 20, 2025 20:54 — with GitHub Actions Failure

kaizencc closed this Mar 20, 2025

kaizencc reopened this Mar 20, 2025

kaizencc had a problem deploying to integ-approval March 20, 2025 20:54 — with GitHub Actions Failure

kaizencc marked this pull request as ready for review March 20, 2025 20:54

aws-cdk-automation enabled auto-merge March 20, 2025 20:54

kaizencc commented Mar 20, 2025

View reviewed changes

packages/aws-cdk/lib/commands/diff.ts Outdated Show resolved Hide resolved

kaizencc commented Mar 20, 2025

View reviewed changes

packages/aws-cdk/lib/commands/diff.ts Outdated Show resolved Hide resolved

kaizencc commented Mar 20, 2025

View reviewed changes

packages/aws-cdk/test/commands/diff.test.ts Show resolved Hide resolved

mrgrain reviewed Mar 21, 2025

View reviewed changes

mrgrain requested changes Mar 21, 2025

View reviewed changes

packages/aws-cdk/lib/commands/diff.ts Outdated Show resolved Hide resolved

packages/aws-cdk/lib/commands/diff.ts Outdated Show resolved Hide resolved

packages/aws-cdk/lib/commands/diff.ts Outdated Show resolved Hide resolved

pr feedback

7096b6a

kaizencc had a problem deploying to integ-approval March 21, 2025 18:00 — with GitHub Actions Failure

await

59fb5d2

kaizencc had a problem deploying to integ-approval March 21, 2025 18:08 — with GitHub Actions Failure

kaizencc changed the title ~~refactor(cli): diff no longer prints directly to a stream~~ refactor(cli): stack diff no longer prints directly to a stream Mar 21, 2025

jsdoc

9e891c9

kaizencc had a problem deploying to integ-approval March 21, 2025 18:11 — with GitHub Actions Failure

security diff

5f61257

kaizencc had a problem deploying to integ-approval March 21, 2025 18:48 — with GitHub Actions Failure

jsdoc

12302b8

kaizencc had a problem deploying to integ-approval March 21, 2025 18:50 — with GitHub Actions Failure

kaizencc changed the title ~~refactor(cli): stack diff no longer prints directly to a stream~~ refactor(cli): stack/security diff no longer prints directly to a stream Mar 21, 2025

more stuff

11351c6

kaizencc had a problem deploying to integ-approval March 21, 2025 18:56 — with GitHub Actions Failure

kaizencc added 2 commits March 21, 2025 14:58

merge

20b1f88

merges

949eb08

kaizencc had a problem deploying to integ-approval March 21, 2025 19:01 — with GitHub Actions Failure

kaizencc requested a review from mrgrain March 21, 2025 19:01

mrgrain requested changes Mar 21, 2025

View reviewed changes

io

c9463d0

kaizencc temporarily deployed to integ-approval March 21, 2025 19:08 — with GitHub Actions Inactive

kaizencc requested a review from mrgrain March 21, 2025 19:09

mrgrain approved these changes Mar 21, 2025

View reviewed changes

kaizencc temporarily deployed to run-tests March 21, 2025 20:58 — with GitHub Actions Inactive

aws-cdk-automation added this pull request to the merge queue Mar 21, 2025

Merged via the queue into main with commit 762e57f Mar 21, 2025
20 checks passed

aws-cdk-automation deleted the conroy/diff-refactor branch March 21, 2025 22:03

kaizencc mentioned this pull request Apr 1, 2025

fix(cli): diff always prints stack name #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(cli): stack/security diff no longer prints directly to a stream #264

refactor(cli): stack/security diff no longer prints directly to a stream #264

Uh oh!

kaizencc commented Mar 19, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain Mar 21, 2025

Uh oh!

mrgrain left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain Mar 21, 2025

Uh oh!

codecov-commenter commented Mar 21, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor(cli): stack/security diff no longer prints directly to a stream #264

refactor(cli): stack/security diff no longer prints directly to a stream #264

Uh oh!

Conversation

kaizencc commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrgrain Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaizencc commented Mar 19, 2025 •

edited

Loading

codecov-commenter commented Mar 21, 2025 •

edited

Loading