refactor(cli): resource-import uses IoHost #33412

mrgrain · 2025-02-12T13:40:05Z

Issue

Relates to #32292

Description of changes

combines ResourceImporter and ResourceMigrator into a new api submodule
introduces explicit ImportDeploymentOptions options that only expose actually used deployment options, it is used for both ResourceImporter and ResourceMigrator
switch to IoHost for all logging

Describe any new or updated permissions being added

n/a

Description of how you validated changes

Existing tests and cli-integ tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

mrgrain · 2025-02-12T13:40:43Z

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

some minor refactoring to make the code more human readable

aws-cdk-automation

(This review is outdated)

codecov · 2025-02-12T13:52:30Z

Codecov Report

Attention: Patch coverage is 71.92982% with 16 lines in your changes missing coverage. Please review.

Project coverage is 80.90%. Comparing base (c3bd4d9) to head (3f8a3ed).
Report is 1 commits behind head on main.

❌ Your patch status has failed because the patch coverage (71.92%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #33412      +/-   ##
==========================================
- Coverage   80.90%   80.90%   -0.01%     
==========================================
  Files         236      238       +2     
  Lines       14261    14287      +26     
  Branches     2493     2495       +2     
==========================================
+ Hits        11538    11559      +21     
- Misses       2438     2442       +4     
- Partials      285      286       +1

Flag	Coverage Δ
suite.unit	`80.90% <71.92%> (-0.01%)`	⬇️

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

Components	Coverage Δ
packages/aws-cdk	`79.74% <71.92%> (+<0.01%)`	⬆️
packages/aws-cdk-lib/core	`82.16% <ø> (ø)`

mrgrain · 2025-02-12T13:53:20Z

packages/aws-cdk/lib/api/resource-import/importer.ts

+  action: ToolkitAction;
+}
+
+export interface ImportDeploymentOptions {


Made this an explicit copy to limit the interface to a small subset of expected options.

mrgrain · 2025-02-12T13:55:41Z

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


  constructor(private readonly props: CdkToolkitProps) {
    this.ioHost = props.ioHost ?? CliIoHost.instance();
+    this.toolkitStackName = props.toolkitStackName ?? DEFAULT_TOOLKIT_STACK_NAME;


Made this a class property to mimic behavior in new toolkit and to provide a fallback when an action option is unset. In practice, the class toolkitStackName and action option toolkitStackName will be the same in the CLI. Will clean this up in a later follow-up PR.

mrgrain · 2025-02-12T13:56:27Z

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

+          ioHost: this.ioHost,
+          action: 'diff',


I would love to use the withAction pattern here, but I don't to duplicate code. Will revisit this after the CLI split.

mrgrain · 2025-02-12T18:22:48Z

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

-   *
-   * @default CDKToolkit
-   */
-  readonly toolkitStackName?: string;


Removed in favor of the same option on the Toolkit.

kaizencc

minor comments otherwise looks good

kaizencc · 2025-02-12T18:43:54Z

packages/aws-cdk/lib/api/resource-import/importer.ts

        : ' ✅  %s';

-      success('\n' + message, this.stack.displayName);
+      await this.ioHost.notify(info(this.action, '\n' + chalk.green(format(message, this.stack.displayName))));


why did we do away with the success helper?

Getting rid of all helpers with implicit formatting.

kaizencc · 2025-02-12T18:45:37Z

packages/aws-cdk/lib/api/resource-import/index.ts

@@ -0,0 +1,2 @@
+export * from './importer';
+export * from './migrator';


sort of a meta question -- is there a way to block us from importing directly from the file itself when we have an index.ts?

i want to disawllow import * as blah from 'aws-cdk/lib/api/resource-inport/importer'; in favor of import * as blah from 'aws-cdk/lib/api/resource-import'; in this case, so that we don't have multiple ways of doing the same thing

You could write an eslint rule for it, might need a plugin. There can be good reasons to import directly though. My current concern is that anything outside of api should only import form the barrel files. Within api I don't mind so much.

kaizencc · 2025-02-12T18:49:02Z

packages/aws-cdk/lib/api/resource-import/importer.ts

+   *
+   * @default CDKToolkit
+   */
+  readonly toolkitStackName: string;


is this where you're pointing to when you say "makes toolkitStackName a mandatory option"? outside of 'mandatory option' feeling like an oxymoron, i'm curious why that decision was made and what it means to have a mandatory option with a default.

when i first read 'mandatory option' i thought you meant that you were mandating toolkitStackName as an option for cli option bags.

It's mandatory on the Toolkit.

outside of 'mandatory option' feeling like an oxymoron

I guess it's more of a forced choice. You do have options, but you are mandated to pick one. I'm open to learn about better language choices though!

packages/aws-cdk/lib/api/resource-import/importer.ts

kaizencc · 2025-02-12T18:50:43Z

packages/aws-cdk/test/api/resource-import/import.test.ts

  const importable = await importer.askForResourceIdentifiers(additions);
-  await importer.importResourcesFromMap(importable, {});
+  await importer.importResourcesFromMap(importable, {
+    toolkitStackName: 'CDKToolkit',


seems like this is required now. similar to the other comment, im iffy on why that's necessary.

Hmm. We currently have toolkitStackName passed through from the CLI Action options all the way down and at every level it's optional. A few times when the value is needed we add a default fallback. I'm annoyed that this exists so many times.

But it's even worse - it doesn't even look used.

mrgrain · 2025-02-12T22:42:02Z

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

+   *
+   * @default CDKToolkit
+   */
+  toolkitStackName?: string;


@kaizencc This is the new option on the toolkit itself. Already exists on the new toolkit.

aws-cdk-automation · 2025-02-12T22:44:36Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2025-02-13T14:51:06Z

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).

mergify · 2025-02-13T15:22:03Z

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 · 2025-02-13T15:22:05Z

AWS CodeBuild CI Report

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

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

github-actions · 2025-02-13T15:22:20Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

mrgrain requested a review from a team as a code owner February 12, 2025 13:40

aws-cdk-automation requested a review from a team February 12, 2025 13:40

github-actions bot added the p2 label Feb 12, 2025

mergify bot added the contribution/core This is a PR that came from AWS. label Feb 12, 2025

mrgrain commented Feb 12, 2025

View reviewed changes

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

Copy link

Contributor Author

mrgrain Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some minor refactoring to make the code more human readable

mrgrain had a problem deploying to test-pipeline February 12, 2025 13:41 — with GitHub Actions Failure

aws-cdk-automation previously requested changes Feb 12, 2025

View reviewed changes

mrgrain force-pushed the mrgrain/refactor/cli/resource-import branch from e3322f5 to 05df617 Compare February 12, 2025 13:52

mrgrain commented Feb 12, 2025

View reviewed changes

mrgrain had a problem deploying to test-pipeline February 12, 2025 13:53 — with GitHub Actions Failure

mrgrain commented Feb 12, 2025

View reviewed changes

mrgrain had a problem deploying to test-pipeline February 12, 2025 14:08 — with GitHub Actions Failure

mrgrain force-pushed the mrgrain/refactor/cli/resource-import branch from 321f073 to ffe21fd Compare February 12, 2025 14:41

mrgrain had a problem deploying to test-pipeline February 12, 2025 14:42 — with GitHub Actions Failure

refactor(cli): resource-import uses IoHost

4fcc061

mrgrain force-pushed the mrgrain/refactor/cli/resource-import branch from ffe21fd to 4fcc061 Compare February 12, 2025 14:44

mrgrain temporarily deployed to test-pipeline February 12, 2025 14:44 — with GitHub Actions Inactive

mrgrain added the pr-linter/exempt-codecov The PR linter will not require codecov checks to pass label Feb 12, 2025

mrgrain commented Feb 12, 2025

View reviewed changes

kaizencc reviewed Feb 12, 2025

View reviewed changes

remove unused toolkitStackName

21397e6

mrgrain temporarily deployed to test-pipeline February 12, 2025 22:38 — with GitHub Actions Inactive

mrgrain commented Feb 12, 2025

View reviewed changes

mrgrain added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Feb 13, 2025

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 13, 2025

kaizencc approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'main' into mrgrain/refactor/cli/resource-import

3f8a3ed

mergify bot had a problem deploying to test-pipeline February 13, 2025 14:52 Failure

mergify bot merged commit a850ec6 into main Feb 13, 2025
16 of 20 checks passed

mergify bot deleted the mrgrain/refactor/cli/resource-import branch February 13, 2025 15:22

github-actions bot locked as resolved and limited conversation to collaborators Feb 13, 2025

		@@ -0,0 +1,2 @@
		export * from './importer';
		export * from './migrator';

refactor(cli): resource-import uses IoHost #33412

refactor(cli): resource-import uses IoHost #33412

Uh oh!

Conversation

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

Uh oh!

Issue

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation left a comment • 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!

codecov bot commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Feb 12, 2025

Uh oh!

mergify bot commented Feb 13, 2025

Uh oh!

Uh oh!

mergify bot commented Feb 13, 2025

Uh oh!

aws-cdk-automation commented Feb 13, 2025

AWS CodeBuild CI Report

Uh oh!

github-actions bot commented Feb 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrgrain commented Feb 12, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading