-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(migrate): enable import of resources on apps created from cdk migrate #28678
Conversation
2d3bee2
to
fea7a97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the other stacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- i want to check that you mean to remove
migrate.json
after 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
localfile
or the ARN environment matches the deployment environment, runningcdk deploy
will:Note:
localfile
is 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