Simplify New-TestResources usage #1077

pakrym · 2020-10-06T19:08:24Z

Make the script easy to use locally:

Default to Out-File in .NET repo.
Default to username + servicedirectory as a basepath
Automatically login into Azure SDK Developer Playground if not logged in
Suppress extra output caused by returning variable locally
Auto-create service principal

Typical usage becomes eng\common\TestResources\New-TestResources.ps1 search

Fixes #541
Fixes #485

pakrym · 2020-10-06T19:09:24Z

cc @jsquire, you might like it

heaths

You need to regenerate the .md file as well for the cmdlet.

eng/common/TestResources/New-TestResources.ps1

jsquire · 2020-10-06T19:51:08Z

eng/common/TestResources/New-TestResources.ps1

+        Connect-AzAccount -Subscription $defaultSubscription
+    }
+
+    # If no test application id is specified create a new service principal


oooohhhh.... nice!

jsquire · 2020-10-06T19:51:59Z

eng/common/TestResources/New-TestResources.ps1

+    # 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


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?)

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.

Is there any downside to removing it at the end of the script?

Yeah, tests won't run because they won't be able to sign in :)

You could only remove it in the Remove-TestResources script, assuming the same principal is used to run the tests (by default they are). You could save that to an environment variable or some other means to know which to remove.

Filed #1078

Co-authored-by: Heath Stewart <heaths@outlook.com>

JoshLove-msft · 2020-10-06T19:59:45Z

eng/common/TestResources/New-TestResources.ps1

@@ -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"


Will this work for partner teams?

I thought we were giving them access. If not they can log in using Connect-AzAccount on their own.

@mitchdenny @weshaggard do you happen to know if partner teams will have access to Azure SDK Dev Playground subscription?

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.

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.

Yeah, it fails with pretty clear access denied message.

pakrym · 2020-10-06T20:05:13Z

cc @drwill-ms, did you have any other pain points with the script?

heaths · 2020-10-06T20:07:41Z

eng/common/TestResources/New-TestResources.ps1

    [ValidatePattern('^[-a-zA-Z0-9\.\(\)_]{0,80}(?<=[a-zA-Z0-9\(\)])$')]
    [string] $BaseName,

    [ValidatePattern('^[-\w\._\(\)]+$')]
    [string] $ResourceGroupName,

-    [Parameter(Mandatory = $true)]
+    [Parameter(Mandatory = $true, Position = 0)]


This is a breaking change. You moved the first unnamed parameter position. Leave them both named.

Do you think it would actually break anyone?

I think simplifying the script is worth it.

Can't say. Do you think specifying -ServiceDirectory is 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.

Can't say. Do you think specifying -ServiceDirectory is too much to type especially compared to everything they had to type before, whether using the script or az CLI directly?

Not too much comparatively, but if we can do even better and it's pretty safe I would like to try.

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.

Great point @mitchdenny, I'll do that.

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.

I don't default the ServiceDirectory only the base name. ServiceDirectory is not the only required parameter and I made it positional as well.

drwill-ms · 2020-10-06T22:15:32Z

cc @drwill-ms, did you have any other pain points with the script?

Thanks for the call out, Pavel. It has been a while since I used it, and can't remember at the moment. It works pretty well. I think the main thing was the confusion I had with two parameter sets and figuring out which params applied to which set, and wanting to leave some out for the set I used.

azure-sdk · 2020-10-07T15:37:35Z

The following pipelines have been queued for testing:
java - template
js - template
net - template
python - template
Sign off on the approval gate to test the release stage of each pipeline.

JoshLove-msft · 2020-10-07T17:47:33Z

eng/common/TestResources/README.md

-  -ServiceDirectory 'search' `
-  -TestApplicationId $sp.ApplicationId `
-  -TestApplicationSecret (ConvertFrom-SecureString $sp.Secret -AsPlainText)
+eng\common\TestResources\New-TestResources.ps1 -ServiceDirectory 'search'


Since partner teams will not have access to Playground subscription, should we leave the first line with Connect-AzAccount?

Or should we prompt them to enter a subscription after the fact?

What do you think is more user friendly?

I think I'll add the first line back

If there was a way that we can try to login and only prompt if it failed, I would vote for that.

azure-sdk · 2020-10-07T19:13:43Z

The following pipelines have been queued for testing:
java - template
js - template
net - template
python - template
Sign off on the approval gate to test the release stage of each pipeline.

weshaggard · 2020-10-08T17:50:09Z

eng/common/TestResources/New-TestResources.ps1

+# 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


@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 $language defined.

Do you mind if I file this as a followup?

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.

chidozieononiwu · 2020-10-08T17:56:41Z

/check-enforcer override

* Simplify Net-TestResources usage * docs and windows check * Update eng/common/TestResources/New-TestResources.ps1 Co-authored-by: Heath Stewart <heaths@outlook.com> * update markdown * make service directory the default parameter * Fix links * Doc change Co-authored-by: Heath Stewart <heaths@outlook.com>

…Azure#850 (Azure#1077)

Simplify Net-TestResources usage

8a84177

pakrym requested review from heaths and danieljurek October 6, 2020 19:08

pakrym requested a review from weshaggard as a code owner October 6, 2020 19:08

pakrym requested a review from JoshLove-msft October 6, 2020 19:08

docs and windows check

c4279c8

JoshLove-msft approved these changes Oct 6, 2020

View reviewed changes

heaths changed the title ~~Simplify Net-TestResources usage~~ Simplify New-TestResources usage Oct 6, 2020

heaths approved these changes Oct 6, 2020

View reviewed changes

eng/common/TestResources/New-TestResources.ps1 Outdated Show resolved Hide resolved

jsquire approved these changes Oct 6, 2020

View reviewed changes

pakrym and others added 2 commits October 6, 2020 12:54

Update eng/common/TestResources/New-TestResources.ps1

aad2aca

Co-authored-by: Heath Stewart <heaths@outlook.com>

update markdown

ef1a10a

JoshLove-msft reviewed Oct 6, 2020

View reviewed changes

pakrym mentioned this pull request Oct 6, 2020

Remove the automatically created service principal #1078

Open

make service directory the default parameter

7e4a377

heaths requested changes Oct 6, 2020

View reviewed changes

heaths approved these changes Oct 6, 2020

View reviewed changes

Fix links

2bb3633

JoshLove-msft reviewed Oct 7, 2020

View reviewed changes

Doc change

0fe749f

weshaggard reviewed Oct 8, 2020

View reviewed changes

chidozieononiwu merged commit dada9b0 into Azure:master Oct 8, 2020

This was referenced Oct 8, 2020

Use common.ps1 in New-TestResources to detect the language. #1082

Open

Invoke template live test pipeline as part of common tool sync process #1080

Closed

sima-zhu pushed a commit to sima-zhu/azure-sdk-tools that referenced this pull request Dec 3, 2020

Sync eng/common directory with azure-sdk-tools repository for Tools PR …

44fad21

…Azure#850 (Azure#1077)

Simplify New-TestResources usage #1077

Simplify New-TestResources usage #1077

Conversation

pakrym commented Oct 6, 2020 • edited

pakrym commented Oct 6, 2020

heaths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Oct 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weshaggard Oct 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwill-ms commented Oct 6, 2020

azure-sdk commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chidozieononiwu commented Oct 8, 2020

pakrym commented Oct 6, 2020 •

edited

weshaggard Oct 8, 2020 •

edited