chore(migrate): enable import of resources on apps created from cdk migrate#28678
Conversation
2d3bee2 to
fea7a97
Compare
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
fea7a97 to
d6af519
Compare
d6af519 to
deebef5
Compare
deebef5 to
0b06515
Compare
0b06515 to
ba72d74
Compare
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
packages/aws-cdk/lib/cdk-toolkit.ts
Outdated
| } | ||
| } | ||
|
|
||
| private async tryGetResources(migrateDeployment: ResourceImporter) { |
There was a problem hiding this comment.
can we swap the order of tryGetResources and performResourceMigration? performResourceMigration is called by the public function, so it should be above tryGetResources
There was a problem hiding this comment.
Will do
| * to add back in any outputs and the CDKMetadata. | ||
| */ | ||
| private async tryMigrateResources(stacks: StackCollection, options: DeployOptions): Promise<void> { | ||
| const stack = stacks.stackArtifacts[0]; |
There was a problem hiding this comment.
what about the other stacks?
There was a problem hiding this comment.
CDK migrate only creates one stack. It is not compatible with multiple stacks at this time.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
|
||
| await this.performResourceMigration(migrateDeployment, resourcesToImport, options); | ||
|
|
||
| fs.rmSync('migrate.json'); |
There was a problem hiding this comment.
- i want to check that you mean to remove
migrate.jsonafter consuming it. - why does this function "deserve" to remove this file? it seems like an unnecessary side effect of a function named
tryMigrateResources. Is this the place to govern the lifecycle of that file?
This is a non-blocking comment. I don't have the full picture of migrate, just want to call attention to this and if there's no problem, then ignore.
There was a problem hiding this comment.
The intent here was to make deployment after using cdk migrate to generate an app zero touch, or as close to it as possible. CDK migrate generates this file that contains all the import data needed. So, the attempt at an import is only made when this file is present. Since the import only needs to happen on the first deployment, it deletes it at the end so that future deployments are not of the import type.
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
Apps generated from cdk migrate with resources that aren't already part of a stack will (soon) create a migrate.json file. This file contains the list of resources that should be imported upon creation of the new app.
If this file is present and the source is either
localfileor the ARN environment matches the deployment environment, runningcdk deploywill:Note:
localfileis a placeholder value so that we can run integration tests on this change. Once some of the other in-progress work is finished, this will be updated.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license