-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
CFn v2: Implement stack deletion #12576
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
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 3m 5s ⏱️ - 39m 11s Results for commit 3790756. ± Comparison against base commit 5b623bf. This pull request removes 2752 tests.
♻️ This comment has been updated with latest results. |
a459112
to
36039d4
Compare
60db6f4
to
ac56fa8
Compare
dc764a7
to
a3bcf96
Compare
a3bcf96
to
d7e8b94
Compare
a46547d
to
4ad07df
Compare
006a1e2
to
d71e8ab
Compare
577117b
to
0e6c490
Compare
0e6c490
to
72c5fd5
Compare
72c5fd5
to
96722da
Compare
Test Results - Alternative Providers987 tests 585 ✅ 29m 36s ⏱️ Results for commit 3790756. ♻️ This comment has been updated with latest results. |
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 1h 21m 5s ⏱️ Results for commit 3790756. ♻️ This comment has been updated with latest results. |
96722da
to
820bb88
Compare
a99cf4d
to
7572bc8
Compare
## Motivation Currently CDK bootstrap fails for a variety of reasons. Without this, users who use CDK cannot test their applications with our new engine. ## Changes * Handle dynamic parameter lookup of SSM parameters * Set resource status on success/failure of a deployment * Handle the case where an Fn::Join is called without a list as a second argument, in the special case of an empty string, e.g. `Fn::Join: ["", ""] as the CDK does this * Add helper `find_stack_v2` function for finding v2 stacks * Implement `describe_stack_resources` to support our CDK bootstrap tests * Update the cdk bootstrap test skip reason as our lack of deletion causes test failures ## Testing I have not added any integration tests for this since its functionality is covered by our existing `TestCdkInit.test_cdk_bootstrap` (parametrized) tests. Unfortunately since we don't support deletions yet (see #12576), the tests fail when run together. Each test individually passes, so I've updated the skip reason to reflect the updated nature of the failure. To test manually, unskip the tests, and run ``` pytest tests/aws/services/cloudformation/v2/ported_from_v1/resources/test_cdk.py::TestCdkInit::test_cdk_bootstrap[10] ```
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.
Thanks! This is definitely one of the key pieces for the v2 engine; great to see so many test cases unblocked now.
resource_name=depends_on_resource_logical_id, node_template=self._node_template | ||
) | ||
self.visit_node_resource(node_resource) | ||
for node_resource in self._node_template.resources.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.
Why are we avoiding _get_node_resource_for
? It looks like the user can define depends_on
values that don't match a resource in the template
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 removed this function because it visits the resource, and was triggering a double deployment. Perhaps this is more a failure of the node caching aspect? Visiting a node that has already been visited should be a no-op right?
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.
After discussion with @MEPalma I replaced this removal of _get_node_resource_for
with the original function, but using self.visit
instead of self.visit_node_resource
as self.visit
implements the expected caching behaviour to prevent double-deployment of resources.
00c0cb1
to
1ab337f
Compare
We create a dummy change set which contains no resources. This is then executed, therefore deleting all resources. This commit also unskips (and re-validates) an ec2 test which exercises this behaviour.
We should return the resource state if the resource is still being deleted, because the next time through the loop an item is fetched from the model.
Because we were not updating the stack template, when we tried to destroy the stack we would try to delete resources that no longer existed. Now we don't.
1ab337f
to
3790756
Compare
Motivation
Quite a few resource tests fail because the deletion of a stack is not correctly implemented. The tests assert that resources are deleted, but they aren't.
Changes
None
in for theafter
template and parametersvisit_node_depends_on
to not visit the depends on target twiceDELETE_IN_PROGRESS
events in the_execute_resource_action
desired_state
and some usedprevious_state
in the delete methodLimitations
While developing this change I noticed that we do not delete resources in the correct order. Namely:
The resources are correctly deployed in the right order (B then A) but they are also deleted in that order (B then A) rather than the order in CloudFormation: A then B. I will tackle this in a follow up PR.