-
Notifications
You must be signed in to change notification settings - Fork 177
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
Ensure test principal creation succeeds properly #1944
Conversation
The following pipelines have been queued for testing: |
Trying out a pipeline that uses New-TestResources.ps1 Green. Doing a template release run just to ensure I'm not missing anything, then starting the merge process across the repos. |
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
I think this breaks when an already existing baseName is used, whether passed explicitly or defaulted: New-AzADServicePrincipal: C:\Users\jolov\src\r\Azure\azure-sdk-for-net\eng\common\TestResources\New-TestResources.ps1:292 |
@@ -289,7 +289,7 @@ try { | |||
$AzureTestPrincipal | |||
} else { | |||
Log "TestApplicationId was not specified; creating a new service principal in subscription '$SubscriptionId'" | |||
$global:AzureTestPrincipal = New-AzADServicePrincipal -Role Owner -Scope "/subscriptions/$SubscriptionId" | |||
$global:AzureTestPrincipal = New-AzADServicePrincipal -Role Owner -Scope "/subscriptions/$SubscriptionId" -DisplayName "test-resources-$($baseName).microsoft.com" |
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.
@benbp Will this display name cause problems in other clouds that aren't using the microsoft public tenant?
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 think it will cause functional problems, as it's just a display name, but it would make sense to see if we can grab the suffix out of the tenant/authority host info in the context and use that instead (or just not have the display name include .com
).
Pulled directly from @annelo-msft Azure/azure-sdk-for-net#23591