-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Fixed default parameter set for migration #29049
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
base: main
Are you sure you want to change the base?
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
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.
Pull request overview
This pull request fixes a parameter set issue in the Start-AzFrontDoorCdnProfilePrepareMigration cmdlet. The cmdlet now correctly defaults to the 'CreateExpanded' parameter set when the MigrationWebApplicationFirewallMapping parameter is not provided, and properly handles null/empty WAF mapping parameters.
Changes:
- Added DefaultParameterSetName='CreateExpanded' to the cmdlet binding to ensure proper parameter set selection
- Enhanced null/empty parameter handling for MigrationWebApplicationFirewallMapping with safer validation logic
- Updated help documentation to reflect the corrected default parameter set
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Cdn/Cdn.Autorest/custom/Start-AzFrontDoorCdnProfilePrepareMigration.ps1 | Added DefaultParameterSetName and improved null handling for WAF mapping parameter |
| src/Cdn/Cdn/help/Start-AzFrontDoorCdnProfilePrepareMigration.md | Updated help documentation to show CreateExpanded as the default parameter set |
| src/Cdn/Cdn.Autorest/docs/Start-AzFrontDoorCdnProfilePrepareMigration.md | Updated AutoRest docs to show CreateExpanded as the default parameter set |
| src/Cdn/Cdn/ChangeLog.md | Added changelog entry for the fix |
| src/Cdn/Cdn/Az.Cdn.psd1 | Updated generated date and formatting (auto-generated changes) |
| src/Cdn/Cdn.sln | Updated project GUID (auto-generated change) |
| src/Cdn/Cdn.Autorest/generate-info.json | Updated generate ID (auto-generated change) |
| src/Cdn/Cdn.Autorest/docs/Az.Cdn.md | Updated module GUID (auto-generated change) |
| src/Cdn/Cdn.Autorest/Properties/AssemblyInfo.cs | Updated assembly version from 5.0.1 to 6.0.0 |
| --> | ||
|
|
||
| ## Upcoming Release | ||
| * Fixed default parameter set issue. |
Copilot
AI
Jan 11, 2026
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.
The ChangeLog entry "Fixed default parameter set issue" is too vague and not user-focused. According to the coding guidelines for ChangeLog.md files, entries should be written from the user's perspective and explain what changed and how it affects their usage. Consider rephrasing to something like: "Fixed parameter set selection for 'Start-AzFrontDoorCdnProfilePrepareMigration' cmdlet to properly default to 'CreateExpanded' when 'MigrationWebApplicationFirewallMapping' is not provided."
| if (-not ${MigrationWebApplicationFirewallMapping} -or ${MigrationWebApplicationFirewallMapping}.count -eq 0) { | ||
| Write-Debug("No WAF policies to validate.") | ||
| return | ||
| } | ||
|
|
||
| if (${MigrationWebApplicationFirewallMapping}.count -gt 0) { |
Copilot
AI
Jan 11, 2026
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.
The ValidateWafPolicies function has redundant logic. Lines 436-439 check if MigrationWebApplicationFirewallMapping is null or empty and return early, but then line 441 checks the same condition again. The inner if statement on line 441 will always be true when reached since we've already returned early if count is 0. Consider removing the redundant check on line 441.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@Ptnan7 The default parameter set has been updated which causes breaking change. Can you please confirm it is really a breaking change? If so, this PR can only be merged during bc window. |
|
@vidai-msft |
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.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
| } else { | ||
| # In CreateExpanded, if Front Door has WAF policies, user must use MigrateExpanded parameter set | ||
| if ($allPoliciesWithWAF.count -gt 0) { | ||
| throw "The Front Door has $($allPoliciesWithWAF.count) WAF policy/policies associated. Please provide the -MigrationWebApplicationFirewallMapping parameter with WAF policy mappings." |
Copilot
AI
Jan 12, 2026
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.
The error message uses "policy/policies" which appears awkward. Consider rephrasing to be more grammatically clear, such as: "The Front Door has $(
| throw "The Front Door has $($allPoliciesWithWAF.count) WAF policy/policies associated. Please provide the -MigrationWebApplicationFirewallMapping parameter with WAF policy mappings." | |
| throw "The Front Door has $($allPoliciesWithWAF.count) associated WAF $(if ($allPoliciesWithWAF.count -eq 1) { 'policy' } else { 'policies' }). Please provide the -MigrationWebApplicationFirewallMapping parameter with WAF policy mappings." |
| # In MigrateExpanded, MigrationWebApplicationFirewallMapping is mandatory | ||
| # Validate the count matches | ||
| if (${MigrationWebApplicationFirewallMapping}.count -ne $allPoliciesWithWAF.count) { | ||
| throw "MigrationWebApplicationFirewallMapping parameter instance should be equal to the number of WAF policy instance in the profile. Expected: $($allPoliciesWithWAF.count), Provided: $($MigrationWebApplicationFirewallMapping.count)" |
Copilot
AI
Jan 12, 2026
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.
The error message enhancement provides better clarity by showing both expected and provided counts. However, consider using consistent terminology. The message says "parameter instance" which might be unclear to users. Consider rewording to: "The number of WAF mapping entries must match the number of WAF policies in the profile. Expected: $(
| throw "MigrationWebApplicationFirewallMapping parameter instance should be equal to the number of WAF policy instance in the profile. Expected: $($allPoliciesWithWAF.count), Provided: $($MigrationWebApplicationFirewallMapping.count)" | |
| throw "The number of WAF mapping entries must match the number of WAF policies in the profile. Expected: $($allPoliciesWithWAF.count), Provided: $($MigrationWebApplicationFirewallMapping.count)" |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@Ptnan7 You changed the default parameter set and also the param -MigrationWebApplicationFirewallMapping was updated from optional to required. These are considered as breaking changes for sure. This PR is only allowed to be merged during Build or Ignite release. |
|
To the author of the pull request, |
Description
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.