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.

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

Add logical<->physical zone mapping functions to Bicep #11660

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jmorerice
Copy link
Contributor

@jmorerice jmorerice commented Aug 29, 2023

  • Added availability zone mapping functions
  • Do not merge until PR's for Deployments and ARM are merged
Microsoft Reviewers: Open in CodeFlow

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

Test Results (linux-x64)

       33 files  ±0         33 suites  ±0   44m 15s ⏱️ + 10m 11s
10 420 tests +2  10 420 ✔️ +38  0 💤  - 36  0 ±0 
12 637 runs  +2  12 637 ✔️ +38  0 💤  - 36  0 ±0 

Results for commit 452072d. ± Comparison against base commit 744dd34.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

Test Results (linux-musl-x64)

       33 files  ±0         33 suites  ±0   36m 16s ⏱️ -14s
10 420 tests +2  10 420 ✔️ +38  0 💤  - 36  0 ±0 
12 637 runs  +2  12 637 ✔️ +38  0 💤  - 36  0 ±0 

Results for commit 452072d. ± Comparison against base commit 744dd34.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

Test Results (win-x64)

       33 files  ±0         33 suites  ±0   33m 5s ⏱️ -24s
10 432 tests +2  10 432 ✔️ +38  0 💤  - 36  0 ±0 
12 648 runs  +2  12 648 ✔️ +38  0 💤  - 36  0 ±0 

Results for commit 452072d. ± Comparison against base commit 744dd34.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

Test Results (osx-x64)

       33 files  ±0         33 suites  ±0   1h 27m 10s ⏱️ - 11m 5s
10 424 tests +2  10 424 ✔️ +38  0 💤  - 36  0 ±0 
12 641 runs  +2  12 641 ✔️ +38  0 💤  - 36  0 ±0 

Results for commit 452072d. ± Comparison against base commit 744dd34.

♻️ This comment has been updated with latest results.

@jmorerice jmorerice marked this pull request as ready for review August 29, 2023 09:59
@jmorerice jmorerice requested a review from majastrz August 29, 2023 10:00
@@ -113,6 +113,38 @@ maxLength switch
.WithDescription("Combines multiple arrays and returns the concatenated array.")
.WithVariableParameter("arg", LanguageConstants.Array, minimumCount: 1, "The array for concatenation")
.Build();

yield return new FunctionOverloadBuilder("toLogicalZone")
.WithReturnType(LanguageConstants.String)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the result of this function be known at compile time (like toLower('FOO')), or is this something that can only be decided at deploy time (like resourceGroup())?

If this function is decidable at compile time, you can use .WithReturnResultBuilder(TryDeriveLiteralReturnType("toLogicalZone", LanguageConstants.String), LanguageConstants.String) instead of .WithReturnType(LanguageConstants.String) to have the type system try to execute this function on its arguments and derive a literal return type.

If this function is only decidable at deploy time (e.g., if the mapping is based on ARM's service configuration or varies from subscription to subscription), it should probably be defined in AzNamespaceType (like resourceGroup() is), which would make it part of the az namespace.

Copy link
Member

Choose a reason for hiding this comment

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

Good point! The fact that it contains a parameter named subscriptionId tells me it probably belongs in az regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback! It has been moved to the az namespace.

@jeskew jeskew added the do not merge Do not merge this pull request yet. label Sep 1, 2023
@jeskew
Copy link
Contributor

jeskew commented Sep 1, 2023

@jmorerice Do you know which ARM release is required for this PR to be merged?

@jmorerice
Copy link
Contributor Author

@jmorerice Do you know which ARM release is required for this PR to be merged?

The Deployments and ARM release is blocked because of logistical issues with configuring a callback so I'm not quite sure of the exact timeline when it would be ready by unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this pull request yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants