CFn v2: Implement stack deletion #12576
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
simonrw
added a commit
that referenced
this pull request
Jun 9, 2025
## 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] ```
MEPalma
approved these changes
Jun 10, 2025
| 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.
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.
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.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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
Nonein for theaftertemplate and parametersvisit_node_depends_onto not visit the depends on target twiceDELETE_IN_PROGRESSevents in the_execute_resource_actiondesired_stateand some usedprevious_statein 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.