Simplify New-TestResources usage #1077
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,17 +11,17 @@ | |
| [CmdletBinding(DefaultParameterSetName = 'Default', SupportsShouldProcess = $true, ConfirmImpact = 'Medium')] | ||
| param ( | ||
| # Limit $BaseName to enough characters to be under limit plus prefixes, and https://docs.microsoft.com/azure/architecture/best-practices/resource-naming. | ||
| [Parameter()] | ||
| [ValidatePattern('^[-a-zA-Z0-9\.\(\)_]{0,80}(?<=[a-zA-Z0-9\(\)])$')] | ||
| [string] $BaseName, | ||
|
|
||
| [ValidatePattern('^[-\w\._\(\)]+$')] | ||
| [string] $ResourceGroupName, | ||
|
|
||
| [Parameter(Mandatory = $true, Position = 0)] | ||
| [string] $ServiceDirectory, | ||
|
|
||
| [Parameter()] | ||
| [ValidatePattern('^[0-9a-f]{8}(-[0-9a-f]{4}){3}-[0-9a-f]{12}$')] | ||
| [string] $TestApplicationId, | ||
|
|
||
|
|
@@ -119,6 +119,8 @@ $root = [System.IO.Path]::Combine($repositoryRoot, "sdk", $ServiceDirectory) | R | |
| $templateFileName = 'test-resources.json' | ||
| $templateFiles = @() | ||
| $environmentVariables = @{} | ||
| # Azure SDK Developer Playground | ||
| $defaultSubscription = "faa080af-c1d8-40ad-9cce-e1a450ca5b57" | ||
|
There was a problem hiding this comment. Will this work for partner teams? There was a problem hiding this comment. I thought we were giving them access. If not they can log in using There was a problem hiding this comment. @mitchdenny @weshaggard do you happen to know if partner teams will have access to Azure SDK Dev Playground subscription? There was a problem hiding this comment. Based on the current permissions set I believe that its just ADP. Generally folks have contributor access for our org and then a few folks have ownership. There was a problem hiding this comment. I'm fine with defaulting to this subscription but can we please test the failure case for someone that doesn't have access to this default sub (or another test sub)? I want to make sure it is pretty clear what is going wrong and not failing in some very hard to diagnose way. There was a problem hiding this comment. Yeah, it fails with pretty clear access denied message. |
||
|
|
||
| Write-Verbose "Checking for '$templateFileName' files under '$root'" | ||
| Get-ChildItem -Path $root -Filter $templateFileName -Recurse | ForEach-Object { | ||
|
|
@@ -133,6 +135,26 @@ if (!$templateFiles) { | |
| exit | ||
| } | ||
|
|
||
| $UserName = if ($env:USER) { $env:USER } else { "${env:USERNAME}" } | ||
|
|
||
| # If no base name is specified use current user name | ||
| if (!$BaseName) { | ||
| $BaseName = "$UserName$ServiceDirectory" | ||
|
|
||
| Log "BaseName was not set. Using default base name: '$BaseName'" | ||
| } | ||
|
|
||
| # Try detecting repos that support OutFile and defaulting to it | ||
| if (!$CI -and !$PSBoundParameters.ContainsKey('OutFile') -and $IsWindows) | ||
| { | ||
| # TODO: find a better way to detect the language | ||
|
There was a problem hiding this comment. @chidozieononiwu is working on some common scripts one of which would give you the language. If you import common.ps1 it will import the language-settings.ps1 which will have There was a problem hiding this comment. Do you mind if I file this as a followup? There was a problem hiding this comment. Is that really the heuristic we want anyway? Ideally, eventually every language in any platform can do this. It's just a matter of how. When we three talked about this months ago, one idea was even just to chmod the file, which is all the CLI does on linux anyway. I'd rather we base this on some marker - perhaps a file under /eng, or something more general purpose we can read and getting repo configs, like a .json file that PS can read as an object. Then we don't have to keep updating this script and going through all the process around pushing it down. |
||
| if (Test-Path "$repositoryRoot/eng/service.proj") | ||
| { | ||
| $OutFile = $true | ||
| Log "Detected .NET repository. Defaulting OutFile to true. Test environment settings would be stored into the file so you don't need to set environment variables manually." | ||
| } | ||
| } | ||
|
|
||
| # If no location is specified use safe default locations for the given | ||
| # environment. If no matching environment is found $Location remains an empty | ||
| # string. | ||
|
|
@@ -147,7 +169,7 @@ if (!$Location) { | |
| Write-Verbose "Location was not set. Using default location for environment: '$Location'" | ||
| } | ||
|
|
||
| # Log in if requested; otherwise, try to login into playground subscription. | ||
| if ($ProvisionerApplicationId) { | ||
| $null = Disable-AzContextAutosave -Scope Process | ||
|
|
||
|
|
@@ -176,6 +198,25 @@ if ($ProvisionerApplicationId) { | |
| } | ||
| } | ||
| } | ||
| elseif (!$CI) | ||
| { | ||
| # check if user is logged in and login into | ||
| $context = Get-AzContext; | ||
| if (!$context) | ||
| { | ||
| Log "You are not logged in, connecting to 'Azure SDK Developer Playground'" | ||
| Connect-AzAccount -Subscription $defaultSubscription | ||
| } | ||
|
|
||
| # If no test application id is specified create a new service principal | ||
|
|
||
| if (!$TestApplicationId) { | ||
| Log "TestApplicationId was not specified, creating a new service principal." | ||
| $servicePrincipal = New-AzADServicePrincipal -Role Owner | ||
|
There was a problem hiding this comment. Does that need to be removed at the end of the run, if it was dynamic? (or is it already and I'm just overlooking?) There was a problem hiding this comment. Yeah, in theory, you are right. But we never told anyone to remove their service principals after manual creation so I decided to postpone fixing the removing part. There was a problem hiding this comment. Is there any downside to removing it at the end of the script? There was a problem hiding this comment. Yeah, tests won't run because they won't be able to sign in :) There was a problem hiding this comment. You could only remove it in the |
||
|
|
||
| $TestApplicationId = $servicePrincipal.ApplicationId | ||
| $TestApplicationSecret = (ConvertFrom-SecureString $servicePrincipal.Secret -AsPlainText); | ||
| } | ||
| } | ||
|
|
||
| # Get test application OID from ID if not already provided. | ||
| if ($TestApplicationId -and !$TestApplicationOid) { | ||
|
|
@@ -215,7 +256,7 @@ $ResourceGroupName = if ($ResourceGroupName) { | |
|
|
||
| # Tag the resource group to be deleted after a certain number of hours if specified. | ||
| $tags = @{ | ||
| Creator = $UserName | ||
| ServiceDirectory = $ServiceDirectory | ||
| } | ||
|
|
||
|
|
@@ -403,7 +444,11 @@ foreach ($templateFile in $templateFiles) { | |
|
|
||
| $exitActions.Invoke() | ||
|
|
||
| # Suppress output locally | ||
| if ($CI) | ||
| { | ||
| return $environmentVariables | ||
| } | ||
|
|
||
| <# | ||
| .SYNOPSIS | ||
|
|
@@ -568,4 +613,4 @@ log redaction). | |
|
|
||
| .LINK | ||
| Remove-TestResources.ps1 | ||
| #> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. You moved the first unnamed parameter position. Leave them both named.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it would actually break anyone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think simplifying the script is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't say. Do you think specifying
-ServiceDirectoryis too much to type especially compared to everything they had to type before, whether using the script or az CLI directly?I suppose, at the very least, both were mandatory before so if they did expect this to be
$BaseName, they'd likely get an error that "sdk/Whatever" doesn't exist.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too much comparatively, but if we can do even better and it's pretty safe I would like to try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you know you are introducing a breaking change, make sure you go to the sync PRs that get created on changes in this directory and trigger some of the live test pipelines against them just to make sure that they continue to function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point @mitchdenny, I'll do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you defaulting the ServiceDirectory? Isn't it needed in order to know which arm template to deploy?
edit: Sorry I miss-read the comments it is still mandatory here so you can ignore my comment about it being required.
However we could consider figuring out a way of setting this based on where the script is executed, but that might be a little magical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't default the ServiceDirectory only the base name. ServiceDirectory is not the only required parameter and I made it positional as well.