Skip to content

CFNv2: minor parity assessment and triaging #12871

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

Merged
merged 30 commits into from
Jul 25, 2025
Merged

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