From 0e21539c301a235f4da58e397e1cb790872c153e Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Thu, 23 Feb 2023 13:57:02 -0800 Subject: [PATCH] Remove the obsolete, prefix-based `CODEOWNERS` matcher (#5431) * 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` --- eng/common/scripts/get-codeowners.ps1 | 2 +- .../CodeownersFileTests.cs | 350 +++++++++--------- .../CodeownersManualAnalysisTests.cs | 60 ++- ...s.cs => RetrieveCodeOwnersProgramTests.cs} | 17 +- .../TestData/simple_path_CODEOWNERS | 33 -- .../{glob_path_CODEOWNERS => test_CODEOWNERS} | 0 .../Program.cs | 25 +- .../CodeOwnersParser/CodeownersFile.cs | 43 +-- .../MatchedCodeownersEntry.cs | 8 +- 9 files changed, 222 insertions(+), 316 deletions(-) rename tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/{ProgramGlobPathTests.cs => RetrieveCodeOwnersProgramTests.cs} (90%) delete mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS rename tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/{glob_path_CODEOWNERS => test_CODEOWNERS} (100%) diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1 index 5c2bb9b50ab..afe993d50ba 100644 --- a/eng/common/scripts/get-codeowners.ps1 +++ b/eng/common/scripts/get-codeowners.ps1 @@ -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 } diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs index dbfba22914f..e13bbc776b9 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs @@ -3,191 +3,190 @@ namespace Azure.Sdk.Tools.CodeOwnersParser.Tests; +/// +/// Please see the comment on: +/// - Azure.Sdk.Tools.CodeOwnersParser.Tests.CodeownersFileTests.testCases +/// [TestFixture] public class CodeownersFileTests { /// - /// A battery of test cases specifying behavior of new logic matching target - /// path to CODEOWNERS entries, and comparing it to existing, legacy logic. - /// - /// The logic that has changed is located in CodeownersFile.GetMatchingCodeownersEntry. - /// - /// In the test case table below, any discrepancy between legacy and new - /// matcher expected matches that doesn't pertain to wildcard matching denotes - /// a potential backward compatibility and/or existing defect in the legacy matcher. + /// A battery of test cases specifying the behavior of logic matching target + /// path to CODEOWNERS paths. /// /// For further details, please see: /// - Class comment for Azure.Sdk.Tools.CodeOwnersParser.MatchedCodeOwnerEntry + /// - Azure.Sdk.Tools.RetrieveCodeOwners.Tests.RetrieveCodeOwnersProgramTests /// - https://github.com/Azure/azure-sdk-tools/issues/2770 /// - https://github.com/Azure/azure-sdk-tools/issues/4859 /// private static readonly TestCase[] testCases = { // @formatter:off - // Path: Expected match: - // Codeowners , Target , Legacy , New - new( "/**" , "a" , false , true ), - new( "/**" , "A" , false , true ), - new( "/**" , "/a" , false , true ), - new( "/**" , "a/" , false , true ), - new( "/**" , "/a/" , false , true ), - new( "/**" , "/a/b" , false , true ), - new( "/**" , "/a/b/" , false , true ), - new( "/**" , "/a/b/c" , false , true ), - new( "/**" , "[" , false , true ), - new( "/**" , "]" , false , true ), - new( "/" , "a" , true , false ), - new( "/" , "A" , true , false ), - new( "/" , "/a" , true , false ), - new( "/" , "a/" , true , false ), - new( "/" , "/a/" , true , false ), - new( "/" , "/a/b" , true , false ), - new( "/a" , "a" , true , true ), - new( "/a" , "A" , true , false ), - new( "/a" , "/a" , true , true ), - new( "/a" , "a/" , true , false ), - new( "/a" , "/a/" , true , false ), - new( "/a" , "/a/b" , true , false ), - new( "/a" , "/a/b/" , true , false ), - new( "/a" , "/a\\ b" , true , false ), - new( "/a" , "/x/a/b" , false , false ), - new( "a" , "a" , true , false ), - new( "a" , "ab" , true , false ), - new( "a" , "ab/" , true , false ), - new( "a" , "/ab/" , true , false ), - new( "a" , "A" , true , false ), - new( "a" , "/a" , true , false ), - new( "a" , "a/" , true , false ), - new( "a" , "/a/" , true , false ), - new( "a" , "/a/b" , true , false ), - new( "a" , "/a/b/" , true , false ), - new( "a" , "/x/a/b" , false , false ), - new( "/a/" , "a" , true , false ), - new( "/a/" , "/a" , true , false ), - new( "/a/" , "a/" , true , true ), - new( "/a/" , "/a/" , true , true ), - new( "/a/" , "/a/b" , true , true ), - new( "/a/" , "/a\\ b" , true , false ), - new( "/a/" , "/a\\ b/" , true , false ), - new( "/a/" , "/a/a\\ b/" , true , true ), - new( "/a/" , "/a/b/" , true , true ), - new( "/a/" , "/A/b/" , true , false ), - new( "/a/" , "/x/a/b" , false , false ), - new( "/a/b/" , "/a" , false , false ), - new( "/a/b/" , "/a/" , false , false ), - new( "/a/b/" , "/a/b" , true , false ), - new( "/a/b/" , "/a/b/" , true , true ), - new( "/a/b/" , "/a/b/c" , true , true ), - new( "/a/b/" , "/a/b/c/" , true , true ), - new( "/a/b/" , "/a/b/c/d" , true , true ), - new( "/a/b" , "/a" , false , false ), - new( "/a/b" , "/a/" , false , false ), - new( "/a/b" , "/a/b" , true , true ), - new( "/a/b" , "/a/b/" , true , false ), - new( "/a/b" , "/a/bc" , true , false ), - new( "/a/b" , "/a/bc/" , true , false ), - new( "/a/b" , "/a/b/c" , true , false ), - new( "/a/b" , "/a/b/c/" , true , false ), - new( "/a/b" , "/a/b/c/d" , true , false ), - new( "/!a" , "!a" , true , false ), - new( "/!a" , "b" , false , false ), - new( "/a[b" , "a[b" , true , false ), - new( "/a]b" , "a]b" , true , false ), - new( "/a?b" , "a?b" , true , false ), - new( "/a?b" , "axb" , false , false ), - new( "/a" , "*" , false , false ), - new( "/*" , "*" , true , false ), - new( "/*" , "a" , false , true ), - new( "/*" , "a/" , false , false ), - new( "/*" , "/a" , false , true ), - new( "/*" , "/a/" , false , false ), - new( "/*" , "a/b" , false , false ), - new( "/*" , "/a/b" , false , false ), - new( "/*" , "[" , false , true ), - new( "/*" , "]" , false , true ), - new( "/*" , "!" , false , true ), - new( "/**" , "!" , false , true ), - new( "/a*" , "a" , false , true ), - new( "/a*" , "a/x" , false , true ), - new( "/a*" , "a/x/d" , false , true ), - new( "/a*" , "ab" , false , true ), - new( "/a*" , "ab/x" , false , true ), - new( "/a*" , "ab/x/d" , false , true ), - new( "/a/**" , "a" , false , false ), - new( "/*/**" , "a" , false , false ), - new( "/*/**" , "a/" , false , false ), - new( "/*/**" , "a/b" , false , false ), - new( "/*/" , "a" , false , false ), - new( "/*/" , "a/" , false , true ), - new( "/*/b" , "a/b" , false , true ), - new( "/**/a" , "a" , false , true ), - new( "/**/a" , "x/ba" , false , false ), - new( "/a/*" , "a" , false , false ), - new( "/a/*" , "a/" , false , true ), - new( "/a/*" , "a/b" , false , true ), - new( "/a/*" , "a/b/" , false , false ), - new( "/a/*" , "a/b/c" , false , false ), - new( "/a/*/" , "a" , false , false ), - new( "/a/*/" , "a/" , false , false ), - new( "/a/*/" , "a/b" , false , false ), - new( "/a/*/" , "a/b/" , false , true ), - new( "/a/*/" , "a/b/c" , false , true ), - new( "/a/**" , "a" , false , false ), - new( "/a/**" , "a/" , false , false ), - new( "/a/**" , "a/b" , false , false ), - new( "/a/**" , "a/b/" , false , false ), - new( "/a/**" , "a/b/c" , false , false ), - new( "/a/**/" , "a" , false , false ), - new( "/a/**/" , "a/" , false , false ), - new( "/a/**/" , "a/b" , false , false ), - new( "/a/**/" , "a/b/" , false , false ), - new( "/a/**/" , "a/b/c" , false , false ), - new( "/**/a/" , "a" , false , false ), - new( "/**/a/" , "a/" , false , true ), - new( "/**/a/" , "a/b" , false , true ), - new( "/**/b/" , "a/b" , false , false ), - new( "/**/b/" , "a/b/" , false , true ), - new( "/**/b/" , "a/c/" , false , false ), - new( "/a/*/b/" , "a/b/" , false , false ), - new( "/a/*/b/" , "a/x/b/" , false , true ), - new( "/a/*/b/" , "a/x/b/c" , false , true ), - new( "/a/*/b/" , "a/x/c" , false , false ), - new( "/a/*/b/" , "a/x/y/b" , false , false ), - new( "/a**b/" , "a/x/y/b" , false , false ), - new( "/a/**/b/" , "a/b" , false , false ), - new( "/a/**/b/" , "a/b/" , false , true ), - new( "/a/**/b/" , "a/x/b/" , false , true ), - new( "/a/**/b/" , "a/x/y/b/" , false , true ), - new( "/a/**/b/" , "a/x/y/c" , false , false ), - new( "/a/**/b/" , "a-b/" , false , false ), - new( "a/*/*" , "a/b" , false , false ), - new( "/a/*/*/d" , "a/b/c/d" , false , true ), - new( "/a/*/*/d" , "a/b/x/c/d" , false , false ), - new( "/a/**/*/d" , "a/b/x/c/d" , false , true ), - new( "*/*/b" , "a/b" , false , false ), - new( "/a*/" , "abc/" , false , true ), - new( "/a*/" , "ab/c/" , false , true ), - new( "/*b*/" , "axbyc/" , false , true ), - new( "/*c/" , "abc/" , false , true ), - new( "/*c/" , "a/abc/" , false , false ), - new( "/a*c/" , "axbyc/" , false , true ), - new( "/a*c/" , "axb/yc/" , false , false ), - new( "/**/*x*/" , "a/b/cxy/d" , false , true ), - new( "/a/*.md" , "a/x.md" , false , true ), - new( "/*/*/*.md" , "a/b/x.md" , false , true ), - new( "/**/*.md" , "a/b.md/x.md" , false , true ), - new( "**/*.md" , "a/b.md/x.md" , false , false ), - new( "/*.md" , "a/md" , false , false ), - new( "/a.*" , "a.b" , false , true ), - new( "/a.*" , "a.b/" , false , true ), - new( "/a.*" , "x/a.b/" , false , false ), - new( "/a.*/" , "a.b" , false , false ), - new( "/a.*/" , "a.b/" , false , true ), - new( "/**/*x*/AB/*/CD" , "a/b/cxy/AB/fff/CD" , false, true ), - new( "/**/*x*/AB/*/CD" , "a/b/cxy/AB/ff/ff/CD" , false, false ), - new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/ff/ff/CD" , false, false ), - new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/ff/ff/CD/" , false, true ), - new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/[]/!!/CD/h" , false, true ), + // Path: Expected match: + // Codeowners , Target , | + new( "/**" , "a" , true ), + new( "/**" , "A" , true ), + new( "/**" , "/a" , true ), + new( "/**" , "a/" , true ), + new( "/**" , "/a/" , true ), + new( "/**" , "/a/b" , true ), + new( "/**" , "/a/b/" , true ), + new( "/**" , "/a/b/c" , true ), + new( "/**" , "[" , true ), + new( "/**" , "]" , true ), + new( "/" , "a" , false ), + new( "/" , "A" , false ), + new( "/" , "/a" , false ), + new( "/" , "a/" , false ), + new( "/" , "/a/" , false ), + new( "/" , "/a/b" , false ), + new( "/a" , "a" , true ), + new( "/a" , "A" , false ), + new( "/a" , "/a" , true ), + new( "/a" , "a/" , false ), + new( "/a" , "/a/" , false ), + new( "/a" , "/a/b" , false ), + new( "/a" , "/a/b/" , false ), + new( "/a" , "/a\\ b" , false ), + new( "/a" , "/x/a/b" , false ), + new( "a" , "a" , false ), + new( "a" , "ab" , false ), + new( "a" , "ab/" , false ), + new( "a" , "/ab/" , false ), + new( "a" , "A" , false ), + new( "a" , "/a" , false ), + new( "a" , "a/" , false ), + new( "a" , "/a/" , false ), + new( "a" , "/a/b" , false ), + new( "a" , "/a/b/" , false ), + new( "a" , "/x/a/b" , false ), + new( "/a/" , "a" , false ), + new( "/a/" , "/a" , false ), + new( "/a/" , "a/" , true ), + new( "/a/" , "/a/" , true ), + new( "/a/" , "/a/b" , true ), + new( "/a/" , "/a\\ b" , false ), + new( "/a/" , "/a\\ b/" , false ), + new( "/a/" , "/a/a\\ b/" , true ), + new( "/a/" , "/a/b/" , true ), + new( "/a/" , "/A/b/" , false ), + new( "/a/" , "/x/a/b" , false ), + new( "/a/b/" , "/a" , false ), + new( "/a/b/" , "/a/" , false ), + new( "/a/b/" , "/a/b" , false ), + new( "/a/b/" , "/a/b/" , true ), + new( "/a/b/" , "/a/b/c" , true ), + new( "/a/b/" , "/a/b/c/" , true ), + new( "/a/b/" , "/a/b/c/d" , true ), + new( "/a/b" , "/a" , false ), + new( "/a/b" , "/a/" , false ), + new( "/a/b" , "/a/b" , true ), + new( "/a/b" , "/a/b/" , false ), + new( "/a/b" , "/a/bc" , false ), + new( "/a/b" , "/a/bc/" , false ), + new( "/a/b" , "/a/b/c" , false ), + new( "/a/b" , "/a/b/c/" , false ), + new( "/a/b" , "/a/b/c/d" , false ), + new( "/!a" , "!a" , false ), + new( "/!a" , "b" , false ), + new( "/a[b" , "a[b" , false ), + new( "/a]b" , "a]b" , false ), + new( "/a?b" , "a?b" , false ), + new( "/a?b" , "axb" , false ), + new( "/a" , "*" , false ), + new( "/*" , "*" , false ), + new( "/*" , "a" , true ), + new( "/*" , "a/" , false ), + new( "/*" , "/a" , true ), + new( "/*" , "/a/" , false ), + new( "/*" , "a/b" , false ), + new( "/*" , "/a/b" , false ), + new( "/*" , "[" , true ), + new( "/*" , "]" , true ), + new( "/*" , "!" , true ), + new( "/**" , "!" , true ), + new( "/a*" , "a" , true ), + new( "/a*" , "a/x" , true ), + new( "/a*" , "a/x/d" , true ), + new( "/a*" , "ab" , true ), + new( "/a*" , "ab/x" , true ), + new( "/a*" , "ab/x/d" , true ), + new( "/a/**" , "a" , false ), + new( "/*/**" , "a" , false ), + new( "/*/**" , "a/" , false ), + new( "/*/**" , "a/b" , false ), + new( "/*/" , "a" , false ), + new( "/*/" , "a/" , true ), + new( "/*/b" , "a/b" , true ), + new( "/**/a" , "a" , true ), + new( "/**/a" , "x/ba" , false ), + new( "/a/*" , "a" , false ), + new( "/a/*" , "a/" , true ), + new( "/a/*" , "a/b" , true ), + new( "/a/*" , "a/b/" , false ), + new( "/a/*" , "a/b/c" , false ), + new( "/a/*/" , "a" , false ), + new( "/a/*/" , "a/" , false ), + new( "/a/*/" , "a/b" , false ), + new( "/a/*/" , "a/b/" , true ), + new( "/a/*/" , "a/b/c" , true ), + new( "/a/**" , "a" , false ), + new( "/a/**" , "a/" , false ), + new( "/a/**" , "a/b" , false ), + new( "/a/**" , "a/b/" , false ), + new( "/a/**" , "a/b/c" , false ), + new( "/a/**/" , "a" , false ), + new( "/a/**/" , "a/" , false ), + new( "/a/**/" , "a/b" , false ), + new( "/a/**/" , "a/b/" , false ), + new( "/a/**/" , "a/b/c" , false ), + new( "/**/a/" , "a" , false ), + new( "/**/a/" , "a/" , true ), + new( "/**/a/" , "a/b" , true ), + new( "/**/b/" , "a/b" , false ), + new( "/**/b/" , "a/b/" , true ), + new( "/**/b/" , "a/c/" , false ), + new( "/a/*/b/" , "a/b/" , false ), + new( "/a/*/b/" , "a/x/b/" , true ), + new( "/a/*/b/" , "a/x/b/c" , true ), + new( "/a/*/b/" , "a/x/c" , false ), + new( "/a/*/b/" , "a/x/y/b" , false ), + new( "/a**b/" , "a/x/y/b" , false ), + new( "/a/**/b/" , "a/b" , false ), + new( "/a/**/b/" , "a/b/" , true ), + new( "/a/**/b/" , "a/x/b/" , true ), + new( "/a/**/b/" , "a/x/y/b/" , true ), + new( "/a/**/b/" , "a/x/y/c" , false ), + new( "/a/**/b/" , "a-b/" , false ), + new( "a/*/*" , "a/b" , false ), + new( "/a/*/*/d" , "a/b/c/d" , true ), + new( "/a/*/*/d" , "a/b/x/c/d" , false ), + new( "/a/**/*/d" , "a/b/x/c/d" , true ), + new( "*/*/b" , "a/b" , false ), + new( "/a*/" , "abc/" , true ), + new( "/a*/" , "ab/c/" , true ), + new( "/*b*/" , "axbyc/" , true ), + new( "/*c/" , "abc/" , true ), + new( "/*c/" , "a/abc/" , false ), + new( "/a*c/" , "axbyc/" , true ), + new( "/a*c/" , "axb/yc/" , false ), + new( "/**/*x*/" , "a/b/cxy/d" , true ), + new( "/a/*.md" , "a/x.md" , true ), + new( "/*/*/*.md" , "a/b/x.md" , true ), + new( "/**/*.md" , "a/b.md/x.md" , true ), + new( "**/*.md" , "a/b.md/x.md" , false ), + new( "/*.md" , "a/md" , false ), + new( "/a.*" , "a.b" , true ), + new( "/a.*" , "a.b/" , true ), + new( "/a.*" , "x/a.b/" , false ), + new( "/a.*/" , "a.b" , false ), + new( "/a.*/" , "a.b/" , true ), + new( "/**/*x*/AB/*/CD" , "a/b/cxy/AB/fff/CD" , true ), + new( "/**/*x*/AB/*/CD" , "a/b/cxy/AB/ff/ff/CD" , false ), + new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/ff/ff/CD" , false ), + new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/ff/ff/CD/" , true ), + new( "/**/*x*/AB/**/CD/*" , "a/b/cxy/AB/[]/!!/CD/h" , true ), // @formatter:on }; @@ -202,20 +201,18 @@ public void TestGetMatchingCodeownersEntry(TestCase testCase) List codeownersEntries = CodeownersFile.GetCodeownersEntries(testCase.CodeownersPath + "@owner"); - VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: false, testCase.ExpectedLegacyMatch); - VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: true, testCase.ExpectedNewMatch); + VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, testCase.ExpectedNewMatch); } private static void VerifyGetMatchingCodeownersEntry( TestCase testCase, List codeownersEntries, - bool useRegexMatcher, bool expectedMatch) { CodeownersEntry entry = // Act CodeownersFile.GetMatchingCodeownersEntry(testCase.TargetPath, - codeownersEntries, useRegexMatcher); + codeownersEntries); Assert.That(entry.Owners, Has.Count.EqualTo(expectedMatch ? 1 : 0)); } @@ -223,6 +220,5 @@ private static void VerifyGetMatchingCodeownersEntry( public record TestCase( string CodeownersPath, string TargetPath, - bool ExpectedLegacyMatch, bool ExpectedNewMatch); } diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs index b206011cfb2..1ce50113dad 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs @@ -12,8 +12,7 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; /// /// 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. @@ -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 /// /// private const string SecondaryCodeownersFilePathSuffix = "/.github/CODEOWNERS2"; @@ -71,7 +70,7 @@ public class CodeownersManualAnalysisTests [Test] // Runtime <1s public void OwnersForAzureDev() - => WriteOwnersToCsvUsingRegexMatcher( + => WriteOwnersToCsv( targetDirPathSuffix: "/../azure-dev", outputFileNamePrefix: "azure-dev", ignoredPathPrefixes: ".git|artifacts"); @@ -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"); @@ -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) @@ -171,7 +170,6 @@ private void WriteOwnersToCsvUsingRegexMatcher( targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, - useRegexMatcher: true, outputFileNamePrefix); } @@ -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"); @@ -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. /// - private void WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( + private void WriteTwoCodeownersFilesOwnersDiffToCsv( string targetDirPathSuffix, string outputFileNamePrefix, string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) @@ -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); } @@ -296,7 +293,6 @@ private static void WriteOwnersToCsv( string targetDirPathSuffix, string codeownersFilePathSuffix, string ignoredPrefixes, - bool useRegexMatcher, string outputFilePrefix) { var stopwatch = Stopwatch.StartNew(); @@ -306,8 +302,7 @@ private static void WriteOwnersToCsv( Dictionary ownersData = RetrieveCodeowners( targetDirPathSuffix, codeownersFilePathSuffix, - ignoredPrefixes, - useRegexMatcher); + ignoredPrefixes); List outputLines = new List { "PATH | PATH EXPRESSION | OWNERS | ISSUE" }; @@ -495,8 +490,7 @@ private static void WriteOwnersDiffToCsv( ( string targetDirPathSuffix, string codeownersFilePathSuffix, - string ignoredPrefixes, - bool useRegexMatcher + string ignoredPrefixes )[] input, string outputFilePrefix) { @@ -505,13 +499,11 @@ bool useRegexMatcher Dictionary leftOwners = RetrieveCodeowners( input[0].targetDirPathSuffix, input[0].codeownersFilePathSuffix, - input[0].ignoredPrefixes, - input[0].useRegexMatcher); + input[0].ignoredPrefixes); Dictionary rightOwners = RetrieveCodeowners( input[1].targetDirPathSuffix, input[1].codeownersFilePathSuffix, - input[1].ignoredPrefixes, - input[1].useRegexMatcher); + input[1].ignoredPrefixes); string[] diffLines = PathOwnersDiff(leftOwners, rightOwners); @@ -525,8 +517,7 @@ bool useRegexMatcher private static Dictionary RetrieveCodeowners( string targetDirPathSuffix, string codeownersFilePathSuffixToRootDir, - string ignoredPathPrefixes, - bool useRegexMatcher) + string ignoredPathPrefixes) { string rootDir = PathNavigatingToRootDir(CurrentDir); string targetDir = rootDir + targetDirPathSuffix; @@ -547,8 +538,7 @@ private static Dictionary RetrieveCodeowners( // because Contacts.GetMatchingCodeownersEntry() calls ExcludeNonUserAliases(). excludeNonUserAliases: false, targetDir, - ignoredPathPrefixes, - useRegexMatcher); + ignoredPathPrefixes); actualOutput = consoleOutput.GetStdout(); actualErr = consoleOutput.GetStderr(); diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/RetrieveCodeOwnersProgramTests.cs similarity index 90% rename from tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs rename to tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/RetrieveCodeOwnersProgramTests.cs index 9551769f669..2ab96772ac8 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/RetrieveCodeOwnersProgramTests.cs @@ -8,16 +8,17 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; /// /// 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 /// [TestFixture] -public class ProgramGlobPathTests +public class RetrieveCodeOwnersProgramTests { /// /// Given: @@ -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. @@ -39,13 +40,12 @@ public class ProgramGlobPathTests /// /// [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 { @@ -74,8 +74,7 @@ public void OutputsCodeownersForGlobPath() targetPath, codeownersFilePathOrUrl, excludeNonUserAliases, - targetDir, - useRegexMatcher: useRegexMatcher); + targetDir); actualOutput = consoleOutput.GetStdout(); actualErr = consoleOutput.GetStderr(); diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS deleted file mode 100644 index d4a23d1bbb1..00000000000 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS +++ /dev/null @@ -1,33 +0,0 @@ -# Instructions for CODEOWNERS file format and automatic build failure notifications: -# https://github.com/Azure/azure-sdk/blob/main/docs/policies/opensource.md#codeowners - -########### -# SDK -########### - -# Catch-all for SDK changes -/sdk/ @person1 @person2 - -# Service teams -/sdk/azconfig/ @person3 @person4 - -# Example for a service that needs issues to be labeled -# ServiceLabel: %KeyVault %Service Attention -/sdk/keyvault/ @person5 @person6 - -# Example for a service that needs PRs to be labeled -# PRLabel: %label -/sdk/servicebus/ @person7 @person8 - -# Example for a service that needs both issues and PRs to be labeled -# ServiceLabel: %label -# PRLabel: %label -/sdk/eventhubs/ @person7 @person8 - -# This is for testing non user aliases case. -/sdk/testUser/ @azure/azure-sdk-eng @azure-sdk - -# Example for service that does not have the code in the repo but wants issues to be labeled -# Notice the use of the moniker // -# ServiceLabel: %label %Service Attention -// @person7 @person8 diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/test_CODEOWNERS similarity index 100% rename from tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS rename to tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/test_CODEOWNERS diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs index 0b2d2dc2d65..1661dda8d51 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs @@ -37,10 +37,6 @@ public static class Program /// Defaults to ".git". /// Example usage: ".git|foo|bar" /// - /// - /// Whether to use the new matcher for CODEOWNERS file paths, that supports matching - /// entries with * and **, instead of silently ignoring them. - /// /// /// On STDOUT: The JSON representation of the matched CodeownersEntry. /// "new CodeownersEntry()" if no path in the CODEOWNERS data matches. @@ -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 { @@ -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, @@ -108,8 +101,7 @@ private static Dictionary GetCodeownersForGlobPath( string targetDir, string codeownersFilePathOrUrl, bool excludeNonUserAliases, - string[]? ignoredPathPrefixes = null, - bool useRegexMatcher = CodeownersFile.UseRegexMatcherDefault) + string[]? ignoredPathPrefixes = null) { ignoredPathPrefixes ??= Array.Empty(); @@ -118,8 +110,7 @@ private static Dictionary GetCodeownersForGlobPath( targetPath, targetDir, codeownersFilePathOrUrl, - ignoredPathPrefixes, - useRegexMatcher); + ignoredPathPrefixes); if (excludeNonUserAliases) codeownersEntries.Values.ToList().ForEach(entry => entry.ExcludeNonUserAliases()); @@ -130,14 +121,12 @@ private static Dictionary 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(); diff --git a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs index ba90dcde5f8..83f1dc376ee 100644 --- a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs +++ b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs @@ -8,8 +8,6 @@ namespace Azure.Sdk.Tools.CodeOwnersParser { public static class CodeownersFile { - public const bool UseRegexMatcherDefault = true; - public static List GetCodeownersEntriesFromFileOrUrl( string codeownersFilePathOrUrl) { @@ -38,19 +36,17 @@ public static List GetCodeownersEntries(string codeownersConten public static CodeownersEntry GetMatchingCodeownersEntry( string targetPath, - string codeownersFilePathOrUrl, - bool useRegexMatcher = UseRegexMatcherDefault) + string codeownersFilePathOrUrl) { var codeownersEntries = GetCodeownersEntriesFromFileOrUrl(codeownersFilePathOrUrl); - return GetMatchingCodeownersEntry(targetPath, codeownersEntries, useRegexMatcher); + return GetMatchingCodeownersEntry(targetPath, codeownersEntries); } public static Dictionary GetMatchingCodeownersEntries( GlobFilePath targetPath, string targetDir, string codeownersFilePathOrUrl, - string[]? ignoredPathPrefixes = null, - bool useRegexMatcher = UseRegexMatcherDefault) + string[]? ignoredPathPrefixes = null) { ignoredPathPrefixes ??= Array.Empty(); @@ -62,21 +58,17 @@ public static Dictionary GetMatchingCodeownersEntries( path => path, path => GetMatchingCodeownersEntry( path, - codeownersEntries, - useRegexMatcher)); + codeownersEntries)); return codeownersEntriesByPath; } public static CodeownersEntry GetMatchingCodeownersEntry( string targetPath, - List codeownersEntries, - bool useRegexMatcher = UseRegexMatcherDefault) + List codeownersEntries) { Debug.Assert(targetPath != null); - return useRegexMatcher - ? new MatchedCodeownersEntry(targetPath, codeownersEntries).Value - : GetMatchingCodeownersEntryLegacyImpl(targetPath, codeownersEntries); + return new MatchedCodeownersEntry(targetPath, codeownersEntries).Value; } private static CodeownersEntry ProcessCodeownersLine( @@ -124,28 +116,5 @@ private static string NormalizeLine(string line) // Remove tabs and trim extra whitespace ? line.Replace('\t', ' ').Trim() : line; - - private static CodeownersEntry GetMatchingCodeownersEntryLegacyImpl( - string targetPath, - List codeownersEntries) - { - // Normalize the start and end of the paths by trimming slash - targetPath = targetPath.Trim('/'); - - // We want to find the match closest to the bottom of the codeowners file. - // CODEOWNERS sorts the paths in order of 'RepoPath', 'ServicePath' and then 'PackagePath'. - for (int i = codeownersEntries.Count - 1; i >= 0; i--) - { - string codeownersPath = codeownersEntries[i].PathExpression.Trim('/'); - // Note that this only matches on paths without glob patterns which is good enough - // for our current scenarios but in the future might need to support globs - if (targetPath.StartsWith(codeownersPath, StringComparison.OrdinalIgnoreCase)) - { - return codeownersEntries[i]; - } - } - - return new CodeownersEntry(); - } } } diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs index c42f729b6a8..a719576029d 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs @@ -13,11 +13,7 @@ namespace Azure.Sdk.Tools.CodeOwnersParser /// To use this class, construct it, passing as input relevant paths. /// Then, to obtain the value of the matched entry, reference "Value" member. /// - /// This class uses a regex-based wildcard-supporting (* and **) matcher, compared to the old one, located in: - /// - /// CodeownersFile.GetMatchingCodeownersEntryLegacyImpl() - /// - /// The old matcher is prefix-based, strips suffix "/", and doesn't support wildcards. + /// This class uses a regex-based wildcard-supporting (* and **) matcher. /// /// This matcher reflects the matching behavior of the built-in GitHub CODEOWNERS interpreter, /// but with additional assumptions imposed about the paths present in CODEOWNERS, as guaranteed @@ -29,7 +25,7 @@ namespace Azure.Sdk.Tools.CodeOwnersParser /// /// The validation spec is given in this comment: /// https://github.com/Azure/azure-sdk-tools/issues/4859#issuecomment-1370360622 - /// See also ProgramGlobPathTests and CodeownersFileTests tests. + /// See also RetrieveCodeOwnersProgramTests and CodeownersFileTests tests. /// /// Reference: /// https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax