Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Disable legacy CFn model execution #10353

Merged
merged 5 commits into from Mar 5, 2024
Merged

Disable legacy CFn model execution #10353

merged 5 commits into from Mar 5, 2024

Conversation

dominikschubert
Copy link
Member

@dominikschubert dominikschubert commented Feb 29, 2024 •
edited

Motivation

First part of the cleanup, now that we have basically finished the migration of the resource providers

Changes

  • Remove adapter for legacy providers
  • Remove special cases that were only in place for legacy usage
  • Stricter handling of the returned OperationStatus from a resource provider
  • Improved exception message for TimeoutError (now includes logical resource id / type)

TODO

  • Test -ext against this to avoid any surprises

Follow-up

  • Delete old models

@dominikschubert dominikschubert self-assigned this Feb 29, 2024
dominikschubert
dominikschubert commented Feb 29, 2024
View reviewed changes
localstack/services/cloudformation/resource_provider.py
Comment on lines -764 to -766
"AWS::ECR::Repository",
"AWS::SecretsManager::SecretTargetAttachment",
"AWS::EC2::SubnetRouteTableAssociation",
Copy link
Member Author

Choose a reason for hiding this comment

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

this especially might cause some new issues with -ext since we didn't test them yet because of this exemption list!

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 29, 2024 •
edited

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 29m 17s ⏱️ + 1m 27s
2 688 tests ±0  2 432 ✅ ±0  256 💤 ±0  0 ❌ ±0 
2 690 runs  ±0  2 432 ✅ ±0  258 💤 ±0  0 ❌ ±0 

Results for commit ee83b68. ± Comparison against base commit 76292e1.

This pull request skips 1 and un-skips 1 tests. 
tests.aws.services.stepfunctions.v2.timeouts.test_timeouts.TestTimeouts ‑ test_fixed_timeout_service_lambda_with_path

tests.aws.services.events.scheduled_rules.test_events_scheduled_rules_logs ‑ test_scheduled_rule_logs

♻️ This comment has been updated with latest results.

@dominikschubert dominikschubert added the semver: patch Non-breaking changes which can be included in patch releases label Mar 5, 2024
@coveralls
Copy link

Coverage Status

coverage: 83.478% (+0.07%) from 83.409%
when pulling ee83b68 on cfn-disable-legacy-models
into 76292e1 on master.

dominikschubert
dominikschubert commented Mar 5, 2024
View reviewed changes
tests/aws/services/stepfunctions/v2/timeouts/test_timeouts.py
@@ -83,6 +83,8 @@ def test_fixed_timeout_service_lambda(
exec_input,
)

# FIXME: https://app.circleci.com/pipelines/github/localstack/localstack/22941/workflows/ed5c7ca1-f354-4e6a-b4c2-c85007d2cceb/jobs/186457?invite=true#step-104-3159
@pytest.mark.skip(reason="flaky")
Copy link
Member Author

Choose a reason for hiding this comment

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

sneaked that in 👀

@dominikschubert dominikschubert marked this pull request as ready for review March 5, 2024 16:43
simonrw
simonrw approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

My only concern is lack of test coverage and missed resource providers (now that we are strictly using the new versions). How confident are we that we have migrated them all?!

localstack/services/cloudformation/engine/template_deployer.py
Comment on lines -1416 to -1418
# FIXME: ugly
resources=self.resources,
legacy_base_models=RESOURCE_MODELS,
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

localstack/services/cloudformation/resource_provider.py
Comment on lines +468 to +470
raise TimeoutError(
f"Resource deployment for resource {raw_payload['requestData']['logicalResourceId']} (type {raw_payload['resourceType']}) timed out."
)
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 including this. This will be a nice QOL improvement

@dominikschubert
Copy link
Member Author

dominikschubert commented Mar 5, 2024 •
edited

How confident are we that we have migrated them all?!

Fairly confident, since I've checked that via script at least (comparing old/new resources). Yes, some resources might not be covered via tests but those should be fairly few now. At least when I was going through resources and doing reviews I've added a lot of missing tests as well.

Everything else can be done reactively, I'd say 👍

@dominikschubert dominikschubert merged commit 4e568f0 into master Mar 5, 2024
31 checks passed
@dominikschubert dominikschubert deleted the cfn-disable-legacy-models branch March 5, 2024 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonrw simonrw simonrw approved these changes

@MEPalma MEPalma Awaiting requested review from MEPalma MEPalma is a code owner

@pinzon pinzon Awaiting requested review from pinzon pinzon is a code owner

Assignees

@dominikschubert dominikschubert

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dominikschubert @coveralls @simonrw