Skip to content

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Jul 17, 2025

Motivation

While triaging the outstanding work for the CFn v2 provider project, I have made some minor changes (typically minor fixes) to gain us more parity with the v1 provider.

Changes

  • Recursively fetch nested property values (mostly for nested stacks)
  • Validate the stack name is provided in describe_stack_events
  • Don't assume a previous update model is present, for the case of calling update_stack after create_stack (or another update_stack)
  • Handle stacks with no changes
  • Correctly model stack change based on additional template properties
  • Update tests that have been updated in the v1 directory
  • Unskip 22 tests and rescope 13 tests
  • Explicitly skip the v2 directory in the main integration test step
    • the markers in the individual files are clearly not enough to correctly skip the tests, and running these tests just slows down the main pipeline run

@simonrw simonrw added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jul 17, 2025
@localstack-bot
Copy link
Contributor

Currently, only patch changes are allowed on main. Your PR labels (semver: minor) indicate that it cannot be merged into the main at this time.

Copy link

github-actions bot commented Jul 17, 2025

Test Results - Preflight, Unit

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

Results for commit 2c6ffd1. ± Comparison against base commit 1775743.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 17, 2025

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 45m 21s ⏱️ +14s
4 938 tests  - 1  4 167 ✅ +5  771 💤  - 6  0 ❌ ±0 
4 940 runs   - 1  4 167 ✅ +5  773 💤  - 6  0 ❌ ±0 

Results for commit 2c6ffd1. ± Comparison against base commit 1775743.

This pull request removes 1 test.
tests.aws.services.cloudformation.v2.ported_from_v1.api.test_stacks.TestStacksApi ‑ test_list_stack_resources_for_removed_resource

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 17, 2025

Test Results (amd64) - Acceptance

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

Results for commit 2c6ffd1. ± Comparison against base commit 1775743.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 17, 2025

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 19m 45s ⏱️
4 839 tests 4 359 ✅ 480 💤 0 ❌
4 845 runs  4 359 ✅ 486 💤 0 ❌

Results for commit 2c6ffd1.

♻️ This comment has been updated with latest results.

@simonrw simonrw force-pushed the cfn/v2/parity-assessment branch from 99634d3 to 4959841 Compare July 23, 2025 22:20
@simonrw simonrw marked this pull request as ready for review July 23, 2025 23:16
@simonrw simonrw modified the milestones: Playground, 4.8 Jul 24, 2025
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.

Great changes! 🎉

Comment on lines +88 to +93
class StackNotFoundError(ValidationError):
def __init__(self, stack_name_or_id: str):
if is_stack_arn(stack_name_or_id):
super().__init__(f"Stack with id {stack_name_or_id} does not exist")
else:
super().__init__(f"Stack [{stack_name_or_id}] does not exist")
Copy link
Member

Choose a reason for hiding this comment

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

comment: Thanks for the refactoring

@@ -242,18 +242,14 @@ def create_change_set(
request_payload=request,
template=structured_template,
template_body=template_body,
initial_status=StackStatus.REVIEW_IN_PROGRESS,
Copy link
Member

Choose a reason for hiding this comment

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

comment: Glad that we now how the correct initial status

Copy link
Member

Choose a reason for hiding this comment

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

question: is this file intentional? I didn't see any unskipped test that uses CDK

@@ -66,7 +66,7 @@ def create(
response = kinesis.register_stream_consumer(
StreamARN=model["StreamARN"], ConsumerName=model["ConsumerName"]
)
model["ConsumerARN"] = response["Consumer"]["ConsumerARN"]
model["Id"] = model["ConsumerARN"] = response["Consumer"]["ConsumerARN"]
Copy link
Member

Choose a reason for hiding this comment

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

Comment: nice small fix 👍

@simonrw simonrw force-pushed the cfn/v2/parity-assessment branch from 09ae1f3 to 4cdd95d Compare July 25, 2025 10:22
Copy link

Test Results (MA/MR) - Preflight, Unit

21 983 tests   20 249 ✅  6m 25s ⏱️
     1 suites   1 734 💤
     1 files         0 ❌

Results for commit 2c6ffd1.

Copy link

Test Results (amd64, MA/MR) - Acceptance

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

Results for commit 2c6ffd1.

@@ -394,7 +394,7 @@ jobs:
env:
# add the GitHub API token to avoid rate limit issues
GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
PYTEST_ARGS: "${{ env.TINYBIRD_PYTEST_ARGS }}${{ env.TESTSELECTION_PYTEST_ARGS }} --splits 4 --group ${{ matrix.group }} --store-durations --clean-durations --ignore=tests/unit/ --ignore=tests/bootstrap"
PYTEST_ARGS: "${{ env.TINYBIRD_PYTEST_ARGS }}${{ env.TESTSELECTION_PYTEST_ARGS }} --splits 4 --group ${{ matrix.group }} --store-durations --clean-durations --ignore=tests/unit/ --ignore=tests/bootstrap --ignore=tests/aws/services/cloudformation/v2"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding the V2 provider-specific tests to the ignored list 👍

Copy link

Test Results (amd64, MA/MR) - Integration, Bootstrap

    5 files      5 suites   2h 20m 5s ⏱️
4 839 tests 4 359 ✅ 480 💤 0 ❌
4 845 runs  4 359 ✅ 486 💤 0 ❌

Results for commit 2c6ffd1.

@simonrw simonrw merged commit 34049aa into main Jul 25, 2025
61 checks passed
@simonrw simonrw deleted the cfn/v2/parity-assessment branch July 25, 2025 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants