Skip to content

Commit

Permalink
Remove the obsolete, prefix-based CODEOWNERS matcher (#5431)
Browse files Browse the repository at this point in the history
* remove obsolete tests from `get-codeowners.ps1`
remove ProgramSimplePathTests; rename ProgramGlobPathTests to RetrieveCodeOwnersProgramTests
remove the obsolete prefix-based `CODEOWNERS` matcher

* Fix path to `test_CODEOWNERS`
  • Loading branch information
Konrad Jamrozik authored Feb 23, 2023
1 parent b0b5a8b commit 0e21539
Show file tree
Hide file tree
Showing 9 changed files with 222 additions and 316 deletions.
2 changes: 1 addition & 1 deletion eng/common/scripts/get-codeowners.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ if ($Test) {
$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")
$testCodeowners = (Resolve-Path "$PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/test_CODEOWNERS")
TestGetCodeowners -targetPath "tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/a.txt" -codeownersFileLocation $testCodeowners -includeNonUserAliases $true -expectReturn @("2star")
exit 0
}
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests;
/// <summary>
/// A class containing a set of tools, implemented as unit tests,
/// allowing you to view and diff owners of files of locally cloned repositories,
/// by obtaining the owners based on specified CODEOWNERS files, and/or using specified
/// matching logic: the legacy prefix-based or the new regex-based wildcard-supporting matcher.
/// by obtaining the owners based on specified CODEOWNERS files.
///
/// These tools are to be run manually, locally, by a developer.
/// They do not participate in an automated regression test suite.
Expand Down Expand Up @@ -58,7 +57,7 @@ public class CodeownersManualAnalysisTests
/// This file is expected to be manually created by you, in your local repo clone.
/// For details of usage of this file, see:
///
/// WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv
/// WriteTwoCodeownersFilesOwnersDiffToCsv
///
/// </summary>
private const string SecondaryCodeownersFilePathSuffix = "/.github/CODEOWNERS2";
Expand All @@ -71,7 +70,7 @@ public class CodeownersManualAnalysisTests

[Test] // Runtime <1s
public void OwnersForAzureDev()
=> WriteOwnersToCsvUsingRegexMatcher(
=> WriteOwnersToCsv(
targetDirPathSuffix: "/../azure-dev",
outputFileNamePrefix: "azure-dev",
ignoredPathPrefixes: ".git|artifacts");
Expand All @@ -90,18 +89,18 @@ public void OwnersForAzureDev()

[Test] // Runtime <1s
public void OwnersForAzureSdkTools()
=> WriteOwnersToCsvUsingRegexMatcher(
=> WriteOwnersToCsv(
targetDirPathSuffix: "",
outputFileNamePrefix: "azure-sdk-tools",
ignoredPathPrefixes: ".git|artifacts");

#endregion

#region Tests - Owners diffs for the prefix-based vs regex-based CODEOWNERS matchers, plus differing contents.
#region Tests - Owners diffs for differing CODEOWNERS contents.

[Test] // Runtime <1s
public void OwnersDiffForAzureDev()
=> WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv(
=> WriteTwoCodeownersFilesOwnersDiffToCsv(
targetDirPathSuffix: "/../azure-dev",
outputFileNamePrefix: "azure-dev",
ignoredPathPrefixes: ".git|artifacts");
Expand Down Expand Up @@ -149,12 +148,12 @@ public void OwnersDiffForAzureDev()
#region Parameterized tests - Owners

private void WriteLangRepoOwnersToCsv(string langName)
=> WriteOwnersToCsvUsingRegexMatcher(
=> WriteOwnersToCsv(
targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName),
outputFileNamePrefix: $"azure-sdk-for-{langName}",
ignoredPathPrefixes: ".git|artifacts");

private void WriteOwnersToCsvUsingRegexMatcher(
private void WriteOwnersToCsv(
string targetDirPathSuffix,
string outputFileNamePrefix,
string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes)
Expand All @@ -171,7 +170,6 @@ private void WriteOwnersToCsvUsingRegexMatcher(
targetDirPathSuffix,
CodeownersFilePathSuffix,
ignoredPathPrefixes,
useRegexMatcher: true,
outputFileNamePrefix);
}

Expand All @@ -180,7 +178,7 @@ private void WriteOwnersToCsvUsingRegexMatcher(
#region Parameterized tests - Owners diff

private void WriteLangRepoOwnersDiffToCsv(string langName)
=> WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv(
=> WriteTwoCodeownersFilesOwnersDiffToCsv(
targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName),
outputFileNamePrefix: $"azure-sdk-for-{langName}",
ignoredPathPrefixes: ".git|artifacts");
Expand All @@ -192,21 +190,20 @@ private void WriteLangRepoOwnersDiffToCsv(string langName)
///
/// with following meanings bound to LEFT and RIGHT:
///
/// LEFT: RetrieveCodeowners configuration using the legacy prefix-based CODEOWNERS paths matcher,
/// and given input local repository clone CODEOWNERS file.
/// LEFT: RetrieveCodeowners configuration given input local repository clone CODEOWNERS file.
///
/// RIGHT: RetrieveCodeowners configuration using the new regex-based wildcard-supporting matcher,
/// and given input repository CODEOWNERS2 file, located beside CODEOWNERS file.
/// RIGHT: RetrieveCodeowners configuration given input repository CODEOWNERS2 file,
/// located beside CODEOWNERS file.
///
/// The CODEOWNERS2 file is expected to be created manually by you. This way you can diff CODEOWNERS
/// to whatever version of it you want to express in CODEOWNERS2. For example, CODEOWNERS2 could have
/// contents of CODEOWNERS as seen in an open PR pending being merged.
///
/// Note that modifying or reordering existing paths may always impact which PR reviewers are auto-assigned,
/// but the build failure notification recipients (owners) changes apply only to paths that represent
/// but the build failure notification recipients changes apply only to paths that represent
/// build definition .yml files.
/// </summary>
private void WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv(
private void WriteTwoCodeownersFilesOwnersDiffToCsv(
string targetDirPathSuffix,
string outputFileNamePrefix,
string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes)
Expand All @@ -215,19 +212,19 @@ private void WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv(
string targetDir = rootDir + targetDirPathSuffix;
Debug.Assert(Directory.Exists(targetDir),
$"Ensure you have cloned the repo into '{targetDir}'. " +
"See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details.");
"See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesOwnersDiffToCsv for details.");
Debug.Assert(File.Exists(targetDir + CodeownersFilePathSuffix),
$"Ensure you have cloned the repo into '{targetDir}'. " +
"See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details.");
"See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesOwnersDiffToCsv for details.");
Debug.Assert(File.Exists(targetDir + SecondaryCodeownersFilePathSuffix),
$"Ensure you have created '{Path.GetFullPath(targetDir + SecondaryCodeownersFilePathSuffix)}'. " +
$"See comment on WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details.");
$"See comment on WriteTwoCodeownersFilesOwnersDiffToCsv for details.");

WriteOwnersDiffToCsv(
new[]
{
(targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: false),
(targetDirPathSuffix, SecondaryCodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true)
(targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes),
(targetDirPathSuffix, SecondaryCodeownersFilePathSuffix, ignoredPathPrefixes)
},
outputFileNamePrefix);
}
Expand Down Expand Up @@ -296,7 +293,6 @@ private static void WriteOwnersToCsv(
string targetDirPathSuffix,
string codeownersFilePathSuffix,
string ignoredPrefixes,
bool useRegexMatcher,
string outputFilePrefix)
{
var stopwatch = Stopwatch.StartNew();
Expand All @@ -306,8 +302,7 @@ private static void WriteOwnersToCsv(
Dictionary<string, CodeownersEntry> ownersData = RetrieveCodeowners(
targetDirPathSuffix,
codeownersFilePathSuffix,
ignoredPrefixes,
useRegexMatcher);
ignoredPrefixes);

List<string> outputLines =
new List<string> { "PATH | PATH EXPRESSION | OWNERS | ISSUE" };
Expand Down Expand Up @@ -495,8 +490,7 @@ private static void WriteOwnersDiffToCsv(
(
string targetDirPathSuffix,
string codeownersFilePathSuffix,
string ignoredPrefixes,
bool useRegexMatcher
string ignoredPrefixes
)[] input,
string outputFilePrefix)
{
Expand All @@ -505,13 +499,11 @@ bool useRegexMatcher
Dictionary<string, CodeownersEntry> leftOwners = RetrieveCodeowners(
input[0].targetDirPathSuffix,
input[0].codeownersFilePathSuffix,
input[0].ignoredPrefixes,
input[0].useRegexMatcher);
input[0].ignoredPrefixes);
Dictionary<string, CodeownersEntry> rightOwners = RetrieveCodeowners(
input[1].targetDirPathSuffix,
input[1].codeownersFilePathSuffix,
input[1].ignoredPrefixes,
input[1].useRegexMatcher);
input[1].ignoredPrefixes);

string[] diffLines = PathOwnersDiff(leftOwners, rightOwners);

Expand All @@ -525,8 +517,7 @@ bool useRegexMatcher
private static Dictionary<string, CodeownersEntry> RetrieveCodeowners(
string targetDirPathSuffix,
string codeownersFilePathSuffixToRootDir,
string ignoredPathPrefixes,
bool useRegexMatcher)
string ignoredPathPrefixes)
{
string rootDir = PathNavigatingToRootDir(CurrentDir);
string targetDir = rootDir + targetDirPathSuffix;
Expand All @@ -547,8 +538,7 @@ private static Dictionary<string, CodeownersEntry> RetrieveCodeowners(
// because Contacts.GetMatchingCodeownersEntry() calls ExcludeNonUserAliases().
excludeNonUserAliases: false,
targetDir,
ignoredPathPrefixes,
useRegexMatcher);
ignoredPathPrefixes);

actualOutput = consoleOutput.GetStdout();
actualErr = consoleOutput.GetStderr();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,17 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests;

/// <summary>
/// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main(),
/// for scenario in which targetPath is a glob path, i.e.
/// targetPath.IsGlobPath() returns true.
///
/// The tests assertion expectations are set to match GitHub CODEOWNERS interpreter behavior,
/// as explained here:
/// https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners
/// and as observed based on manual tests and verification.
///
/// For additional related tests, please see:
/// - Azure.Sdk.Tools.CodeOwnersParser.Tests.CodeownersFileTests
/// </summary>
[TestFixture]
public class ProgramGlobPathTests
public class RetrieveCodeOwnersProgramTests
{
/// <summary>
/// Given:
Expand All @@ -28,7 +29,7 @@ public class ProgramGlobPathTests
///
/// targetPath of /**
///
/// excludeNonUserAliases set to false and useRegexMatcher set to true.
/// excludeNonUserAliases set to false
///
/// When:
/// The retrieve-codeowners tool is executed on these inputs.
Expand All @@ -39,13 +40,12 @@ public class ProgramGlobPathTests
///
/// </summary>
[Test]
public void OutputsCodeownersForGlobPath()
public void OutputsCodeowners()
{
const string targetDir = "./TestData/InputDir";
const string targetPath = "/**";
const string codeownersFilePathOrUrl = "./TestData/glob_path_CODEOWNERS";
const string codeownersFilePathOrUrl = "./TestData/test_CODEOWNERS";
const bool excludeNonUserAliases = false;
const bool useRegexMatcher = true;

var expectedEntries = new Dictionary<string, CodeownersEntry>
{
Expand Down Expand Up @@ -74,8 +74,7 @@ public void OutputsCodeownersForGlobPath()
targetPath,
codeownersFilePathOrUrl,
excludeNonUserAliases,
targetDir,
useRegexMatcher: useRegexMatcher);
targetDir);

actualOutput = consoleOutput.GetStdout();
actualErr = consoleOutput.GetStderr();
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ public static class Program
/// Defaults to ".git".
/// Example usage: ".git|foo|bar"
/// </param>
/// <param name="useRegexMatcher">
/// Whether to use the new matcher for CODEOWNERS file paths, that supports matching
/// entries with * and **, instead of silently ignoring them.
/// </param>
/// <returns>
/// On STDOUT: The JSON representation of the matched CodeownersEntry.
/// "new CodeownersEntry()" if no path in the CODEOWNERS data matches.
Expand All @@ -52,8 +48,7 @@ public static int Main(
string codeownersFilePathOrUrl,
bool excludeNonUserAliases = false,
string? targetDir = null,
string ignoredPathPrefixes = DefaultIgnoredPrefixes,
bool useRegexMatcher = CodeownersFile.UseRegexMatcherDefault)
string ignoredPathPrefixes = DefaultIgnoredPrefixes)
{
try
{
Expand All @@ -76,13 +71,11 @@ public static int Main(
targetDir!,
codeownersFilePathOrUrl,
excludeNonUserAliases,
SplitIgnoredPathPrefixes(),
useRegexMatcher)
SplitIgnoredPathPrefixes())
: GetCodeownersForSimplePath(
targetPath,
codeownersFilePathOrUrl,
excludeNonUserAliases,
useRegexMatcher);
excludeNonUserAliases);

string codeownersJson = JsonSerializer.Serialize(
codeownersData,
Expand All @@ -108,8 +101,7 @@ private static Dictionary<string, CodeownersEntry> GetCodeownersForGlobPath(
string targetDir,
string codeownersFilePathOrUrl,
bool excludeNonUserAliases,
string[]? ignoredPathPrefixes = null,
bool useRegexMatcher = CodeownersFile.UseRegexMatcherDefault)
string[]? ignoredPathPrefixes = null)
{
ignoredPathPrefixes ??= Array.Empty<string>();

Expand All @@ -118,8 +110,7 @@ private static Dictionary<string, CodeownersEntry> GetCodeownersForGlobPath(
targetPath,
targetDir,
codeownersFilePathOrUrl,
ignoredPathPrefixes,
useRegexMatcher);
ignoredPathPrefixes);

if (excludeNonUserAliases)
codeownersEntries.Values.ToList().ForEach(entry => entry.ExcludeNonUserAliases());
Expand All @@ -130,14 +121,12 @@ private static Dictionary<string, CodeownersEntry> GetCodeownersForGlobPath(
private static CodeownersEntry GetCodeownersForSimplePath(
string targetPath,
string codeownersFilePathOrUrl,
bool excludeNonUserAliases,
bool useRegexMatcher = CodeownersFile.UseRegexMatcherDefault)
bool excludeNonUserAliases)
{
CodeownersEntry codeownersEntry =
CodeownersFile.GetMatchingCodeownersEntry(
targetPath,
codeownersFilePathOrUrl,
useRegexMatcher);
codeownersFilePathOrUrl);

if (excludeNonUserAliases)
codeownersEntry.ExcludeNonUserAliases();
Expand Down
Loading

0 comments on commit 0e21539

Please sign in to comment.