From d23dd6035314e6dff5673d7c1a0fa4b6e6bbfdfe Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Wed, 21 Oct 2026 15:40:36 -0600 Subject: [PATCH 1/2] Remove management plan workarounds. --- .../scripts/Get-PackageApprovalStatus.ps1 | 12 +++--- .../tests/Get-PackageApprovalStatus.Tests.ps1 | 29 +++++++++++++- .../templates/stages/archetype-sdk-client.yml | 40 ++----------------- .../Filter-PackageInfoForPackageApproval.ps1 | 34 ---------------- 4 files changed, 36 insertions(+), 79 deletions(-) delete mode 100644 eng/scripts/Filter-PackageInfoForPackageApproval.ps1 diff --git a/eng/common/scripts/Get-PackageApprovalStatus.ps1 b/eng/common/scripts/Get-PackageApprovalStatus.ps1 index 642add267c79..a5ae9282939a 100644 --- a/eng/common/scripts/Get-PackageApprovalStatus.ps1 +++ b/eng/common/scripts/Get-PackageApprovalStatus.ps1 @@ -7,6 +7,7 @@ Invokes the centralized Azure SDK CLI API review release gate and fails unless i .PARAMETER PackageInfoFiles Package-info JSON files containing the package name, version, and optional API hash. +Missing or blank hashes are sent as "dummy" so API Review Hub can decide whether review is required. .PARAMETER RepoOwner The optional GitHub repository owner to query in API Review Hub. @@ -83,25 +84,22 @@ function Write-ApprovalSummary([object] $Response) { } function Test-PackageApproval([string] $PackageName, [string] $PackageVersion, [string] $ApiHash) { + $ApiHash = if ([string]::IsNullOrWhiteSpace($ApiHash)) { "dummy" } else { $ApiHash } $arguments = @( "package", "get-approval-status", "--language", $LanguageShort, "--package-name", $PackageName, "--package-version", $PackageVersion, - "--output", "json" + "--output", "json", + "--api-hash", $ApiHash ) - if (-not [string]::IsNullOrWhiteSpace($ApiHash)) { - $arguments += @("--api-hash", $ApiHash) - } - if (-not [string]::IsNullOrWhiteSpace($RepoOwner)) { $arguments += @("--repo-owner", $RepoOwner) } - $hashDescription = if ([string]::IsNullOrWhiteSpace($ApiHash)) { "not provided" } else { $ApiHash } - Write-Host "Checking package approval: language=$LanguageShort, package=$PackageName, version=$PackageVersion, apiHash=$hashDescription" + Write-Host "Checking package approval: language=$LanguageShort, package=$PackageName, version=$PackageVersion, apiHash=$ApiHash" $formattedArguments = @($arguments | ForEach-Object { Format-CommandArgument $_ }) Write-Host "Command: azsdk $($formattedArguments -join ' ')" diff --git a/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 b/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 index d6a744e3d937..62310a40cc45 100644 --- a/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 +++ b/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 @@ -57,13 +57,38 @@ Describe "Get-PackageApprovalStatus.ps1" { ) -join "|") } - It "omits the API hash when it is unavailable" { + It "uses dummy when the API hash is unavailable" { & $scriptPath -PackageInfoFiles $packageInfoPath - ($global:CapturedAzSdkArguments -join "|") | Should Not Match "--api-hash" + ($global:CapturedAzSdkArguments -join "|") | Should Match "--api-hash\|dummy" ($global:CapturedAzSdkArguments -join "|") | Should Not Match "--repo-owner" } + It "uses dummy for null, empty, and whitespace API hashes" { + foreach ($hash in @($null, "", " ")) { + $packageInfo = Get-Content $packageInfoPath -Raw | ConvertFrom-Json + $packageInfo | Add-Member -NotePropertyName ApiHash -NotePropertyValue $hash -Force + $packageInfo | ConvertTo-Json | Set-Content $packageInfoPath + + & $scriptPath -PackageInfoFiles $packageInfoPath + + ($global:CapturedAzSdkArguments -join "|") | Should Match "--api-hash\|dummy" + } + } + + It "checks management packages without a hash and accepts reviewNotRequired" { + $packageInfo = Get-Content $packageInfoPath -Raw | ConvertFrom-Json + $packageInfo.Name = "azure-mgmt-test" + $packageInfo | Add-Member -NotePropertyName SdkType -NotePropertyValue mgmt + $packageInfo | ConvertTo-Json | Set-Content $packageInfoPath + $global:AzSdkOutput = '{"operation_status":"Succeeded","result":{"isApproved":true,"finalSource":"reviewHub","reason":"reviewNotRequired"}}' + + & $scriptPath -PackageInfoFiles $packageInfoPath + + ($global:CapturedAzSdkArguments -join "|") | Should Match "--package-name\|azure-mgmt-test" + ($global:CapturedAzSdkArguments -join "|") | Should Match "--api-hash\|dummy" + } + It "fails without prompting when the azsdk executable is unavailable" { $missingExecutable = Join-Path $TestDrive "missing-azsdk.exe" $caughtError = $null diff --git a/eng/pipelines/templates/stages/archetype-sdk-client.yml b/eng/pipelines/templates/stages/archetype-sdk-client.yml index 63f1a8a263ac..3e6a9cdcc8ee 100644 --- a/eng/pipelines/templates/stages/archetype-sdk-client.yml +++ b/eng/pipelines/templates/stages/archetype-sdk-client.yml @@ -164,8 +164,8 @@ extends: GenerateApiReviewForManualOnly: ${{ parameters.GenerateApiReviewForManualOnly }} - ${{ if and(eq(variables['System.TeamProject'], 'internal'), or(in(variables['Build.Reason'], 'Manual', ''), and(eq(variables['Build.Reason'], 'IndividualCI'), eq(variables['Build.SourceBranch'], 'refs/heads/main')))) }}: - - job: FilterPackageInfo - displayName: Filter PackageInfo files for package approval + - job: CheckPackageApproval + displayName: Check Package Approval Status dependsOn: - Build_Extended - Analyze @@ -184,39 +184,6 @@ extends: name: $(LINUXPOOL) image: $(LINUXVMIMAGE) os: linux - steps: - - checkout: self - fetchDepth: 1 - fetchTags: false - sparseCheckoutDirectories: eng/scripts - - - download: current - artifact: packages_extended - - - pwsh: | - & '$(Build.SourcesDirectory)/eng/scripts/Filter-PackageInfoForPackageApproval.ps1' ` - -PackageInfoDirectory '$(Pipeline.Workspace)/packages_extended/PackageInfo' ` - -ArtifactsJson $env:ARTIFACTS_JSON ` - -PackageInfoPathPrefix 'packages_extended/PackageInfo' - name: filterPackageInfo - displayName: Filter management plane packages from package approval - env: - ARTIFACTS_JSON: ${{ convertToJson(parameters.Artifacts) }} - - - job: CheckPackageApproval - displayName: Check Package Approval Status - dependsOn: FilterPackageInfo - condition: >- - and( - in(dependencies.FilterPackageInfo.result, 'Succeeded', 'SucceededWithIssues'), - eq(dependencies.FilterPackageInfo.outputs['filterPackageInfo.HasPackageApprovalPackages'], 'True') - ) - variables: - PackageApprovalInfoFiles: $[ dependencies.FilterPackageInfo.outputs['filterPackageInfo.PackageApprovalInfoFiles'] ] - pool: - name: $(LINUXPOOL) - image: $(LINUXVMIMAGE) - os: linux steps: - checkout: self fetchDepth: 1 @@ -233,7 +200,8 @@ extends: - template: /eng/common/pipelines/templates/steps/get-package-approval-status.yml parameters: PackageInfoFiles: - - $(PackageApprovalInfoFiles) + - ${{ each artifact in parameters.Artifacts }}: + - packages_extended/PackageInfo/${{ artifact.name }}.json RepoOwner: Azure variables: diff --git a/eng/scripts/Filter-PackageInfoForPackageApproval.ps1 b/eng/scripts/Filter-PackageInfoForPackageApproval.ps1 deleted file mode 100644 index b9f364297636..000000000000 --- a/eng/scripts/Filter-PackageInfoForPackageApproval.ps1 +++ /dev/null @@ -1,34 +0,0 @@ -[CmdletBinding()] -param ( - [Parameter(Mandatory = $true)] - [string] $PackageInfoDirectory, - - [Parameter(Mandatory = $true)] - [string] $ArtifactsJson, - - [Parameter(Mandatory = $true)] - [string] $PackageInfoPathPrefix -) - -$artifacts = $ArtifactsJson | ConvertFrom-Json -$packageInfoFiles = @() - -foreach ($artifact in $artifacts) { - $packageInfoPath = Join-Path $PackageInfoDirectory "$($artifact.name).json" - if (!(Test-Path -Path $packageInfoPath -PathType Leaf)) { - Write-Host "PackageInfo file was not found; skipping: $packageInfoPath" - continue - } - - $packageInfo = Get-Content -Raw -Path $packageInfoPath | ConvertFrom-Json - if ($packageInfo.SdkType -eq 'mgmt') { - Write-Host "Package approval is not required for management plane SDKs: $($packageInfo.Name)" - continue - } - - $packageInfoFiles += "$PackageInfoPathPrefix/$($artifact.name).json" -} - -$hasPackageApprovalPackages = $packageInfoFiles.Count -gt 0 -Write-Host "##vso[task.setvariable variable=PackageApprovalInfoFiles;isOutput=true]$($packageInfoFiles -join "','")" -Write-Host "##vso[task.setvariable variable=HasPackageApprovalPackages;isOutput=true]$hasPackageApprovalPackages" \ No newline at end of file From a7dbb0c8aaa7bf30eb5e856668d8c64ecc137fb2 Mon Sep 17 00:00:00 2001 From: Travis Prescott Date: Thu, 22 Oct 2026 12:46:34 -0600 Subject: [PATCH 2/2] Remove changes to eng/common. --- .../scripts/Get-PackageApprovalStatus.ps1 | 12 +- .../tests/Get-PackageApprovalStatus.Tests.ps1 | 29 +---- .../steps/build-extended-artifacts.yml | 11 +- eng/scripts/save_package_api_hash.py | 21 ++-- .../tests/test_save_package_api_hash.py | 116 ++++++++++++++++++ 5 files changed, 145 insertions(+), 44 deletions(-) create mode 100644 eng/scripts/tests/test_save_package_api_hash.py diff --git a/eng/common/scripts/Get-PackageApprovalStatus.ps1 b/eng/common/scripts/Get-PackageApprovalStatus.ps1 index a5ae9282939a..642add267c79 100644 --- a/eng/common/scripts/Get-PackageApprovalStatus.ps1 +++ b/eng/common/scripts/Get-PackageApprovalStatus.ps1 @@ -7,7 +7,6 @@ Invokes the centralized Azure SDK CLI API review release gate and fails unless i .PARAMETER PackageInfoFiles Package-info JSON files containing the package name, version, and optional API hash. -Missing or blank hashes are sent as "dummy" so API Review Hub can decide whether review is required. .PARAMETER RepoOwner The optional GitHub repository owner to query in API Review Hub. @@ -84,22 +83,25 @@ function Write-ApprovalSummary([object] $Response) { } function Test-PackageApproval([string] $PackageName, [string] $PackageVersion, [string] $ApiHash) { - $ApiHash = if ([string]::IsNullOrWhiteSpace($ApiHash)) { "dummy" } else { $ApiHash } $arguments = @( "package", "get-approval-status", "--language", $LanguageShort, "--package-name", $PackageName, "--package-version", $PackageVersion, - "--output", "json", - "--api-hash", $ApiHash + "--output", "json" ) + if (-not [string]::IsNullOrWhiteSpace($ApiHash)) { + $arguments += @("--api-hash", $ApiHash) + } + if (-not [string]::IsNullOrWhiteSpace($RepoOwner)) { $arguments += @("--repo-owner", $RepoOwner) } - Write-Host "Checking package approval: language=$LanguageShort, package=$PackageName, version=$PackageVersion, apiHash=$ApiHash" + $hashDescription = if ([string]::IsNullOrWhiteSpace($ApiHash)) { "not provided" } else { $ApiHash } + Write-Host "Checking package approval: language=$LanguageShort, package=$PackageName, version=$PackageVersion, apiHash=$hashDescription" $formattedArguments = @($arguments | ForEach-Object { Format-CommandArgument $_ }) Write-Host "Command: azsdk $($formattedArguments -join ' ')" diff --git a/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 b/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 index 62310a40cc45..d6a744e3d937 100644 --- a/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 +++ b/eng/common/scripts/tests/Get-PackageApprovalStatus.Tests.ps1 @@ -57,38 +57,13 @@ Describe "Get-PackageApprovalStatus.ps1" { ) -join "|") } - It "uses dummy when the API hash is unavailable" { + It "omits the API hash when it is unavailable" { & $scriptPath -PackageInfoFiles $packageInfoPath - ($global:CapturedAzSdkArguments -join "|") | Should Match "--api-hash\|dummy" + ($global:CapturedAzSdkArguments -join "|") | Should Not Match "--api-hash" ($global:CapturedAzSdkArguments -join "|") | Should Not Match "--repo-owner" } - It "uses dummy for null, empty, and whitespace API hashes" { - foreach ($hash in @($null, "", " ")) { - $packageInfo = Get-Content $packageInfoPath -Raw | ConvertFrom-Json - $packageInfo | Add-Member -NotePropertyName ApiHash -NotePropertyValue $hash -Force - $packageInfo | ConvertTo-Json | Set-Content $packageInfoPath - - & $scriptPath -PackageInfoFiles $packageInfoPath - - ($global:CapturedAzSdkArguments -join "|") | Should Match "--api-hash\|dummy" - } - } - - It "checks management packages without a hash and accepts reviewNotRequired" { - $packageInfo = Get-Content $packageInfoPath -Raw | ConvertFrom-Json - $packageInfo.Name = "azure-mgmt-test" - $packageInfo | Add-Member -NotePropertyName SdkType -NotePropertyValue mgmt - $packageInfo | ConvertTo-Json | Set-Content $packageInfoPath - $global:AzSdkOutput = '{"operation_status":"Succeeded","result":{"isApproved":true,"finalSource":"reviewHub","reason":"reviewNotRequired"}}' - - & $scriptPath -PackageInfoFiles $packageInfoPath - - ($global:CapturedAzSdkArguments -join "|") | Should Match "--package-name\|azure-mgmt-test" - ($global:CapturedAzSdkArguments -join "|") | Should Match "--api-hash\|dummy" - } - It "fails without prompting when the azsdk executable is unavailable" { $missingExecutable = Join-Path $TestDrive "missing-azsdk.exe" $caughtError = $null diff --git a/eng/pipelines/templates/steps/build-extended-artifacts.yml b/eng/pipelines/templates/steps/build-extended-artifacts.yml index ce90d0d4cb34..0665ede7dacd 100644 --- a/eng/pipelines/templates/steps/build-extended-artifacts.yml +++ b/eng/pipelines/templates/steps/build-extended-artifacts.yml @@ -82,11 +82,12 @@ steps: ServiceDirectory: ${{ parameters.ServiceDirectory }} AdditionalTestArgs: '--wheel_dir="$(Build.ArtifactStagingDirectory)"' - - script: >- - python eng/scripts/save_package_api_hash.py - --artifact-staging-directory "$(Build.ArtifactStagingDirectory)" - --repo-root "$(Build.SourcesDirectory)" - displayName: 'Update package properties with API hashes' + # Packages without API tokens still need an ApiHash for the shared release gate. + - script: >- + python eng/scripts/save_package_api_hash.py + --artifact-staging-directory "$(Build.ArtifactStagingDirectory)" + --repo-root "$(Build.SourcesDirectory)" + displayName: 'Update package properties with API hashes' - ${{ parameters.BeforePublishSteps }} diff --git a/eng/scripts/save_package_api_hash.py b/eng/scripts/save_package_api_hash.py index 97f0e14b2844..494843d084cc 100644 --- a/eng/scripts/save_package_api_hash.py +++ b/eng/scripts/save_package_api_hash.py @@ -8,13 +8,19 @@ from extract_apiview_metadata import extract_metadata -def update_package_info(metadata_path: pathlib.Path, package_info_path: pathlib.Path) -> None: - metadata = yaml.safe_load(metadata_path.read_text(encoding="utf-8-sig")) - api_hash = metadata.get("apiHash") if isinstance(metadata, dict) else None - if not isinstance(api_hash, str) or not api_hash: - raise ValueError(f"apiHash was not found in {metadata_path}") - +def update_package_info(metadata_path: pathlib.Path | None, package_info_path: pathlib.Path) -> None: + """Store the generated hash, or retain an existing hash and default to 'dummy' without API tokens.""" package_info = json.loads(package_info_path.read_text(encoding="utf-8-sig")) + if metadata_path is not None: + metadata = yaml.safe_load(metadata_path.read_text(encoding="utf-8-sig")) + api_hash = metadata.get("apiHash") if isinstance(metadata, dict) else None + if not isinstance(api_hash, str) or not api_hash.strip(): + raise ValueError(f"apiHash was not found in {metadata_path}") + else: + api_hash = package_info.get("ApiHash") + if not isinstance(api_hash, str) or not api_hash.strip(): + api_hash = "dummy" + package_info["ApiHash"] = api_hash package_info_path.write_text(json.dumps(package_info, indent=2) + "\n", encoding="utf-8") print(f"Stored ApiHash in {package_info_path}") @@ -39,7 +45,8 @@ def main() -> None: # API stub generation intentionally omits management-plane packages, so they have no token file. # Revisit this behavior if management-plane packages are included in API stub generation. if not token_file.is_file(): - print(f"API token file was not found for {package_name}; skipping ApiHash update") + print(f"API token file was not found for {package_name}; retaining ApiHash or using 'dummy'") + update_package_info(None, package_info_path) continue subprocess.run( diff --git a/eng/scripts/tests/test_save_package_api_hash.py b/eng/scripts/tests/test_save_package_api_hash.py new file mode 100644 index 000000000000..1373e9decc1f --- /dev/null +++ b/eng/scripts/tests/test_save_package_api_hash.py @@ -0,0 +1,116 @@ +import json +import pathlib +import subprocess +import sys +import tempfile +import unittest +from unittest.mock import patch + +import yaml + +sys.path.insert(0, str(pathlib.Path(__file__).resolve().parents[1])) + +import save_package_api_hash + + +class SavePackageApiHashTests(unittest.TestCase): + def setUp(self): + self.temp_dir = tempfile.TemporaryDirectory() + self.addCleanup(self.temp_dir.cleanup) + self.artifact_dir = pathlib.Path(self.temp_dir.name) + self.package_info_dir = self.artifact_dir / "PackageInfo" + self.package_info_dir.mkdir() + + def write_package_info(self, name="azure-mgmt-test", **properties): + path = self.package_info_dir / f"{name}.json" + path.write_text( + json.dumps({"Name": name, "Version": "1.0.0", "SdkType": "mgmt", **properties}), + encoding="utf-8-sig", + ) + return path + + def run_main(self): + with patch.object( + sys, + "argv", + [ + "save_package_api_hash.py", + "--artifact-staging-directory", + str(self.artifact_dir), + "--repo-root", + str(self.artifact_dir), + ], + ): + save_package_api_hash.main() + + def test_missing_tokens_default_missing_or_blank_hashes(self): + for properties in ({}, {"ApiHash": None}, {"ApiHash": ""}, {"ApiHash": " "}): + with self.subTest(properties=properties): + path = self.write_package_info(**properties) + with patch.object(save_package_api_hash.subprocess, "run") as export: + self.run_main() + export.assert_not_called() + self.assertEqual( + json.loads(path.read_text(encoding="utf-8")), + {"Name": "azure-mgmt-test", "Version": "1.0.0", "SdkType": "mgmt", "ApiHash": "dummy"}, + ) + + def test_missing_tokens_preserve_existing_hash(self): + path = self.write_package_info(ApiHash="existing-hash") + self.run_main() + self.assertEqual(json.loads(path.read_text(encoding="utf-8"))["ApiHash"], "existing-hash") + + def test_generated_hash_and_dummy_in_same_run(self): + management_path = self.write_package_info() + generated_path = self.write_package_info("azure-test", ApiHash="dummy") + package_dir = self.artifact_dir / "azure-test" + package_dir.mkdir() + token_file = package_dir / "azure-test_python.json" + token_file.write_text("{}", encoding="utf-8") + (package_dir / "api.md").write_text("```\nclass Test:\n```\n", encoding="utf-8") + + with patch.object(save_package_api_hash.subprocess, "run") as export: + self.run_main() + + export.assert_called_once_with( + [ + "pwsh", + str(self.artifact_dir / "eng" / "common" / "scripts" / "Export-APIViewMarkdown.ps1"), + "-TokenJsonPath", + str(token_file), + "-OutputPath", + str(package_dir), + ], + check=True, + ) + metadata = yaml.safe_load((package_dir / "api.metadata.yml").read_text(encoding="utf-8")) + self.assertEqual(json.loads(generated_path.read_text(encoding="utf-8"))["ApiHash"], metadata["apiHash"]) + self.assertEqual(len(metadata["apiHash"]), 64) + self.assertEqual(json.loads(management_path.read_text(encoding="utf-8"))["ApiHash"], "dummy") + + def test_invalid_generated_metadata_still_fails(self): + path = self.write_package_info() + metadata_path = self.artifact_dir / "api.metadata.yml" + original = path.read_bytes() + for metadata in ({}, {"apiHash": None}, {"apiHash": ""}, {"apiHash": " "}, {"apiHash": 123}): + with self.subTest(metadata=metadata): + metadata_path.write_text(yaml.safe_dump(metadata), encoding="utf-8") + with self.assertRaisesRegex(ValueError, "apiHash was not found"): + save_package_api_hash.update_package_info(metadata_path, path) + self.assertEqual(path.read_bytes(), original) + + def test_export_failure_is_not_replaced_with_dummy(self): + path = self.write_package_info("azure-test") + package_dir = self.artifact_dir / "azure-test" + package_dir.mkdir() + (package_dir / "azure-test_python.json").write_text("{}", encoding="utf-8") + with patch.object( + save_package_api_hash.subprocess, "run", side_effect=subprocess.CalledProcessError(1, "pwsh") + ): + with self.assertRaises(subprocess.CalledProcessError): + self.run_main() + self.assertNotIn("ApiHash", json.loads(path.read_text(encoding="utf-8-sig"))) + + +if __name__ == "__main__": + unittest.main()