Skip to content

Conversation

@Pan-Qi
Copy link
Contributor

@Pan-Qi Pan-Qi commented Jul 28, 2025

Description

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • Update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

@azure-client-tools-bot-prd
Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

@Pan-Qi Pan-Qi force-pushed the bernard-migrate-LoadTesting-to-autorest-v4 branch 2 times, most recently from 8e2e91f to 6b1ebc1 Compare August 13, 2025 09:01
@Pan-Qi Pan-Qi added the Contains Breaking Change This PR contains breaking change label Aug 28, 2025
@Pan-Qi Pan-Qi marked this pull request as ready for review August 28, 2025 13:26
@Copilot Copilot AI review requested due to automatic review settings August 28, 2025 13:26
@github-actions
Copy link

To the author of the pull request,
This PR was labeled "Breaking Change Release" because it contains breaking changes.

  • According to our policy, breaking changes can only take place during major release and they must be preannounced.
  • Please follow our guide on the detailed steps.
  • Required: Please fill in the task below to facilitate our contact,you will receive notifications related to breaking changes.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the LoadTesting module from AutoRest v3 to AutoRest v4, introducing significant changes to the API interface and parameter handling. The migration simplifies managed identity configuration by replacing hashtable-based user identity assignment with string arrays and introduces system-assigned identity toggle parameters.

Key changes include:

  • Updated parameter interface for managed identity operations with new EnableSystemAssignedIdentity and UserAssignedIdentity parameters
  • Generated new parameter sets for JSON file and string inputs across all cmdlets
  • Updated documentation and examples to reflect the new parameter syntax

Reviewed Changes

Copilot reviewed 33 out of 34 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tools/StaticAnalysis/Exceptions/Az.LoadTesting/BreakingChangeIssues.csv Documents breaking changes from v3 to v4 migration including parameter removals and type changes
src/LoadTesting/LoadTesting/help/*.md Updated help documentation with new parameter sets and simplified identity management syntax
src/LoadTesting/LoadTesting/ChangeLog.md Documents the AutoRest v4 upgrade and breaking changes to identity parameters
src/LoadTesting/LoadTesting.Autorest/custom/*.ps1 Updated cmdlet implementations with new identity parameter handling logic
src/LoadTesting/LoadTesting.Autorest/test/* Updated test files with new parameter syntax and test environment configurations
src/LoadTesting/LoadTesting.Autorest/README.md Updated AutoRest configuration to disable identity type transforms and add new directives


# calculate IdentityType
$supportsSystemAssignedIdentity = $EnableSystemAssignedIdentity -or (($null -eq $EnableSystemAssignedIdentity) -and ($loadTestingObject.IdentityType.Contains('SystemAssigned')))
$supportsUserAssignedIdentity = ($PSBoundParameters.ContainsKey('UserAssignedIdentity') -and $UserAssignedIdentity.Length -gt 0) -or ((-not $PSBoundParameters.ContainsKey('UserAssignedIdentity')) -and ($loadTestingObject.IdentityType.Contains('UserAssigned')));
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

This complex boolean logic should be simplified for better readability. Consider breaking it into multiple variables or using helper functions to make the condition more understandable.

Suggested change
$supportsUserAssignedIdentity = ($PSBoundParameters.ContainsKey('UserAssignedIdentity') -and $UserAssignedIdentity.Length -gt 0) -or ((-not $PSBoundParameters.ContainsKey('UserAssignedIdentity')) -and ($loadTestingObject.IdentityType.Contains('UserAssigned')));
$hasUserAssignedIdentityParameter = $PSBoundParameters.ContainsKey('UserAssignedIdentity')
$userAssignedIdentityParameterNotEmpty = $hasUserAssignedIdentityParameter -and ($UserAssignedIdentity.Length -gt 0)
$userAssignedIdentityParameterMissing = -not $hasUserAssignedIdentityParameter
$objectHasUserAssignedIdentity = $loadTestingObject.IdentityType.Contains('UserAssigned')
$supportsUserAssignedIdentity = $userAssignedIdentityParameterNotEmpty -or ($userAssignedIdentityParameterMissing -and $objectHasUserAssignedIdentity)

Copilot uses AI. Check for mistakes.
$supportsUserAssignedIdentity = $PSBoundParameters.ContainsKey("UserAssignedIdentity") -and $UserAssignedIdentity.Length -gt 0

# calculate IdentityType
if (($supportsSystemAssignedIdentity -and $supportsUserAssignedIdentity)) {
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The parentheses around the first condition are unnecessary and create inconsistent formatting with the subsequent else-if conditions.

Suggested change
if (($supportsSystemAssignedIdentity -and $supportsUserAssignedIdentity)) {
if ($supportsSystemAssignedIdentity -and $supportsUserAssignedIdentity) {

Copilot uses AI. Check for mistakes.
# calculate IdentityType
$supportsSystemAssignedIdentity = $EnableSystemAssignedIdentity -or (($null -eq $EnableSystemAssignedIdentity) -and ($loadTestingObject.IdentityType.Contains('SystemAssigned')))
$supportsUserAssignedIdentity = ($PSBoundParameters.ContainsKey('UserAssignedIdentity') -and $UserAssignedIdentity.Length -gt 0) -or ((-not $PSBoundParameters.ContainsKey('UserAssignedIdentity')) -and ($loadTestingObject.IdentityType.Contains('UserAssigned')));
if (($supportsSystemAssignedIdentity -and $supportsUserAssignedIdentity)) {
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The parentheses around the first condition are unnecessary and create inconsistent formatting with the subsequent else-if conditions.

Suggested change
if (($supportsSystemAssignedIdentity -and $supportsUserAssignedIdentity)) {
if ($supportsSystemAssignedIdentity -and $supportsUserAssignedIdentity) {

Copilot uses AI. Check for mistakes.
### Example 2: Update an Azure Load Testing resource to use System-Assigned identity for CMK encryption
```powershell
Update-AzLoad -Name sampleres -ResourceGroupName sample-rg -IdentityType "SystemAssigned" -EncryptionIdentity "SystemAssigned" -EncryptionKey "https://sample-akv.vault.azure.net/keys/cmk/2d1ccd5c50234ea2a0858fe148b69cde"
Update-AzLoad -Name sampleres -ResourceGroupName sample-rg -EnableSystemAssignedIdentity $true -EncryptionIdentity "SystemAssigned" -EncryptionKey "https://sample-akv.vault.azure.net/keys/cmk/2d1ccd5c50234ea2a0858fe148b69cde"
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

Using $true with the EnableSystemAssignedIdentity parameter is redundant since it's a Boolean parameter. The parameter should be used as a switch: -EnableSystemAssignedIdentity instead of -EnableSystemAssignedIdentity $true.

Suggested change
Update-AzLoad -Name sampleres -ResourceGroupName sample-rg -EnableSystemAssignedIdentity $true -EncryptionIdentity "SystemAssigned" -EncryptionKey "https://sample-akv.vault.azure.net/keys/cmk/2d1ccd5c50234ea2a0858fe148b69cde"
Update-AzLoad -Name sampleres -ResourceGroupName sample-rg -EnableSystemAssignedIdentity -EncryptionIdentity "SystemAssigned" -EncryptionKey "https://sample-akv.vault.azure.net/keys/cmk/2d1ccd5c50234ea2a0858fe148b69cde"

Copilot uses AI. Check for mistakes.
Comment on lines +201 to +203
$loadTestingObject.IdentityUserAssignedIdentity.Keys | ForEach-Object {
if (-NOT($_ -in $UserAssignedIdentity)) {
$PSBoundParameters.IdentityUserAssignedIdentity.Add($_, $null)
Copy link

Copilot AI Aug 28, 2025

Choose a reason for hiding this comment

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

Consider using a more efficient approach than piping to ForEach-Object for iterating over dictionary keys, such as using a foreach loop which would be faster for this operation.

Suggested change
$loadTestingObject.IdentityUserAssignedIdentity.Keys | ForEach-Object {
if (-NOT($_ -in $UserAssignedIdentity)) {
$PSBoundParameters.IdentityUserAssignedIdentity.Add($_, $null)
foreach ($key in $loadTestingObject.IdentityUserAssignedIdentity.Keys) {
if (-NOT($key -in $UserAssignedIdentity)) {
$PSBoundParameters.IdentityUserAssignedIdentity.Add($key, $null)

Copilot uses AI. Check for mistakes.
@venkatr21 venkatr21 self-requested a review September 3, 2025 13:31
@isra-fel isra-fel added the autorest v4 migration pr migrating module from generated by autorest.powershell v3 to v4 label Oct 2, 2025
@isra-fel
Copy link
Member

/azp run

@azure-pipelines
Copy link
Contributor

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@Pan-Qi Pan-Qi force-pushed the bernard-migrate-LoadTesting-to-autorest-v4 branch from eac4f6f to 0ea9d89 Compare October 24, 2025 03:55
@Pan-Qi Pan-Qi removed the request for review from DanielMicrosoft October 24, 2025 04:06
@Pan-Qi Pan-Qi force-pushed the bernard-migrate-LoadTesting-to-autorest-v4 branch from 0ea9d89 to 5c8cfbf Compare October 24, 2025 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autorest v4 migration pr migrating module from generated by autorest.powershell v3 to v4 Contains Breaking Change This PR contains breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants