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

CFn: Parametrize Fn::GetAZs test across all regions #10681

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

CFn: Parametrize Fn::GetAZs test across all regions #10681

wants to merge 6 commits into from

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Apr 17, 2024 •
edited

Motivation

We cannot currently run the test_get_azs_function test against any other region but us-east-1 due to snapshot capture, and the deploy_cfn_template fixture being locked to the current profile region. We do not get coverage of other regions with this test.

Note: the Fn::GetAZs intrinsic function must be called with the stack region as its argument, otherwise it returns nothing.

Changes

  • Test cleanup
  • Support custom AWS client for deploy_cfn_template including standardising the stack deletion process between the automatic fixture cleanup, and the DeployResult.destroy method, ensuring the correct region is used
  • Parametrize test_get_azs_function to evaluate all regions. This basically ignores the chosen region for the cross-region tests, but in a sense we gain more coverage as we are testing all regions each time.
  • As a side-effect of the removal of a 2 second sleep after manual destroy in a kinesis test, the kinesis stream resource provider has been updated to asynchronously delete the resource

This PR will fail until the list of AZs per region as been corrected in moto.

/cc @sannya-singal

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Apr 17, 2024
@simonrw simonrw self-assigned this Apr 17, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 17, 2024 •
edited

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 35m 59s ⏱️ -47s
2 939 tests +7  2 645 ✅ +7  294 💤 ±0  0 ❌ ±0 
2 941 runs  +7  2 645 ✅ +7  296 💤 ±0  0 ❌ ±0 

Results for commit c2651db. ± Comparison against base commit 7917b4a.

This pull request removes 1 and adds 8 tests. Note that renamed tests count towards both. 
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function

tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[ap-northeast-1]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[ap-southeast-2]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[eu-central-1]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[eu-west-1]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[us-east-1]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[us-east-2]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[us-west-1]
tests.aws.services.cloudformation.test_template_engine.TestIntrinsicFunctions ‑ test_get_azs_function[us-west-2]

♻️ This comment has been updated with latest results.

sannya-singal
sannya-singal approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@sannya-singal sannya-singal left a comment

Choose a reason for hiding this comment

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

Thanks @simonrw for creating this PR and having a fun exploration debugging session 🙌 🚀 The fixes LGTM!

localstack/testing/pytest/fixtures.py
@@ -1005,17 +1003,18 @@ def _deploy(
if role_arn is not None:
kwargs["RoleARN"] = role_arn

response = aws_client.cloudformation.create_change_set(**kwargs)
cfn_aws_client = custom_aws_client if custom_aws_client is not None else aws_client
Copy link
Contributor

Choose a reason for hiding this comment

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

we can add a comment here stating why was this custom aws client required

@simonrw simonrw force-pushed the fix/parametrize-get-azs-function branch from cd54f9b to f2e4988 Compare April 17, 2024 14:23
@simonrw
Copy link
Contributor Author

simonrw commented Apr 18, 2024

As found by @sannya-singal, there was an attempt to update moto a little while ago which was reverted.

Morijarti
Morijarti approved these changes Apr 18, 2024
View reviewed changes
localstack/services/kinesis/resource_providers/aws_kinesis_stream.py

try:
client.describe_stream(StreamARN=model["Arn"])
return ProgressEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

All good with this changes for Kinesis.
We should probably check in future, if we are missing clean up for any other service

localstack/testing/pytest/fixtures.py
):
state = []
state: list[tuple[str, Callable]] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Always nice to see more typehints added to project :)

tests/aws/services/cloudformation/test_template_engine.py

template_path = os.path.join(
os.path.dirname(__file__), "../../templates/functions_get_azs.yml"
)

aws_client = aws_client_factory(region_name=region)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic looks good to me.
We are allowing CFn to use custom client for deployment.

@simonrw simonrw added this to the Playground milestone Apr 23, 2024
simonrw and others added 5 commits May 9, 2024 13:53
@sannya-singal sannya-singal force-pushed the fix/parametrize-get-azs-function branch from f2e4988 to 49aad81 Compare May 9, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Morijarti Morijarti Morijarti approved these changes

@sannya-singal sannya-singal sannya-singal approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert will be requested when the pull request is marked ready for review dominikschubert is a code owner

@pinzon pinzon Awaiting requested review from pinzon pinzon will be requested when the pull request is marked ready for review pinzon is a code owner

Assignees

@simonrw simonrw

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@simonrw @Morijarti @sannya-singal