Skip to content

Conversation

@simonrw
Copy link
Contributor

@simonrw simonrw commented Jul 25, 2025

Motivation

The CloudFormation V1 provider had basic support for stack sets, with a single test covering the functionality. This functionality should be ported over to the new provider.

Changes

  • Implement stack sets in parity with the old provider (i.e. unskip the test)
  • Implement instance and stack deletion when deleting a stack set (beyond parity)
  • Add additional tests for edge cases of creating stack instances for non-existent stacks
  • Add test covering deleting a non-existent stack set

Testing

Unfortunately there is manual set up required for testing stack sets. See the docs here: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/stacksets-prereqs-self-managed.html so this change does not test/assert the behaviour of deployed resources though they were manually verified.

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Jul 25, 2025
@simonrw simonrw force-pushed the cfn/v2/stack-sets branch from ec4b9b9 to 2f3aa84 Compare July 25, 2025 15:14
@github-actions
Copy link

Test Results - Preflight, Unit

21 983 tests  ±0   20 249 ✅ ±0   6m 33s ⏱️ -11s
     1 suites ±0    1 734 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 2f3aa84. ± Comparison against base commit 34049aa.

@github-actions
Copy link

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   23m 23s ⏱️ - 1h 21m 38s
900 tests  - 4 038  340 ✅  - 3 827  560 💤  - 211  0 ❌ ±0 
902 runs   - 4 038  340 ✅  - 3 827  562 💤  - 211  0 ❌ ±0 

Results for commit 2f3aa84. ± Comparison against base commit 34049aa.

This pull request removes 4040 and adds 2 tests. Note that renamed tests count towards both.
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…
tests.aws.services.cloudformation.v2.ported_from_v1.resources.test_stack_sets ‑ test_delete_nonexistent_stack_set
tests.aws.services.cloudformation.v2.ported_from_v1.resources.test_stack_sets ‑ test_fetch_non_existent_stack_set_instances
This pull request removes 213 skipped tests and adds 2 skipped tests. Note that renamed tests count towards both.
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.apigateway.test_apigateway_api.TestApiGatewayApiRestApi ‑ test_get_api_case_insensitive
tests.aws.services.apigateway.test_apigateway_api.TestApiGatewayApiRestApi ‑ test_update_rest_api_concatenation_of_errors
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_integration_response_invalid_responsetemplates
tests.aws.services.apigateway.test_apigateway_api.TestApigatewayIntegration ‑ test_put_integration_request_parameter_bool_type
tests.aws.services.apigateway.test_apigateway_basic.TestAPIGateway ‑ test_api_gateway_authorizer_crud
…
tests.aws.services.cloudformation.v2.ported_from_v1.resources.test_stack_sets ‑ test_delete_nonexistent_stack_set
tests.aws.services.cloudformation.v2.ported_from_v1.resources.test_stack_sets ‑ test_fetch_non_existent_stack_set_instances

@github-actions
Copy link

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   3m 7s ⏱️ -2s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 2f3aa84. ± Comparison against base commit 34049aa.

@github-actions
Copy link

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   32m 47s ⏱️
464 tests 351 ✅ 113 💤 0 ❌
470 runs  351 ✅ 119 💤 0 ❌

Results for commit 2f3aa84.

@simonrw simonrw marked this pull request as ready for review July 25, 2025 17:26
Copy link
Member

@pinzon pinzon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a minor comment.

Comment on lines +782 to +787
# wait for completion of stack
for stack_name, account_id, region_name in stacks_to_await:
client = connect_to(
aws_access_key_id=account_id, region_name=region_name
).cloudformation
client.get_waiter("stack_create_complete").wait(StackName=stack_name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: is this correct? Maybe a better approach would be to put this code in a separate thread and update the stack set operation status depending on the result of the waits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right but I think you have two suggestions here:

  1. run each stack instance concurrently in a background thread
  2. don't assume success and handle the error
    I don't think point 1. is worth tackling at this stage, however point 2 is very relevant. I would like to tackle this in a follow up since this PR does not support successfully deploying stack instances. See the note here.

I have started a PR to create a validated stack sets test. I think the step after that is to handle error cases. Thanks!

@alexrashed alexrashed added this to the 4.8 milestone Jul 29, 2025
@simonrw simonrw merged commit fb42a08 into main Jul 29, 2025
39 checks passed
@simonrw simonrw deleted the cfn/v2/stack-sets branch July 29, 2025 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants