Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(cli): support CloudFormation simplified resource import #29087
Changes from 23 commits
0f8965f
e42494e
ca9cd25
e803106
a2fa8b4
d66b937
7e04e54
30c1e50
86f6ebc
9229d92
3a2a739
2a6aae5
07fa306
526daed
d5f41e6
2abaeba
3cc542d
e812eaa
6e8813c
dd75942
8b5ea7e
552e0eb
8803a58
1051e8f
af43d96
c8be1f5
8eb1549
293a61f
ced4fb1
5b1db10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
andImportExistingResources
?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
andResourcesToImport
. Auto-import is a subset of resources supported byResourcesToImport
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 isThere 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:
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.
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
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.