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

Use arm template parameters as sole input for live test environment variables #2247

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Use arm template parameters as sole input for live test environment variables #2247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions eng/common/TestResources/New-TestResources.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,13 @@ if (!$PSBoundParameters.ContainsKey('ErrorAction')) {
$ErrorActionPreference = 'Stop'
}

function Log($Message) {
function Log($Message)
{
Copy link
Member

@heaths heaths Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Could we leave as-is? I've long practiced JavaScript-like bracketing because for pipelines like when using ForEach-Object, putting a { on the same line means you don't need to use a continuation character. For people who don't know PowerShell as well, this can be very confusing why a script does things differently in different places. Using a consistent bracketing style eliminates that confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to buy goodwill from @weshaggard because he likes net-style function syntax and usually makes these comments on my powershell PRs 😄 I don't have a strong opinion either way so I'll let you two duke it out.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think your point about script accessibility for powershell newcomers is a good one.

Copy link
Member

Choose a reason for hiding this comment

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

Basically, to highlight the difference:

filter Foo ([scriptblock] $Script) # filter is like function but defaults to `process {}` instead of `end {}` of pipelines
{
  &$Script $_
}

1..10 | Foo {
  Write-Host "Got $_"
}

# Or

1..10 | Foo `
{
  Write-Host "Got $_"
}

Notice the backtick above. Without it, errors abound. So for the sake of consistency and readability, I've gone with JavaScript-style bracing.

Copy link
Member

Choose a reason for hiding this comment

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

Still, this isn't blocking so I'll unblock, but I think it's worth changing back, @weshaggard. You have previously mentioned about newcomers to the script and why a lot of my PowerShelly shortcuts weren't the best for approachability. I think the same is true here.

Copy link
Member

Choose a reason for hiding this comment

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

There is definitely some brace issues in PS. I still like having the braces on there own lines for larger blocks, except for when you cannot like the pipelines. I'm also pushing us to not depend on the pipeline syntax in most of our scripts as well as I think they cause more issues then they help with, they are nice at the command line but less so in script files.

As for consistency there is not a great way to be fully consistent but I do want to get some coding style configurations setup for our PS scripts at some point, I believe we have an issue on the backlog for that.

Copy link
Member

Choose a reason for hiding this comment

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

The pipeline syntax has been there since the beginning. If there are scenarios you'd like me to help with, please point them out. It's been my daily driver - and I'm live in a terminal - since pre-1.0 days, when it was just "Monad". Never had a problem if functions are written correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Pipelines main goal are for doing things on a single line from the command shell which is great, however when using them in a scripting context they can complicate things. One example is the returning of null, single or list of items when you are filtering. That causes a lot of redundant code and errors when trying to work with lists in different contexts from scripting. That is why I recommend we use the ".Where(...)" on an array over using a pipeline with Where-Object. That ensures that you are always working with a list and you don't end up switching data types in different contexts.

For what it's worth I to have been using PS since it was called monad as my daily shell. At any rate probably not worth having this debate on this particular PR.

Write-Host ('{0} - {1}' -f [DateTime]::Now.ToLongTimeString(), $Message)
}

function Retry([scriptblock] $Action, [int] $Attempts = 5) {
function Retry([scriptblock] $Action, [int] $Attempts = 5)
{
$attempt = 0
$sleep = 5

Expand All @@ -109,7 +111,20 @@ function Retry([scriptblock] $Action, [int] $Attempts = 5) {
}
}

function MergeHashes([hashtable] $source, [psvariable] $dest) {
function LoadCloudConfig([string] $env)
{
$configPath = "$PSScriptRoot/clouds/$env.json"
if (!(Test-Path $configPath)) {
Write-Warning "Could not find cloud configuration for environment '$env'"
return @{}
}

$config = Get-Content $configPath | ConvertFrom-Json -AsHashtable
return $config
}

function MergeHashes([hashtable] $source, [psvariable] $dest)
{
foreach ($key in $source.Keys) {
if ($dest.Value.ContainsKey($key) -and $dest.Value[$key] -ne $source[$key]) {
Write-Warning ("Overwriting '$($dest.Name).$($key)' with value '$($dest.Value[$key])' " +
Expand All @@ -119,7 +134,8 @@ function MergeHashes([hashtable] $source, [psvariable] $dest) {
}
}

function BuildBicepFile([System.IO.FileSystemInfo] $file) {
function BuildBicepFile([System.IO.FileSystemInfo] $file)
{
if (!(Get-Command bicep -ErrorAction Ignore)) {
Write-Error "A bicep file was found at '$($file.FullName)' but the Azure Bicep CLI is not installed. See https://aka.ms/install-bicep-pwsh"
throw
Expand Down Expand Up @@ -492,6 +508,8 @@ try {
$templateParameters.Add('testApplicationSecret', $TestApplicationSecret)
}

$defaultCloudParameters = LoadCloudConfig $Environment
MergeHashes $defaultCloudParameters $(Get-Variable templateParameters)
MergeHashes $ArmTemplateParameters $(Get-Variable templateParameters)
MergeHashes $AdditionalParameters $(Get-Variable templateParameters)

Expand Down
15 changes: 15 additions & 0 deletions eng/common/TestResources/clouds/AzureChinaCloud.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"azConfigEndpointSuffix": ".azconfig.azure.cn",
"azureAuthorityHost": "https://login.chinacloudapi.cn/",
"cognitiveServicesEndpointSuffix": ".cognitiveservices.azure.cn",
"containerRegistryEndpointSuffix": ".azurecr.cn",
"cosmosEndpointSuffix": "cosmos.azure.cn",
"enableStorageVersioning": false,
"keyVaultDomainSuffix": ".vault.azure.cn",
"keyVaultEndpointSuffix": ".vault.azure.cn",
"keyVaultSku": "standard",
"searchEndpointSuffix": "search.azure.cn",
"serviceBusEndpointSuffix": ".servicebus.chinacloudapi.cn",
"storageEndpointSuffix": "core.chinacloudapi.cn",
"textAnalyticsSku": "S"
}
8 changes: 8 additions & 0 deletions eng/common/TestResources/clouds/AzureCloud.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"azureAuthorityHost": "https://login.microsoftonline.com/",
"cognitiveServicesEndpointSuffix": ".cognitiveservices.azure.com",
"communicationServicesEndpointSuffix": ".communication.azure.com",
"keyVaultDomainSuffix": ".vault.azure.net",
"keyVaultEndpointSuffix": ".vault.azure.net",
"storageEndpointSuffix": "core.windows.net"
}
15 changes: 15 additions & 0 deletions eng/common/TestResources/clouds/AzureUSGovernment.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"azConfigEndpointSuffix": ".azconfig.azure.us",
"azureAuthorityHost": "https://login.microsoftonline.us/",
"cognitiveServicesEndpointSuffix": ".cognitiveservices.azure.us",
"containerRegistryEndpointSuffix": ".azurecr.us",
"cosmosEndpointSuffix": "cosmos.azure.us",
"enableStorageVersioning": false,
"formRecognizerLocation": "usgovvirginia",
"keyVaultDomainSuffix": ".vault.usgovcloudapi.net",
"keyVaultEndpointSuffix": ".vault.usgovcloudapi.net",
"keyVaultSku": "premium",
"searchEndpointSuffix": "search.azure.us",
"serviceBusEndpointSuffix": ".servicebus.usgovcloudapi.net",
"storageEndpointSuffix": "core.usgovcloudapi.net"
}