-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(cli): support CloudFormation simplified resource import #29087
base: main
Are you sure you want to change the base?
Conversation
maybe revert later
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
@SankyRed Thanks! I guess we also need to add |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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.
we probably want this feature on diff changesets as well... @TheRealAmazonKendra what do you think?
ResourcesToImport: this.options.resourcesToImport, | ||
Description: `CDK Changeset for execution ${this.uuid}`, | ||
ClientToken: `create${this.uuid}`, | ||
ImportExistingResources: importExistingResources, |
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 happens if they specify both ResourcesToImport
and ImportExistingResources
?
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 guess it will not work, but the spec says nothing about such restriction, so I would like to keep it as-is.
Actually users cannot set both ImportExistingResources and ResourcesToImport from the CDK CLI, because the former is only used in cdk deploy, and cdk deploy does not have any command argument to directly set ResourcesToImport.
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 spec isn't really the source of truth we all wish it was so it's not uncommon we enforce logical things the spec does not. However in this specific instance I believe it is correct not to enforce these arguments be exclusive because there is a difference between the resources supported by importExistingResources
and ResourcesToImport
. Auto-import is a subset of resources supported by ResourcesToImport
so it is entirely possible to have both. It is entirely possible then someone could build a command which fails, but I believe this won't really be a common occurence and this is fine to leave as is
@comcalvi @tmokmss @TheRealAmazonKendra What exactly is this PR blocked on? It has been months since the PR was raised. If it's adding this feature in a different cdk operation (cdk diff), can that be a separate PR/issue? |
Pull request has been modified.
@comcalvi Hi, I resolved the merge conflict. Can you take a look at this once again? |
ResourcesToImport: this.options.resourcesToImport, | ||
Description: `CDK Changeset for execution ${this.uuid}`, | ||
ClientToken: `create${this.uuid}`, | ||
ImportExistingResources: importExistingResources, |
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 spec isn't really the source of truth we all wish it was so it's not uncommon we enforce logical things the spec does not. However in this specific instance I believe it is correct not to enforce these arguments be exclusive because there is a difference between the resources supported by importExistingResources
and ResourcesToImport
. Auto-import is a subset of resources supported by ResourcesToImport
so it is entirely possible to have both. It is entirely possible then someone could build a command which fails, but I believe this won't really be a common occurence and this is fine to leave as is
@@ -880,6 +880,41 @@ describe('disable rollback', () => { | |||
|
|||
}); | |||
|
|||
describe('import-existing-resources', () => { |
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 get a CLI integ test that creates a single resource and then imports it with this new flag?
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.
@comcalvi
Thanks, but I don't fully agree with implementing such test on CDK side. Because:
- Such test would more or less complicate the test code, not noly the success case, but also ensuring there is no orphaned resource when test failed halfway through. This possibly increases maintenance cost of the cli integ test code.
- It is not CDK's responsibility to ensure that a resource is successfully imported when the flag is set. It is CFn's responsibility to guarantee the functionality. CDK should just make sure the flag is set properly (as tested in the current code.)
What do you think?
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.
It is not CDK's responsibility to ensure that a resource is successfully imported when the flag is set. It is CFn's responsibility to guarantee the functionality. CDK should just make sure the flag is set properly (as tested in the current code.)
This is true, however ultimately any test involving any cloudformation api usage (such as most CDK deploy tests) also falls under that same umbrella. Even though we don't own cloudformation nor are we responsible to fix any issue blocking our service in cloudformation, it does allow us to report the impact our customer's impact to appropriate CFN teams. Regardless of all that though it's already a just line in the sand we've crossed ages ago, so I believe it's appropriate to add that test
Such test would more or less complicate the test code, not noly the success case, but also ensuring there is no orphaned resource when test failed halfway through. This possibly increases maintenance cost of the cli integ test code.
This is true, it would be far from the most complex integ test we've written but any integ tests which are verifying deployments tend to require some setup/cleanup so it does get a bit complex. Orphaning is technically possible but as long as you set up a finally
to manually cleanup you manually created, and the stack rolls back, it should be g2g. You can look at other tests in CDK Deploy or perhaps CDK Migrate for some ideas of how we've done this in the past.
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.
This is ultimately just a very long winded way of me saying I believe the test is appropriate 😅.
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.
@HBobertz Thanks, hmmm but what is the actual value to adding the test? As long as we set the importExistingResources flag, it should result in a successfull deployment, unless CFn is not working properly. If there happens to be the case when CDK fails although CFn is working as expected, that is when we should add a new test for the very case. At this moment, however, I think such test does not add any reliability to the behavior of CDK; it just adds unneessary complexity to the cli integ test code base.
Existing Resource Import MethodsCDK has 3 ways to import stuff into a CDK stack:
The New Resource Import MethodCFN launched a feature that allows a changeset to absorb new resources into the stack. This is specified as a flag on the changeset and it effectively turns any "Resource already exists" errors into resource import operations. This means that if your stack attempts to create a resource with the same physical ID as another resource in the same environment, it will import it instead. It's unclear if this flag works with That doesn't affect this PR though, because this, conceptually, is a standard Bottom LineWe should add this as a flag to |
Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>
Pull request has been modified.
Co-authored-by: Hogan Bobertz <Bobertzx@gmail.com> Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>
Thanks! I addressed all the comments from you :)
As far as I tested, CFn decides whether a resource is created or imported when we create a changeset. If a resource will be imported, the changeset type for the resource will become |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #28060.
Reason for this change
This feature allows to automatically import exsting resources with the same physical name, such as S3 bucket, DDB table, etc, during a CFn deployment.
Because resource import is a vital feature for CDK users e.g. to refactor a construct tree, cdk migrate, etc, it would benefit many potential users if cdk natively support it.
Description of changes
This PR adds a CLI option --import-exsting-resources: boolean to cdk deploy command and pass it to createChangeSet API call.
Description of how you validated changes
Added a cli integ test.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license