[Eng] Fix parameterset name in publish-rmmodule #27814
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull Request Overview
This PR corrects parameter set handling in the Publish-RMModules function and standardizes tooling versions.
- Added advanced function binding and a formal
param()block toGet-ApiKey - Included the
$ApiKeyparameter in the “ByName” parameter set and guarded against empty values - Bumped Node.js versions in multiple Azure pipeline YAMLs from
18.20.7/14.17.1to18.20.8
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/ModulePublisher.psm1 | Formalized Get-ApiKey as an advanced function; expanded parameter sets in Publish-RMModules and added empty-value guard for $ApiKey |
| .azure-pipelines/windows-powershell.yml | Updated NodeTool versionSpec entries to 18.20.8 |
| .azure-pipelines/powershell-core.yml | Updated NodeTool versionSpec entries to 18.20.8 |
| .azure-pipelines/azure-powershell-ci.yml | Updated NodeTool versionSpec entries to 18.20.8 |
| $RepoLocation, | ||
|
|
||
| [Parameter(ParameterSetName="ByLocation", Mandatory=$true)] | ||
| [Parameter(ParameterSetName="ByName", Mandatory=$false)] |
There was a problem hiding this comment.
[nitpick] Since parameters default to non-mandatory, you can omit Mandatory=$false here to reduce redundancy and clarify that $ApiKey is optional in this set.
| [Parameter(ParameterSetName="ByName", Mandatory=$false)] | |
| [Parameter(ParameterSetName="ByName")] |
| { | ||
| $RepoLocation = (Get-RepoLocation -repoName $RepoName) | ||
| $ApiKey = (Get-ApiKey -repoName $RepoName) | ||
| if ($null -eq $ApiKey -Or "" -eq $ApiKey) { |
There was a problem hiding this comment.
To simplify and standardize null/empty checks, consider using [string]::IsNullOrEmpty($ApiKey) instead of separate comparisons.
| if ($null -eq $ApiKey -Or "" -eq $ApiKey) { | |
| if ([string]::IsNullOrEmpty($ApiKey)) { |
| displayName: Install Autorest | ||
| inputs: | ||
| versionSpec: '18.20.7' | ||
| versionSpec: '18.20.8' |
There was a problem hiding this comment.
[nitpick] These literal version specs are duplicated across multiple pipeline files—extracting the Node.js version into a shared variable or template can reduce drift and simplify future updates.
| versionSpec: '18.20.8' | |
| versionSpec: ${{ variables.NodeJsVersion }} |
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.