Skip to content

fix function get azs cloudformation template #10595

Merged
sannya-singal merged 2 commits intomasterfrom
cfn_azs
Apr 3, 2024
Merged

fix function get azs cloudformation template #10595
sannya-singal merged 2 commits intomasterfrom
cfn_azs

Conversation

@sannya-singal
Copy link
Contributor

@sannya-singal sannya-singal commented Apr 3, 2024 •
edited
Loading

Motivation

The cfn test to check the template engine test_get_azs_function generates a snapshot against us-east-1 AWS region. But when it is run against localstack for different AZs of non-default region values it results in error after the merge of #10586 which instead of just expanding values from a to f, now uses connect_to().ec2.describe_availability_zones(...) to get AZs of a specified region.

According to previous commits in this file, we can see that this is a known gap and there is already a TODO added:

TODO parametrize this test.
For that we need to be able to parametrize the client region. The docs show the we should be
able to put any region in the parameters but it doesn't work. It only accepts the same region from the client config
if you put anything else it just returns an empty list.

When run against different non default regions in LS, the workflow throws an error here:

>> match key: azs
	#x1B[31m(-)#x1B[0m /[3] ( '<region>d' )
	#x1B[32m(+)#x1B[0m /[3] ( not present )
	#x1B[31m(-)#x1B[0m /[4] ( '<region>e' )
	#x1B[32m(+)#x1B[0m /[4] ( not present )
	#x1B[31m(-)#x1B[0m /[5] ( '<region>f' )
	#x1B[32m(+)#x1B[0m /[5] ( not present )

	Ignore list (please keep in mind list indices might not work and should be replaced):
	["$"]

Changes

This PR deploys functions_get_azs.yml specifically in us-east-1 and regenerates the snapshots.

@sannya-singal sannya-singal added the semver: patch Non-breaking changes which can be included in patch releases label Apr 3, 2024
@sannya-singal sannya-singal self-assigned this Apr 3, 2024
@github-actions
Copy link

github-actions bot commented Apr 3, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 30m 45s ⏱️ + 1m 33s
2 751 tests ±0  2 495 ✅ ±0  256 💤 ±0  0 ❌ ±0 
2 753 runs  ±0  2 495 ✅ ±0  258 💤 ±0  0 ❌ ±0 

Results for commit 894dc25. ± Comparison against base commit 9d5104a.

♻️ This comment has been updated with latest results.

@sannya-singal sannya-singal marked this pull request as ready for review April 3, 2024 09:30
Morijarti
Morijarti approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@Morijarti Morijarti left a comment

Choose a reason for hiding this comment

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

I'm ok with hard-coding us-east-1 if it's crashing pipeline, but we should see about resolving issue in how we are fetching azs in non default regions.

I guess that would be change in ec2 provider.

@sannya-singal
Copy link
Contributor Author

sannya-singal commented Apr 3, 2024

I'm ok with hard-coding us-east-1 if it's crashing pipeline, but we should see about resolving issue in how we are fetching azs in non default regions.

I guess that would be change in ec2 provider.

Thanks for the review @Morijarti! So we are getting the correct azs for non-default regions but the issue is with snapshot being generated for us-east-1 always against AWS.

@Morijarti
Copy link
Contributor

Morijarti commented Apr 3, 2024

I'm ok with hard-coding us-east-1 if it's crashing pipeline, but we should see about resolving issue in how we are fetching azs in non default regions.
I guess that would be change in ec2 provider.

Thanks for the review @Morijarti! So we are getting the correct azs for non-default regions but the issue is with snapshot being generated for us-east-1 always against AWS.

@sannya-singal should we than perhaps make test parametrizable so we are doing snapshot testing in multi-region or maybe improve transformers so it changes all mentions of region to region:1

I'm more for updating snapshots, rather than limiting tests deployments to certain us-east-1

@sannya-singal
Copy link
Contributor Author

sannya-singal commented Apr 3, 2024

I'm ok with hard-coding us-east-1 if it's crashing pipeline, but we should see about resolving issue in how we are fetching azs in non default regions.
I guess that would be change in ec2 provider.

Thanks for the review @Morijarti! So we are getting the correct azs for non-default regions but the issue is with snapshot being generated for us-east-1 always against AWS.

@sannya-singal should we than perhaps make test parametrizable so we are doing snapshot testing in multi-region or maybe improve transformers so it changes all mentions of region to region:1

I'm more for updating snapshots, rather than limiting tests deployments to certain us-east-1

Yes so we already have a TODO for that as mentioned in the PR description but currently we get an empty list.

@sannya-singal sannya-singal merged commit 84ce11b into master Apr 3, 2024
@sannya-singal sannya-singal deleted the cfn_azs branch April 3, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dominikschubert dominikschubert Awaiting requested review from dominikschubert

@pinzon pinzon Awaiting requested review from pinzon

@simonrw simonrw Awaiting requested review from simonrw

1 more reviewer

@Morijarti Morijarti Morijarti approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sannya-singal sannya-singal

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.

2 participants

@sannya-singal @Morijarti