[DO NOT MERGE] Reverted - Changes should be made in TypeSpec repository#55322
[DO NOT MERGE] Reverted - Changes should be made in TypeSpec repository#55322
Conversation
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates APIView validation scripts from API Key-based authentication to Azure AD Bearer token authentication, following the pattern established in azure-sdk-tools PR #13235. The changes remove dependency on API keys stored in Azure DevOps variables and instead use Azure AD tokens obtained through Managed Identity/Service Principal.
Changes:
- Added
Get-ApiViewBearerToken()function toApiView-Helpers.ps1for centralized token acquisition - Updated
Check-ApiReviewStatus()to use Bearer token authentication instead of API Key - Removed
$APIKeyparameters fromValidate-Package.ps1andValidate-All-Packages.ps1 - Removed API Key argument from the pipeline YAML configuration
- Eliminated duplicate
Get-ApiViewBearerToken()function fromCreate-APIReview.ps1by importing fromApiView-Helpers.ps1
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eng/common/scripts/Helpers/ApiView-Helpers.ps1 | Added Get-ApiViewBearerToken() function and updated Check-ApiReviewStatus() to use Bearer token authentication |
| eng/common/scripts/Validate-Package.ps1 | Removed $APIKey parameter and updated function call to Check-ApiReviewStatus |
| eng/common/scripts/Validate-All-Packages.ps1 | Removed $APIKey parameter and updated function call to Check-ApiReviewStatus |
| eng/common/scripts/Create-APIReview.ps1 | Removed duplicate Get-ApiViewBearerToken() function (now imported from ApiView-Helpers.ps1) |
| eng/common/pipelines/templates/steps/validate-all-packages.yml | Removed -APIKey $(azuresdk-apiview-apikey) argument from script invocation |
| } | ||
|
|
||
| function Check-ApiReviewStatus($packageName, $packageVersion, $language, $url, $apiKey, $apiApprovalStatus = $null, $packageNameStatus = $null) | ||
| function Check-ApiReviewStatus($packageName, $packageVersion, $language, $url, $apiApprovalStatus = $null, $packageNameStatus = $null) |
There was a problem hiding this comment.
The function signature for Check-ApiReviewStatus has been changed to remove the $apiKey parameter, but there is still a caller in eng/common/scripts/Prepare-Release.ps1 (line 187) that passes the -apiKey parameter. This will cause a breaking change and runtime errors when Prepare-Release.ps1 is executed.
The Prepare-Release.ps1 script at line 187 calls:
Check-ApiReviewStatus -PackageName $fullPackageNameInApiView -packageVersion $newVersion -Language $LanguageDisplayName -url $url -apiKey $apiKey
This needs to be updated to remove the -apiKey $apiKey parameter to match the new function signature. Additionally, lines 184-185 that retrieve the API key from KeyVault should be removed since they are no longer needed.
| function Check-ApiReviewStatus($packageName, $packageVersion, $language, $url, $apiApprovalStatus = $null, $packageNameStatus = $null) | |
| function Check-ApiReviewStatus($packageName, $packageVersion, $language, $url, $apiApprovalStatus = $null, $packageNameStatus = $null, $apiKey = $null) |
There was a problem hiding this comment.
Thank you for catching this breaking change. This was one of the reasons all changes have been reverted in commit 8beda25. The eng/common directory should not be modified in this repository as it's synced from azure-sdk-tools.
|
@copilot we shouldn't be changing anything under eng/common. This is the script we need to fix - https://github.com/microsoft/typespec/blob/main/eng/emitters/scripts/Create-APIReview.ps1 |
Co-authored-by: JoshLove-msft <54595583+JoshLove-msft@users.noreply.github.com>
You're absolutely right. I've reverted all changes to the The original issue was asking to fix the TypeSpec repository's script, not azure-sdk-for-net. This PR should be closed as the work needs to be done in the microsoft/typespec repository instead. |
Description
This PR has been reverted and should not be merged.
After review, it was determined that changes should not be made to the
eng/commondirectory in this repository as these files are synced from the azure-sdk-tools repository. Additionally, the original issue was requesting changes to be made in the microsoft/typespec repository, not azure-sdk-for-net.All changes have been reverted in commit 8beda25.
The original issue requests updating https://github.com/microsoft/typespec/blob/main/eng/emitters/scripts/Create-APIReview.ps1 to use Azure AD Bearer token authentication instead of API Key, following the pattern from azure-sdk-tools PR #13235.
This work needs to be done in the TypeSpec repository, not here.
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.