Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
Pan-Qi
added
autorest v4 migration
|
To the author of the pull request, |
Fix error: Expression of type 'System.Management.Automation.ErrorRecord' cannot be used for assignment to type 'Microsoft.Azure.PowerShell.Cmdlets.Migrate.Models.IEnableMigrationInput'
There was a problem hiding this comment.
Pull request overview
This PR migrates the Az.Migrate module from AutoRest v3 to AutoRest v4. The changes primarily involve:
Changes:
- Updated type namespaces from versioned API-specific namespaces (e.g.,
Microsoft.Azure.PowerShell.Cmdlets.Migrate.Models.Api20250801.*) to non-versioned namespaces (e.g.,Microsoft.Azure.PowerShell.Cmdlets.Migrate.Models.*) - Regenerated help documentation files to reflect the namespace changes
- Updated AutoRest configuration in README.md to use AutoRest v4
- Added new cmdlet
Update-AzMigrateReplicationProtectionCluster - Fixed a typo in
Restart-AzMigrateServerReplication.md("replcating" → "replicating")
Reviewed changes
Copilot reviewed 129 out of 129 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Migrate/Migrate/ChangeLog.md | Added changelog entry for AutoRest v4 migration |
| src/Migrate/Migrate/Az.Migrate.psd1 | Updated module manifest with new cmdlet and generation date |
| src/Migrate/Migrate/help/*.md | Updated help documentation with simplified type names |
| src/Migrate/Migrate.Autorest/custom/*.ps1 | Updated PowerShell scripts to use non-versioned namespaces |
| src/Migrate/Migrate.Autorest/custom/*.cs | Updated C# custom classes with new namespaces |
| src/Migrate/Migrate.Autorest/test/*.ps1 | Updated test files with new type references |
| src/Migrate/Migrate.Autorest/README.md | Updated AutoRest configuration and directives |
| src/Migrate/Migrate.sln | Updated project GUID |
| - Additional information about change #1 | ||
| --> | ||
| ## Upcoming Release | ||
| * Improved user experience and consistency. This may introduce breaking changes. Please refer to [here](https://go.microsoft.com/fwlink/?linkid=2340249). |
There was a problem hiding this comment.
The ChangeLog entry needs more detail. According to the coding guidelines (CodingGuidelineID: 1000003), changelog entries should be written from the user's perspective and explain what changed and how it affects their usage. The current entry is too vague with just a link. Please add specific details about what improvements were made and what breaking changes users should expect.
Why was this added? |
|
Changes look good. In some files, like e.g. https://github.com/Azure/azure-powershell/pull/28721/changes#diff-58ae8c9b84dbd0266820cddfd30b7356ae7609d91b2ef04ab3cf4a5821731837, subscriptionId handling has been explicitly added, where as in other files it has not been. In what scenarios is this change needed? |
| - from: Microsoft.RecoveryServices/SiteRecovery/stable/2025-08-01/service.json | ||
| where: | ||
| verb: Invoke$ | ||
| subject: ^ResyncReplicationMigrationItem |
There was a problem hiding this comment.
ResyncReplicationMigrationItem is mentioned above as well, can we not add the new variant in above line ?
There was a problem hiding this comment.
ok I had removed the duplicate variant from it
| - from: Microsoft.Migrate/preview/2018-09-01-preview/migrate.json | ||
| where: | ||
| verb: Remove | ||
| subject: VCenterVcenter |
There was a problem hiding this comment.
I don't think the spelling of subject is correct. VCenter is mentioned in next block as well.
There was a problem hiding this comment.
this one is to remove a file that was generated wrongly, this block cannot be removed
| suppress-format: true | ||
| - where: | ||
| verb: Update | ||
| subject: ReplicationProtectionCluster |
There was a problem hiding this comment.
This is removing the cmdlet, but in comments it's said added.
There was a problem hiding this comment.
yup this is removing the cmdlet, which I followed up with Abhishek to confirm this cmdlet is not needed.
This reverts commit 956998f.
JiaSeng-v
added
autorest v4 migration
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 153 out of 163 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (8)
src/Migrate/Migrate.Autorest/test/Set-AzMigrateServerReplication.Tests.ps1:1
Get-AzMigrateServerReplicationis being called with-SqlServerLicenseType, which appears to be a parameter forSet-AzMigrateServerReplication, notGet-AzMigrateServerReplication. This will fail parameter binding at runtime. Remove-SqlServerLicenseTypefrom the GET call (keep it on the SET call), or switch to the correct cmdlet/parameter if the intent was different.
src/Migrate/Migrate.Autorest/test/Remove-AzMigrateProject.Recording.json:1- The committed HTTP recordings contain tenantId/objectId and other identifying metadata in headers. Even if not secrets, these are environment-identifying and typically should be sanitized/filtered consistently (similar to
Authorization). Consider adding these header fields to the recording sanitizer (or removing them from recorded output) to avoid leaking tenant/user identifiers and to reduce churn across re-recordings.
src/Migrate/Migrate.Autorest/test/utils.ps1:1 - The test environment setup hardcodes specific subscription/resource IDs and resource group names. This makes tests brittle for other developers/CI environments and increases the chance of committing environment-specific identifiers. Prefer loading these values from
localEnv.json/pipeline variables (and/or using sanitized placeholders for recordings) so tests can be replayed/recorded consistently across environments.
tools/StaticAnalysis/Exceptions/Az.Migrate/ExampleIssues.csv:1 - This exceptions CSV contains machine/agent-specific absolute paths (e.g.,
D:\\a\\_work\\...) and also includes duplicated exception rows (e.g.,Get-AzMigrateLocalJobappears twice). To keep the exception list stable across environments, store only repo-relative paths (or omitExtentpaths if allowed by the tooling) and deduplicate identical entries.
tools/StaticAnalysis/Exceptions/Az.Migrate/ExampleIssues.csv:1 - This exceptions CSV contains machine/agent-specific absolute paths (e.g.,
D:\\a\\_work\\...) and also includes duplicated exception rows (e.g.,Get-AzMigrateLocalJobappears twice). To keep the exception list stable across environments, store only repo-relative paths (or omitExtentpaths if allowed by the tooling) and deduplicate identical entries.
src/Migrate/Migrate.Autorest/test/Update-AzMigrateReplicationProtectionCluster.Tests.ps1:1 - All tests for
Update-AzMigrateReplicationProtectionClusterare currently skipped and contain onlyNotImplementedExceptionplaceholders. If this cmdlet is part of the v4 migration surface, please replace these with at least one real test (playback/mock or LiveOnly) that validates parameter binding and a successful response shape; otherwise, consider removing the placeholder test file to avoid giving a false sense of coverage.
src/Migrate/Migrate.Autorest/test/Update-AzMigrateReplicationProtectionCluster.Tests.ps1:1 - All tests for
Update-AzMigrateReplicationProtectionClusterare currently skipped and contain onlyNotImplementedExceptionplaceholders. If this cmdlet is part of the v4 migration surface, please replace these with at least one real test (playback/mock or LiveOnly) that validates parameter binding and a successful response shape; otherwise, consider removing the placeholder test file to avoid giving a false sense of coverage.
src/Migrate/Migrate/help/Get-AzMigrateSite.md:1 - The help shows three near-identical identity parameter sets (
SubscriptionInputObject,Subscription1InputObject,Subscription2InputObject), which is confusing for users and suggests a generation/config issue. Consider consolidating these into a single identity parameter set (one...InputObjectparameter) or adjusting the AutoRest configuration so only the intended variant(s) are emitted and documented.
| [Microsoft.Azure.PowerShell.Cmdlets.Migrate.ModelCmdletAttribute()] | ||
| [OutputType([Microsoft.Azure.PowerShell.Cmdlets.Migrate.Models.IVMwareCbtNicInput])] | ||
| [CmdletBinding(DefaultParameterSetName = 'VMwareCbt', PositionalBinding = $false, SupportsShouldProcess, ConfirmImpact = 'Medium')] |
There was a problem hiding this comment.
The file name includes a trailing space (New-AzMigrateTestNicMapping .ps1). This is error-prone on different shells/OSes and can break module packaging/import and build scripts. Rename the file to remove the trailing space and ensure any references/manifests point to the corrected path.
| # Code generated by Microsoft (R) AutoRest Code Generator.Changes may cause incorrect behavior and will be lost if the code | ||
| # is regenerated. |
There was a problem hiding this comment.
This file lives under custom/, but the header states it is code-generated and will be lost if regenerated. If this is intended to be a hand-maintained customization layer, this header is misleading; update it to reflect the file’s ownership (custom vs generated) to prevent accidental overwrites or confusion during regeneration.
| # Code generated by Microsoft (R) AutoRest Code Generator.Changes may cause incorrect behavior and will be lost if the code | |
| # is regenerated. | |
| # This file resides in the 'custom' folder and is intended to be hand-maintained | |
| # as part of the Azure Migrate PowerShell customization layer. It will not be | |
| # overwritten by AutoRest code generation. |
Description
Preannouncement PR: #29131
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense.ChangeLog.mdif no new release is required, such as fixing test case only.