From 5a93b8de8fa349624e8221b09288cad42490f467 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Tue, 10 Jan 2023 03:18:38 -0800 Subject: [PATCH 1/4] - improve $commandOutput processing - capitalization fixups + some naming refactoring - update ToolVersion to 1.0.0-dev.20230213.7 - fix param: --codeowners-file-path -> --codeowners-file-path-or-url; minor fixes - update Azure.Sdk.Tools.RetrieveCodeOwners package ver to 20230213.2 - document params + add TargetPath --- eng/common/scripts/get-codeowners.ps1 | 151 +++++++++++++++++++------- 1 file changed, 113 insertions(+), 38 deletions(-) diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1 index 4264d472a5..4af655574f 100644 --- a/eng/common/scripts/get-codeowners.ps1 +++ b/eng/common/scripts/get-codeowners.ps1 @@ -1,25 +1,86 @@ +<# +.SYNOPSIS +A script that given as input $TargetPath param, returns the owners +of that path, as determined by CODEOWNERS file passed in $CodeOwnersFileLocation +param. + +.PARAMETER TargetPath +Required*. Path to file or directory whose owners are to be determined from a +CODEOWNERS file. e.g. sdk/core/azure-amqp/ or sdk/core/foo.txt. + +*for backward compatibility, you might provide $TargetDirectory instead. + +.PARAMETER TargetDirectory +Obsolete. Replaced by $TargetPath. Kept for backward-compatibility. +If both $TargetPath and $TargetDirectory are provided, $TargetDirectory is +ignored. + +.PARAMETER CodeOwnerFileLocation +Optional. An absolute path to the CODEOWNERS file against which the $TargetPath param +will be checked to determine its owners. + +.PARAMETER ToolVersion +Optional. The NuGet package version of the package containing the "retrieve-codeowners" +tool, around which this script is a wrapper. + +.PARAMETER ToolPath +Optional. The place to check the "retrieve-codeowners" tool existence. + +.PARAMETER DevOpsFeed +Optional. The NuGet package feed from which the "retrieve-codeowners" tool is to be installed. + +NuGet feed: +https://dev.azure.com/azure-sdk/public/_artifacts/feed/azure-sdk-for-net/NuGet/Azure.Sdk.Tools.RetrieveCodeOwners + +Pipeline publishing the NuGet package to the feed, "tools - code-owners-parser": +https://dev.azure.com/azure-sdk/internal/_build?definitionId=3188 + +.PARAMETER VsoVariable +Optional. If provided, the determined owners, based on $TargetPath matched against CODEOWNERS file at $CodeOwnerFileLocation, +will be output to Azure DevOps pipeline log as variable named $VsoVariable. + +Reference: +https://learn.microsoft.com/en-us/azure/devops/pipelines/process/variables?view=azure-devops&tabs=yaml%2Cbatch +https://learn.microsoft.com/en-us/azure/devops/pipelines/scripts/logging-commands?view=azure-devops&tabs=bash#logging-command-format + +.PARAMETER IncludeNonUserAliases +Optional. Whether to include in the returned owners list aliases that are team aliases, e.g. Azure/azure-sdk-team + +.PARAMETER Test +Optional. Whether to run the script against hard-coded tests. + +#> param ( - [string]$TargetDirectory = "", # Code path to code owners. e.g sdk/core/azure-amqp - [string]$CodeOwnerFileLocation = (Resolve-Path $PSScriptRoot/../../../.github/CODEOWNERS), # The absolute path of CODEOWNERS file. - [string]$ToolVersion = "1.0.0-dev.20230108.6", - [string]$ToolPath = (Join-Path ([System.IO.Path]::GetTempPath()) "codeowners-tool-path"), # The place to check the tool existence. Put temp path as default - [string]$DevOpsFeed = "https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json", # DevOp tool feeds. - [string]$VsoVariable = "", # Option of write code owners into devop variable - [switch]$IncludeNonUserAliases, # Option to filter out the team alias in code owner list. e.g. Azure/azure-sdk-team - [switch]$Test #Run test functions against the script logic + [string]$TargetPath = "", + [string]$TargetDirectory = "", + # The path used assumes the script is located in azure-sdk-tools/eng/common/scripts/get-codeowners.ps1 + [string]$CodeOwnerFileLocation = (Resolve-Path $PSScriptRoot/../../../.github/CODEOWNERS), + # The $ToolVersion 1.0.0-dev.20230213.7 includes following PR: + # Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher + # https://github.com/Azure/azure-sdk-tools/pull/5088 + # + # but not this one: + # Remove the obsolete, prefix-based CODEOWNERS matcher & related tests + # https://github.com/Azure/azure-sdk-tools/pull/5431 + [string]$ToolVersion = "1.0.0-dev.20230213.7", + [string]$ToolPath = (Join-Path ([System.IO.Path]::GetTempPath()) "codeowners-tool-path"), + [string]$DevOpsFeed = "https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json", + [string]$VsoVariable = "", + [switch]$IncludeNonUserAliases, + [switch]$Test ) -function Get-CodeOwnersTool() +function Get-CodeownersTool() { - $command = Join-Path $ToolPath "retrieve-codeowners" - # Check if the retrieve-codeowners tool exsits or not. - if (Get-Command $command -errorAction SilentlyContinue) { - return $command + $codeownersToolCommand = Join-Path $ToolPath "retrieve-codeowners" + # Check if the retrieve-codeowners tool exists or not. + if (Get-Command $codeownersToolCommand -errorAction SilentlyContinue) { + return $codeownersToolCommand } if (!(Test-Path $ToolPath)) { New-Item -ItemType Directory -Path $ToolPath | Out-Null } - Write-Host "Installing the retrieve-codeowners tool under $ToolPath... " + Write-Host "Installing the retrieve-codeowners tool under tool path: $ToolPath ..." # Run command under tool path to avoid dotnet tool install command checking .csproj files. # This is a bug for dotnet tool command. Issue: https://github.com/dotnet/sdk/issues/9623 @@ -27,46 +88,60 @@ function Get-CodeOwnersTool() dotnet tool install --tool-path $ToolPath --add-source $DevOpsFeed --version $ToolVersion "Azure.Sdk.Tools.RetrieveCodeOwners" | Out-Null Pop-Location # Test to see if the tool properly installed. - if (!(Get-Command $command -errorAction SilentlyContinue)) { - Write-Error "The retrieve-codeowners tool is not properly installed. Please check your tool path. $ToolPath" + if (!(Get-Command $codeownersToolCommand -errorAction SilentlyContinue)) { + Write-Error "The retrieve-codeowners tool is not properly installed. Please check your tool path: $ToolPath" return } - return $command + return $codeownersToolCommand } -function Get-CodeOwners ([string]$targetDirectory, [string]$codeOwnerFileLocation, [bool]$includeNonUserAliases = $false) +function Get-Codeowners( + [string]$targetPath, + [string]$targetDirectory, + [string]$codeownersFileLocation, + [bool]$includeNonUserAliases = $false) { - $command = Get-CodeOwnersTool - # Filter out the non user alias from code owner list. - if($includeNonUserAliases) { - $codeOwnersString = & $command --target-directory $targetDirectory --code-owner-file-path $codeOwnerFileLocation 2>&1 + # Backward compaitiblity: if $targetPath is not provided, fall-back to the legacy $targetDirectory + if ([string]::IsNullOrWhiteSpace($targetPath)) { + $targetPath = $targetDirectory } - else { - $codeOwnersString = & $command --target-directory $targetDirectory --code-owner-file-path $codeOwnerFileLocation --filter-out-non-user-aliases 2>&1 + if ([string]::IsNullOrWhiteSpace($targetPath)) { + Write-Error "TargetPath (or TargetDirectory) parameter must be neither null nor whitespace." + return ,@() } + + $codeownersToolCommand = Get-CodeownersTool + $commandOutput = & $codeownersToolCommand ` + --target-path $targetPath ` + --codeowners-file-path-or-url $codeownersFileLocation ` + --exclude-non-user-aliases:$(!$includeNonUserAliases) ` + 2>&1 + # Failed at the command of fetching code owners. if ($LASTEXITCODE -ne 0) { - Write-Host $codeOwnersString - return ,@() - } - - $codeOwnersJson = $codeOwnersString | ConvertFrom-Json - if (!$codeOwnersJson) { - Write-Host "No code owners returned from the path: $targetDirectory" + Write-Host $commandOutput return ,@() } + +# Assert: $commandOutput is a valid JSON representing: +# - a single CodeownersEntry, if the $targetPath was a single path +# - or a dictionary of CodeownerEntries, keyes by each path resolved from a $targetPath glob path. +# +# For implementation details, see Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main + +$codeownersJson = $commandOutput | ConvertFrom-Json if ($VsoVariable) { - $codeOwners = $codeOwnersJson.Owners -join "," - Write-Host "##vso[task.setvariable variable=$VsoVariable;]$codeOwners" + $codeowners = $codeownersJson.Owners -join "," + Write-Host "##vso[task.setvariable variable=$VsoVariable;]$codeowners" } - return ,@($codeOwnersJson.Owners) + return ,@($codeownersJson.Owners) } -function TestGetCodeOwner([string]$targetDirectory, [string]$codeOwnerFileLocation, [bool]$includeNonUserAliases = $false, [string[]]$expectReturn) { +function TestGetCodeowners([string]$targetDirectory, [string]$codeownersFileLocation, [bool]$includeNonUserAliases = $false, [string[]]$expectReturn) { Write-Host "Testing on $targetDirectory..." - $actualReturn = Get-CodeOwners -targetDirectory $targetDirectory -codeOwnerFileLocation $codeOwnerFileLocation -includeNonUserAliases $IncludeNonUserAliases + $actualReturn = Get-Codeowners -targetDirectory $targetDirectory -codeownersFileLocation $codeownersFileLocation -includeNonUserAliases $IncludeNonUserAliases if ($actualReturn.Count -ne $expectReturn.Count) { Write-Error "The length of actual result is not as expected. Expected length: $($expectReturn.Count), Actual length: $($actualReturn.Count)." @@ -80,7 +155,7 @@ function TestGetCodeOwner([string]$targetDirectory, [string]$codeOwnerFileLocati } } -if($Test) { +if ($Test) { # These tests have been removed; now instead we should run tests from RetrieveCodeOwnersProgramTests, and in a way as explained in: # https://github.com/Azure/azure-sdk-tools/issues/5434 # https://github.com/Azure/azure-sdk-tools/pull/5103#discussion_r1068680818 @@ -96,5 +171,5 @@ if($Test) { exit 0 } else { - return Get-CodeOwners -targetDirectory $TargetDirectory -codeOwnerFileLocation $CodeOwnerFileLocation -includeNonUserAliases $IncludeNonUserAliases + return Get-Codeowners -targetPath $TargetPath -targetDirectory $TargetDirectory -codeownersFileLocation $CodeOwnerFileLocation -includeNonUserAliases $IncludeNonUserAliases } From 2549f46937e97fcc3799f45f50a50e39a8350729 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Tue, 14 Feb 2023 16:00:50 -0800 Subject: [PATCH 2/4] update ToolVersion to 1.0.0-dev.20230214.3 --- eng/common/scripts/get-codeowners.ps1 | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1 index 4af655574f..6c4882a825 100644 --- a/eng/common/scripts/get-codeowners.ps1 +++ b/eng/common/scripts/get-codeowners.ps1 @@ -55,14 +55,14 @@ param ( [string]$TargetDirectory = "", # The path used assumes the script is located in azure-sdk-tools/eng/common/scripts/get-codeowners.ps1 [string]$CodeOwnerFileLocation = (Resolve-Path $PSScriptRoot/../../../.github/CODEOWNERS), - # The $ToolVersion 1.0.0-dev.20230213.7 includes following PR: - # Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher - # https://github.com/Azure/azure-sdk-tools/pull/5088 + # The $ToolVersion 1.0.0-dev.20230214.3 includes following PR: + # Use CodeownersFile.UseRegexMatcherDefault everywhere where applicable + remove obsolete tests + # https://github.com/Azure/azure-sdk-tools/pull/5437 # # but not this one: # Remove the obsolete, prefix-based CODEOWNERS matcher & related tests # https://github.com/Azure/azure-sdk-tools/pull/5431 - [string]$ToolVersion = "1.0.0-dev.20230213.7", + [string]$ToolVersion = "1.0.0-dev.20230214.3", [string]$ToolPath = (Join-Path ([System.IO.Path]::GetTempPath()) "codeowners-tool-path"), [string]$DevOpsFeed = "https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-net/nuget/v3/index.json", [string]$VsoVariable = "", From b766b01991ace3521e3d4665f44ad1212ceaffa6 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Tue, 14 Feb 2023 16:35:45 -0800 Subject: [PATCH 3/4] Add diag output + tests --- eng/common/scripts/get-codeowners.ps1 | 32 ++++++++++++++------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1 index 6c4882a825..11a5625609 100644 --- a/eng/common/scripts/get-codeowners.ps1 +++ b/eng/common/scripts/get-codeowners.ps1 @@ -111,16 +111,19 @@ function Get-Codeowners( } $codeownersToolCommand = Get-CodeownersTool + Write-Host "Executing: & $codeownersToolCommand --target-path $targetPath --codeowners-file-path-or-url $codeownersFileLocation --exclude-non-user-aliases:$(!$includeNonUserAliases)" $commandOutput = & $codeownersToolCommand ` --target-path $targetPath ` --codeowners-file-path-or-url $codeownersFileLocation ` --exclude-non-user-aliases:$(!$includeNonUserAliases) ` 2>&1 - # Failed at the command of fetching code owners. if ($LASTEXITCODE -ne 0) { - Write-Host $commandOutput + Write-Host "Command $codeownersToolCommand execution failed (exit code = $LASTEXITCODE). Output string: $commandOutput" return ,@() + } else + { + Write-Host "Command $codeownersToolCommand executed successfully (exit code = 0). Output string length: $($commandOutput.length)" } # Assert: $commandOutput is a valid JSON representing: @@ -139,9 +142,10 @@ $codeownersJson = $commandOutput | ConvertFrom-Json return ,@($codeownersJson.Owners) } -function TestGetCodeowners([string]$targetDirectory, [string]$codeownersFileLocation, [bool]$includeNonUserAliases = $false, [string[]]$expectReturn) { - Write-Host "Testing on $targetDirectory..." - $actualReturn = Get-Codeowners -targetDirectory $targetDirectory -codeownersFileLocation $codeownersFileLocation -includeNonUserAliases $IncludeNonUserAliases +function TestGetCodeowners([string]$targetPath, [string]$codeownersFileLocation, [bool]$includeNonUserAliases = $false, [string[]]$expectReturn) { + Write-Host "Test: find owners matching '$targetPath' ..." + + $actualReturn = Get-Codeowners -targetPath $targetPath -codeownersFileLocation $codeownersFileLocation -includeNonUserAliases $IncludeNonUserAliases if ($actualReturn.Count -ne $expectReturn.Count) { Write-Error "The length of actual result is not as expected. Expected length: $($expectReturn.Count), Actual length: $($actualReturn.Count)." @@ -156,18 +160,16 @@ function TestGetCodeowners([string]$targetDirectory, [string]$codeownersFileLoca } if ($Test) { - # These tests have been removed; now instead we should run tests from RetrieveCodeOwnersProgramTests, and in a way as explained in: + # Most of tests here have been removed; now instead we should run tests from RetrieveCodeOwnersProgramTests, and in a way as explained in: # https://github.com/Azure/azure-sdk-tools/issues/5434 # https://github.com/Azure/azure-sdk-tools/pull/5103#discussion_r1068680818 - Write-Host "These tests have been removed. Please see https://github.com/Azure/azure-sdk-tools/issues/5434 for more." - # - # $testFile = (Resolve-Path $PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS) - # TestGetCodeOwner -targetDirectory "sdk" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person1", "person2") - # TestGetCodeOwner -targetDirectory "sdk/noPath" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person1", "person2") - # TestGetCodeOwner -targetDirectory "/sdk/azconfig" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person3", "person4") - # TestGetCodeOwner -targetDirectory "/sdk/azconfig/package" -codeOwnerFileLocation $testFile -includeNonUserAliases $true $testFile -expectReturn @("person3", "person4") - # TestGetCodeOwner -targetDirectory "/sd" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @() - # TestGetCodeOwner -targetDirectory "/sdk/testUser/" -codeOwnerFileLocation $testFile -expectReturn @("azure-sdk") + Write-Host "Running reduced test suite at `$PSScriptRoot of $PSSCriptRoot. Please see https://github.com/Azure/azure-sdk-tools/issues/5434 for more." + + $azSdkToolsCodeowners = (Resolve-Path "$PSScriptRoot/../../../.github/CODEOWNERS") + TestGetCodeowners -targetPath "eng/common/scripts/get-codeowners.ps1" -codeownersFileLocation $azSdkToolsCodeowners -includeNonUserAliases $true -expectReturn @("sima-zhu", "weshaggard", "benbp") + + $testCodeowners = (Resolve-Path "$PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS") + TestGetCodeowners -targetPath "tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/a.txt" -codeownersFileLocation $testCodeowners -includeNonUserAliases $true -expectReturn @("2star") exit 0 } else { From e132b0ef5a713c5e6364ad3d731c955640ee2277 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Tue, 14 Feb 2023 18:18:31 -0800 Subject: [PATCH 4/4] replace sima-zhu with konrad-jamrozik --- eng/common/scripts/get-codeowners.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1 index 11a5625609..5c2bb9b50a 100644 --- a/eng/common/scripts/get-codeowners.ps1 +++ b/eng/common/scripts/get-codeowners.ps1 @@ -166,7 +166,7 @@ if ($Test) { Write-Host "Running reduced test suite at `$PSScriptRoot of $PSSCriptRoot. Please see https://github.com/Azure/azure-sdk-tools/issues/5434 for more." $azSdkToolsCodeowners = (Resolve-Path "$PSScriptRoot/../../../.github/CODEOWNERS") - TestGetCodeowners -targetPath "eng/common/scripts/get-codeowners.ps1" -codeownersFileLocation $azSdkToolsCodeowners -includeNonUserAliases $true -expectReturn @("sima-zhu", "weshaggard", "benbp") + TestGetCodeowners -targetPath "eng/common/scripts/get-codeowners.ps1" -codeownersFileLocation $azSdkToolsCodeowners -includeNonUserAliases $true -expectReturn @("konrad-jamrozik", "weshaggard", "benbp") $testCodeowners = (Resolve-Path "$PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS") TestGetCodeowners -targetPath "tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/a.txt" -codeownersFileLocation $testCodeowners -includeNonUserAliases $true -expectReturn @("2star")