Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
bf40356 to
345bc0f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR upgrades the Azure PowerShell Websites module to use AutoRest v4, introducing various new features and breaking changes.
- Upgrades the code generator from AutoRest v3 to v4, bringing new parameter sets and parameter types
- Updates cmdlet signatures, particularly for Static Web App cmdlets with new identity management parameters
- Changes many API response models from versioned namespaces to unversioned interfaces
Reviewed Changes
Copilot reviewed 112 out of 112 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/StaticAnalysis/Exceptions/Az.LoadTesting/BreakingChangeIssues.csv | Documents breaking changes including type changes, removed parameters, and removed parameter sets |
| src/Websites/Websites/help/*.md | Updates documentation for cmdlets with new parameter sets, type changes, and corrected descriptions |
| src/Websites/Websites/ChangeLog.md | Documents the upgrade and breaking changes for New-AzStaticWebApp identity parameters |
| src/Websites/Websites/Az.Websites.psd1 | Updates module metadata including required Az.Accounts version and file paths |
| ### Example 2: Create or updates the app settings of a static site build by pipeline | ||
| ```powershell | ||
| Get-AzStaticWebAppBuildAppSetting -ResourceGroupName resourceGroup -Name staticweb00 -EnvironmentName 'default' | New-AzStaticWebAppBuildAppSetting -AppSetting @{'buildsetting1' = 'someval'; 'buildsetting2' = 'someval2' } | ||
| Get-AzStaticWebAppBuildAppSetting -ResourceGroupName resourceGroup -Name taticweb00 -EnvironmentName 'default' | New-AzStaticWebAppBuildAppSetting -AppSetting @{'buildsetting1' = 'someval'; 'buildsetting2' = 'someval2' } |
There was a problem hiding this comment.
There's a typo in the example: 'taticweb00' should be 'staticweb00' to match the expected static web app naming convention.
| Get-AzStaticWebAppBuildAppSetting -ResourceGroupName resourceGroup -Name taticweb00 -EnvironmentName 'default' | New-AzStaticWebAppBuildAppSetting -AppSetting @{'buildsetting1' = 'someval'; 'buildsetting2' = 'someval2' } | |
| Get-AzStaticWebAppBuildAppSetting -ResourceGroupName resourceGroup -Name staticweb00 -EnvironmentName 'default' | New-AzStaticWebAppBuildAppSetting -AppSetting @{'buildsetting1' = 'someval'; 'buildsetting2' = 'someval2' } |
|
To the author of the pull request, |
|
/azp run |
|
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. |
|
/azp run |
|
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. |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 28268 in repo Azure/azure-powershell |
|
@mishapos and @skylathadani-ms can you guys review too? |
| Get-AzStaticWebAppBuildAppSetting -ResourceGroupName resourceGroup -Name taticweb00 -EnvironmentName 'default' | New-AzStaticWebAppBuildAppSetting -AppSetting @{'buildsetting1' = 'someval'; 'buildsetting2' = 'someval2' } | ||
| ``` |
There was a problem hiding this comment.
Example uses a likely typo in the static site name ("taticweb00"), which will cause copy/paste failures. Please correct it to the intended site name used elsewhere in the examples.
| Description for update a user entry with the listed roles | ||
|
|
There was a problem hiding this comment.
The synopsis/description text is grammatically incorrect ("Description for update a user entry...") and reads like an unedited template. Please change it to a proper verb form (e.g., "Updates a user entry...") for the reference help.
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
| // Code generated by Microsoft (R) AutoRest Code Generator.Changes may cause incorrect behavior and will be lost if the code |
There was a problem hiding this comment.
Minor typo in the header comment: missing space after the period in "Code Generator.Changes". Please add the missing space to keep the generated header readable.
| // Code generated by Microsoft (R) AutoRest Code Generator.Changes may cause incorrect behavior and will be lost if the code | |
| // Code generated by Microsoft (R) AutoRest Code Generator. Changes may cause incorrect behavior and will be lost if the code |
| Description for update a new static site in an existing resource group, or update an existing static site. | ||
|
|
||
| ### [Update-AzStaticWebAppUser](Update-AzStaticWebAppUser.md) | ||
| Description for Updates a user entry with the listed roles | ||
| Description for update a user entry with the listed roles |
There was a problem hiding this comment.
This synopsis line is grammatically incorrect and misleading ("update a new static site..."). Please adjust it to accurately describe the cmdlet (it updates an existing static site; it doesn't "update a new" one).
| # Modules that must be imported into the global environment prior to importing this module | ||
| RequiredModules = @(@{ModuleName = 'Az.Accounts'; ModuleVersion = '4.2.0'; }) | ||
| RequiredModules = @(@{ModuleName = 'Az.Accounts'; ModuleVersion = '5.1.1'; }) | ||
|
|
There was a problem hiding this comment.
Module dependency versions are inconsistent across the Websites module artifacts: the manifest requires Az.Accounts 5.1.1, but the AutoRest README states 2.7.5+ and the .nuspec depends on 2.7.5. Please align these (and ensure the chosen minimum is reflected consistently in psd1, nuspec, and README) to avoid install/import mismatches for users.
| - 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 Upcoming Release entry is too generic for users and doesn't clearly describe the actual change (AutoRest v4 migration) or reference the related announcement PRs/issues. Please update the entry to explicitly state what's changing for Az.Websites users and include relevant issue/PR references.
|
|
||
| ### [New-AzStaticWebApp](New-AzStaticWebApp.md) | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for create a new static site in an existing resource group, or create an existing static site. |
There was a problem hiding this comment.
This description is grammatically incorrect and also changes the meaning: "or create an existing static site" should be "or update an existing static site". Please fix the synopsis/description text so it accurately reflects the cmdlet behavior.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| ## SYNOPSIS | ||
| Description for Creates or updates the app settings of a static site. | ||
| Description for create the app settings of a static site. | ||
|
|
There was a problem hiding this comment.
The SYNOPSIS text is grammatically incorrect ("Description for create …") and reads like a generation artifact. Please update it to a proper sentence (e.g., "Creates or updates the app settings of a static site.") to match the cmdlet behavior and the examples below.
| ### [New-AzStaticWebApp](New-AzStaticWebApp.md) | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for create a new static site in an existing resource group, or create an existing static site. | ||
|
|
||
| ### [New-AzStaticWebAppBuildAppSetting](New-AzStaticWebAppBuildAppSetting.md) | ||
| Description for Creates or updates the app settings of a static site build. | ||
| Description for create the app settings of a static site build. | ||
|
|
||
| ### [New-AzStaticWebAppBuildFunctionAppSetting](New-AzStaticWebAppBuildFunctionAppSetting.md) | ||
| Description for Creates or updates the function app settings of a static site build. | ||
| Description for create the function app settings of a static site build. | ||
|
|
||
| ### [New-AzStaticWebAppCustomDomain](New-AzStaticWebAppCustomDomain.md) | ||
| Description for Creates a new static site custom domain in an existing resource group and static site. | ||
| Description for create a new static site custom domain in an existing resource group and static site. | ||
|
|
||
| ### [New-AzStaticWebAppFunctionAppSetting](New-AzStaticWebAppFunctionAppSetting.md) | ||
| Description for Creates or updates the function app settings of a static site. | ||
| Description for create the function app settings of a static site. | ||
|
|
||
| ### [New-AzStaticWebAppSetting](New-AzStaticWebAppSetting.md) | ||
| Description for Creates or updates the app settings of a static site. | ||
| Description for create the app settings of a static site. | ||
|
|
||
| ### [New-AzStaticWebAppUserRoleInvitationLink](New-AzStaticWebAppUserRoleInvitationLink.md) | ||
| Description for Creates an invitation link for a user with the role | ||
| Description for create an invitation link for a user with the role |
There was a problem hiding this comment.
The cmdlet list entries for the Static Web App cmdlets contain incorrect/awkward descriptions (e.g., "create … or create an existing …" and missing punctuation). This reads like a generation artifact and is misleading (an existing static site is typically updated, not created). Please adjust these descriptions to accurate, grammatical summaries (e.g., "Creates a new static site, or updates an existing static site.").
| ### [Update-AzStaticWebApp](Update-AzStaticWebApp.md) | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for update a new static site in an existing resource group, or update an existing static site. | ||
|
|
||
| ### [Update-AzStaticWebAppUser](Update-AzStaticWebAppUser.md) | ||
| Description for Updates a user entry with the listed roles | ||
| Description for update a user entry with the listed roles |
There was a problem hiding this comment.
The descriptions for Update-AzStaticWebApp and Update-AzStaticWebAppUser are grammatically incorrect ("update a new …") and misleading. Please update these entries to accurate summaries (e.g., "Creates a new … or updates an existing …" and "Updates a user entry …").
| ## SYNOPSIS | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for create a new static site in an existing resource group, or create an existing static site. | ||
|
|
There was a problem hiding this comment.
The SYNOPSIS/DESCRIPTION text says "create … or create an existing …", which is grammatically incorrect and also misleading for users (existing resources are typically updated). Please revise to an accurate sentence (e.g., "Creates a new static site, or updates an existing static site.").
| ## SYNOPSIS | ||
| Description for Creates a new static site custom domain in an existing resource group and static site. | ||
| Description for create a new static site custom domain in an existing resource group and static site. | ||
|
|
There was a problem hiding this comment.
The SYNOPSIS text is grammatically incorrect ("Description for create …"). Please update it to a proper sentence such as "Creates a new static site custom domain in an existing resource group and static site."
| # Remove variant | ||
| # Following is two common directive which are normally required in all the RPs | ||
| # 1. Remove the unexpanded parameter set | ||
| # 2. For New-* cmdlets, ViaIdentity is not required, so CreateViaIdentityExpanded is removed as well | ||
| - where: | ||
| variant: ^CreateViaIdentityExpanded$|^Create$|^CreateViaIdentity$|^Update$|^UpdateViaIdentity$ | ||
| # We got to keep the Create variant of CustomDomain because it's special that it doesn't have a | ||
| # CreateExpanded variant, because the only parameters are all in URL rather than request body | ||
| subject: CustomDomain | ||
|
|
||
| remove: true | ||
| - where: | ||
| verb: Test | ||
| variant: ^Validate$|^ValidateViaIdentity$ | ||
| # We got to keep the Create variant of CustomDomain because it's special that it doesn't have a | ||
| # CreateExpanded variant, because the only parameters are all in URL rather than request body | ||
| subject: CustomDomain | ||
| remove: true | ||
|
|
||
| - where: | ||
| variant: ^Create$|^CreateViaIdentity$|^Update$|^UpdateViaIdentity$ | ||
| # We got to keep the Create variant of CustomDomain because it's special that it doesn't have a | ||
| # CreateExpanded variant, because the only parameters are all in URL rather than request body | ||
| subject: BuildAppSetting | ||
| remove: true | ||
|
|
||
| - where: | ||
| variant: ^Create$|^CreateViaIdentity$|^Update$|^UpdateViaIdentity$ | ||
| # We got to keep the Create variant of CustomDomain because it's special that it doesn't have a | ||
| # CreateExpanded variant, because the only parameters are all in URL rather than request body | ||
| subject: FunctionAppSetting | ||
| remove: true | ||
|
|
||
| - where: | ||
| variant: ^Create$|^CreateViaIdentity$|^Update$|^UpdateViaIdentity$ | ||
| # We got to keep the Create variant of CustomDomain because it's special that it doesn't have a | ||
| # CreateExpanded variant, because the only parameters are all in URL rather than request body | ||
| subject: Setting | ||
| subject: CustomDomain|BuildAppSetting|FunctionAppSetting|Setting|BuildFunctionAppSetting|UserRoleInvitationLink | ||
| variant: ^Create(?!.*?(Expanded|JsonFilePath|JsonString)) | ||
| remove: true |
There was a problem hiding this comment.
The comment says that for New-* cmdlets the CreateViaIdentityExpanded variant is removed, but the directives only remove CreateViaIdentityExpanded for CustomDomain and the generated help still includes CreateViaIdentityExpanded for other New-AzStaticWebApp* cmdlets (e.g., settings). Please align the comment with the actual directive behavior (or expand the directive if the intent is to remove these variants across the listed subjects).
| ## SYNOPSIS | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for update a new static site in an existing resource group, or update an existing static site. | ||
|
|
There was a problem hiding this comment.
The SYNOPSIS is grammatically incorrect/misleading ("update a new static site…"). Please change it to an accurate description (e.g., "Updates an existing static site." or "Creates or updates a static site." depending on actual behavior).
| ## SYNOPSIS | ||
| Description for Updates a user entry with the listed roles | ||
| Description for update a user entry with the listed roles | ||
|
|
There was a problem hiding this comment.
The SYNOPSIS is grammatically incorrect ("Description for update …"). Please update it to a proper sentence such as "Updates a user entry with the listed roles."
| "Module","ClassName","Target","Severity","ProblemId","Description","Remediation" | ||
| "Az.Websites","Get-AzStaticWebApp","Get-AzStaticWebApp","0","3000","The type of property 'SkuCapability' of type 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.Api20201201.IStaticSiteArmResource' has changed from 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.ICapability' to 'System.Collections.Generic.List`1[Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.ICapability]'.","Change the type of property 'SkuCapability' back to 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.ICapability'." | ||
| "Az.Websites","Get-AzStaticWebApp","Get-AzStaticWebApp","0","3000","The type of property 'PrivateEndpointConnection' of type 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.Api20201201.IStaticSiteArmResource' has changed from 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.IResponseMessageEnvelopeRemotePrivateEndpointConnection' to 'System.Collections.Generic.List`1[Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.IResponseMessageEnvelopeRemotePrivateEndpointConnection]'.","Change the type of property 'PrivateEndpointConnection' back to 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.IResponseMessageEnvelopeRemotePrivateEndpointConnection'." | ||
| "Az.Websites","Get-AzStaticWebApp","Get-AzStaticWebApp","0","3000","The type of property 'UserProvidedFunctionApp' of type 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.Api20201201.IStaticSiteArmResource' has changed from 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.IStaticSiteUserProvidedFunctionApp' to 'System.Collections.Generic.List`1[Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.IStaticSiteUserProvidedFunctionApp]'.","Change the type of property 'UserProvidedFunctionApp' back to 'Microsoft.Azure.PowerShell.Cmdlets.Websites.Models.IStaticSiteUserProvidedFunctionApp'." |
There was a problem hiding this comment.
This exception file is under tools/StaticAnalysis/Exceptions/Az.LoadTesting/ but all entries are for "Module" = "Az.Websites". While the static analysis tooling may still pick it up, this placement is confusing and makes the Websites breaking-change exceptions hard to discover/maintain. Please move these entries into tools/StaticAnalysis/Exceptions/Az.Websites/BreakingChangeIssues.csv (the Az.Websites exceptions folder already exists).
| <# | ||
| .Synopsis | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for create a new static site in an existing resource group, or create an existing static site. | ||
| .Description | ||
| Description for Creates a new static site in an existing resource group, or updates an existing static site. | ||
| Description for create a new static site in an existing resource group, or create an existing static site. | ||
| .Example |
There was a problem hiding this comment.
The comment-based help .Synopsis/.Description text is grammatically incorrect and misleading ("create … or create an existing …"). Please update it to an accurate sentence (e.g., "Creates a new static site, or updates an existing static site.") so Get-Help New-AzStaticWebApp reads correctly.
Description
Preannouncement PR:
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.