-
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?
Changes from 29 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -880,6 +880,41 @@ describe('disable rollback', () => { | |
|
||
}); | ||
|
||
describe('import-existing-resources', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. @comcalvi
What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
test('by default, import-existing-resources is disabled', async () => { | ||
// WHEN | ||
await deployStack({ | ||
...standardDeployStackArguments(), | ||
deploymentMethod: { | ||
method: 'change-set', | ||
}, | ||
}); | ||
|
||
// THEN | ||
expect(cfnMocks.createChangeSet).toHaveBeenCalledTimes(1); | ||
expect(cfnMocks.createChangeSet).toHaveBeenCalledWith(expect.objectContaining({ | ||
ImportExistingResources: false, | ||
})); | ||
}); | ||
|
||
test('import-existing-resources is enabled', async () => { | ||
// WHEN | ||
await deployStack({ | ||
...standardDeployStackArguments(), | ||
deploymentMethod: { | ||
method: 'change-set', | ||
importExistingResources: true, | ||
}, | ||
}); | ||
|
||
// THEN | ||
expect(cfnMocks.createChangeSet).toHaveBeenCalledTimes(1); | ||
expect(cfnMocks.createChangeSet).toHaveBeenCalledWith(expect.objectContaining({ | ||
ImportExistingResources: true, | ||
})); | ||
}); | ||
}); | ||
|
||
/** | ||
* Set up the mocks so that it looks like the stack exists to start with | ||
* | ||
|
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 is