refactor(toolkit-lib): diff action uses helper methods #301

kaizencc · 2025-03-31T20:29:25Z

refactors the diff action to use helper methods and in general cleans up the code a lot. introduces an internal concept of TemplateInfo that standardizes the to-be-diffed information, so there's not a bunch of repetitive code

existing tests continue to succeed.

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

codecov-commenter · 2025-03-31T20:36:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.92%. Comparing base (26c4b01) to head (6e0c9ef).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #301      +/-   ##
==========================================
- Coverage   86.98%   86.92%   -0.06%     
==========================================
  Files         220      220              
  Lines       35611    35634      +23     
  Branches     4351     4364      +13     
==========================================
  Hits        30975    30975              
- Misses       4540     4564      +24     
+ Partials       96       95       -1

Flag	Coverage Δ
suite.unit	`86.92% <100.00%> (-0.06%)`	⬇️

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.

packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts

mrgrain · 2025-04-01T16:43:05Z

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

      const formatter = new DiffFormatter({
        ioHelper,
-        oldTemplate: template,
-        newTemplate: stacks.firstStack,
+        oldTemplate: template.oldTemplate,
+        newTemplate: template.newTemplate,
      });


Seems okay, but could DiffFormatter not just take the instance of template here and subsequent calls to formatSecurityDiff and formatStackDiff would not need to the other options.

Or the other way around: template is passed as is into formatSecurityDiff and formatStackDiff

Just seems odd to split up the input.

this is a great idea, simplifies a whole bunch actually

packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts

mrgrain · 2025-04-01T17:20:57Z

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


-    if (diffMethod.method === 'local-file') {
-      const methodOptions = diffMethod.options as LocalFileDiffOptions;
+    const templateInfos = await makeTemplateInfos(ioHelper, stacks, deployments, await this.sdkProvider('diff'), options);


So this is preparing the diff? Current names read unclear to me. Maybe something like this is clearer:

Suggested change

const templateInfos = await makeTemplateInfos(ioHelper, stacks, deployments, await this.sdkProvider('diff'), options);

const diffInput = await prepareDiff(ioHelper, stacks, deployments, await this.sdkProvider('diff'), options);

i like the prepareDiff name but also after my latest refactor i think the type of TemplateInfo now makes more sense -- since its only getting passed into DiffFormatter. lemme know if you disagree

Signed-off-by: github-actions <[email protected]>

refactors the diff action to use helper methods and in general cleans up the code a lot. introduces an internal concept of `TemplateInfo` that standardizes the to-be-diffed information, so there's not a bunch of repetitive code existing tests continue to succeed. --- 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 <[email protected]> Co-authored-by: github-actions <[email protected]>

`Notices` will eventually serve both toolkit-lib and cli. Currently the class has a hard-coded "cli version" - but really the value is reading the version from whatever the current package is. Once we start using this file in another package, it will be wrong. For now, let's just remove the hard dependency on cli version inside the class. In future it will be extended. - Remove direct dependency on versionNumber import in Notices class - Add cliVersion as a required parameter in NoticesProps interface - Store cliVersion as a class property and use it in display method - Improve testability by removing the need to mock version functions --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

The message payload interfaces never were an api. They just ended up there by chance. Move them to their own top-level module. This has the side-effect that it will make future diffs slightly nicer. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

We are about to move this module into a different package, and the target package is gitignoring any directory called `logs`. We could just fix the gitignore, but the module name is very inacurate anyway. Therefore we are renaming it. Also moves `rwlock` from `api/utils` to `api`. This was an oversight and doesn't warrant a separate PR. Finally moves some test files into their correct corresponding directories. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Once this is merged we can increase concurrency. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Fixes #448 Prior to #301 we were using both `stackName` and `displayName` interchangably in different areas of the CLI. #301 chose to standardize to `stackName`. This PR changes that to `displayName`. --- 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 <[email protected]> Co-authored-by: Momo Kornher <[email protected]> Co-authored-by: github-actions <[email protected]>

refactor(toolkit-lib): diff action uses helper methods

4006a56

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

github-actions bot added the p2 label Mar 31, 2025

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

aws-cdk-automation requested a review from a team March 31, 2025 20:30

improts

869a3e1

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

Merge branch 'main' into conroy/refactor-diff

f1cc116

kaizencc had a problem deploying to integ-approval April 1, 2025 15:35 — with GitHub Actions Failure

mrgrain reviewed Apr 1, 2025

View reviewed changes

packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts Show resolved Hide resolved

mrgrain reviewed Apr 1, 2025

View reviewed changes

move functions around

ba025a1

kaizencc had a problem deploying to integ-approval April 1, 2025 17:13 — with GitHub Actions Failure

extraneous code

adb56c0

kaizencc had a problem deploying to integ-approval April 1, 2025 17:14 — with GitHub Actions Failure

imports

be7cf9a

kaizencc had a problem deploying to integ-approval April 1, 2025 17:16 — with GitHub Actions Failure

mrgrain reviewed Apr 1, 2025

View reviewed changes

packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts Outdated Show resolved Hide resolved

mrgrain reviewed Apr 1, 2025

View reviewed changes

chore: self mutation

e9ee393

Signed-off-by: github-actions <[email protected]>

aws-cdk-automation had a problem deploying to integ-approval April 1, 2025 17:26 — with GitHub Actions Failure

merge

6fe9b53

kaizencc had a problem deploying to integ-approval April 1, 2025 17:46 — with GitHub Actions Failure

pr feedback

1e15348

kaizencc had a problem deploying to integ-approval April 1, 2025 17:49 — with GitHub Actions Failure

fix tests

1cd6543

kaizencc had a problem deploying to integ-approval April 1, 2025 17:55 — with GitHub Actions Failure

chore: self mutation

c6bfa56

Signed-off-by: github-actions <[email protected]>

aws-cdk-automation had a problem deploying to integ-approval April 1, 2025 20:40 — with GitHub Actions Failure

kaizencc temporarily deployed to run-tests April 3, 2025 22:08 — with GitHub Actions Inactive

kaizencc added this pull request to the merge queue Apr 3, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 3, 2025

mrgrain and others added 4 commits April 4, 2025 08:59

chore: enable atmosphere (#316)

efab810

Once this is merged we can increase concurrency. --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mrgrain had a problem deploying to integ-approval April 4, 2025 08:01 — with GitHub Actions Failure

fixup

5100eb1

mrgrain had a problem deploying to integ-approval April 4, 2025 08:04 — with GitHub Actions Failure

Merge remote-tracking branch 'origin/main' into conroy/refactor-diff

6e0c9ef

mrgrain temporarily deployed to integ-approval April 4, 2025 08:05 — with GitHub Actions Inactive

mrgrain temporarily deployed to run-tests April 4, 2025 08:15 — with GitHub Actions Inactive

mrgrain added this pull request to the merge queue Apr 4, 2025

Merged via the queue into main with commit 5ed29f0 Apr 4, 2025
20 checks passed

mrgrain deleted the conroy/refactor-diff branch April 4, 2025 09:09

kaizencc self-assigned this Apr 15, 2025

kaizencc mentioned this pull request May 5, 2025

fix(toolkit): diff prints stack name not display name #454

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(toolkit-lib): diff action uses helper methods #301

refactor(toolkit-lib): diff action uses helper methods #301

Uh oh!

kaizencc commented Mar 31, 2025

Uh oh!

codecov-commenter commented Mar 31, 2025 •

edited

Loading

Uh oh!

Uh oh!

mrgrain Apr 1, 2025 •

edited

Loading

Uh oh!

kaizencc Apr 1, 2025

Uh oh!

Uh oh!

mrgrain Apr 1, 2025

Uh oh!

kaizencc Apr 1, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	const templateInfos = await makeTemplateInfos(ioHelper, stacks, deployments, await this.sdkProvider('diff'), options);
	const diffInput = await prepareDiff(ioHelper, stacks, deployments, await this.sdkProvider('diff'), options);

refactor(toolkit-lib): diff action uses helper methods #301

refactor(toolkit-lib): diff action uses helper methods #301

Uh oh!

Conversation

kaizencc commented Mar 31, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

mrgrain Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrgrain Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

kaizencc Apr 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Mar 31, 2025 •

edited

Loading

mrgrain Apr 1, 2025 •

edited

Loading