Update get-codeowners.ps1 to work with the updated `RetrieveCodeOwn…

…ers` executable + add some tests; make assorted refactorings (#5103) This PR is part of work required in preparation to merge: - #5088 Specifically, to enable review of ownership changes due to upcoming wildcards support, as explained in: - #5088 (comment) As such, this PR contributes to: - #2770 This PR is a prerequisite for following PRs: - #5431 ### Merging prerequisite This PR can be merged only after the following PRs are merged and relevant NuGet package published - #5437 - #5104 This is because this PR depends on that NuGet package.
Azure · Feb 16, 2023 · 0b01239 · 0b01239
1 parent b915ffd
commit 0b01239
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 52 deletions.
diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
@@ -24,7 +24,7 @@
 # Eng Sys
 ###########
 /eng/                            @azure/azure-sdk-eng
-/eng/common/                     @sima-zhu @weshaggard @benbp
+/eng/common/                     @konrad-jamrozik @weshaggard @benbp
 /eng/common/TestResources/       @benbp @weshaggard @heaths
 
 ###########

diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1
@@ -1,72 +1,151 @@
+<#
+.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.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.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 = "",
+  [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
   Push-Location $ToolPath
   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
-  }
-  else {
-    $codeOwnersString = & $command --target-directory $targetDirectory --code-owner-file-path $codeOwnerFileLocation --filter-out-non-user-aliases 2>&1
+  # Backward compaitiblity: if $targetPath is not provided, fall-back to the legacy $targetDirectory
+  if ([string]::IsNullOrWhiteSpace($targetPath)) {
+    $targetPath = $targetDirectory
   }
-  # Failed at the command of fetching code owners.
-  if ($LASTEXITCODE -ne 0) {
-    Write-Host $codeOwnersString
+  if ([string]::IsNullOrWhiteSpace($targetPath)) {
+    Write-Error "TargetPath (or TargetDirectory) parameter must be neither null nor whitespace."
     return ,@()
   }
-
-  $codeOwnersJson = $codeOwnersString | ConvertFrom-Json
-  if (!$codeOwnersJson) {
-    Write-Host "No code owners returned from the path: $targetDirectory"
+
+  $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
+
+  if ($LASTEXITCODE -ne 0) {
+    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:
+# - 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) {
-  Write-Host "Testing on $targetDirectory..."
-  $actualReturn = Get-CodeOwners -targetDirectory $targetDirectory -codeOwnerFileLocation $codeOwnerFileLocation -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)."
@@ -80,21 +159,19 @@ function TestGetCodeOwner([string]$targetDirectory, [string]$codeOwnerFileLocati
   }
 }
 
-if($Test) {
-  # These tests have been removed; now instead we should run tests from RetrieveCodeOwnersProgramTests, and in a way as explained in:
+if ($Test) {
+  # 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 @("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")
   exit 0
 }
 else {
-  return Get-CodeOwners -targetDirectory $TargetDirectory -codeOwnerFileLocation $CodeOwnerFileLocation -includeNonUserAliases $IncludeNonUserAliases
+  return Get-Codeowners -targetPath $TargetPath -targetDirectory $TargetDirectory -codeownersFileLocation $CodeOwnerFileLocation -includeNonUserAliases $IncludeNonUserAliases
 }
diff --git a/eng/pipelines/agent-pool-migration.yml b/eng/pipelines/agent-pool-migration.yml
@@ -85,7 +85,7 @@ parameters:
 - name: ShortForMigrateTo
   type: string
   default: ''
-# The value here is github alias: e.g. sima-zhu
+# The value here is github alias: e.g. konrad-jamrozik
 # Users to assign to the PR after opening. Users should be a comma-separated list. e.g. "user1,usertwo,user3"
 - name: GithubAssignessAlias
   type: string