From 259ffe66ae221c6409f714530761c0261a3776bf Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Sat, 28 Jan 2023 00:23:45 -0800 Subject: [PATCH 1/8] Improve CodeownersManualAnalysisTests --- .../CodeownersManualAnalysisTests.cs | 500 +++++++++++++----- .../CodeOwnersParser.sln.DotSettings | 1 + 2 files changed, 381 insertions(+), 120 deletions(-) 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 f25d7d4ee13..91305823ced 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,8 @@ 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, -/// or using specified matching logic. +/// 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. /// /// These tools are to be run manually, locally, by a developer. /// They do not participate in an automated regression test suite. @@ -23,124 +23,304 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; /// Then run the desired tool as a unit test, either from your IDE, /// or "dotnet test" command line tool. /// -/// You can import the tool output into Excel, using .CSV import wizard and -/// selecting "|" as column separator. +/// These tools assume you have made local repo clones of relevant repositories. +/// Ensure that the local repo clones you run these tools against are clean. +/// This is because these tools do not support .gitignore. +/// Hence if you do local builds you might add minutes to runtime, and get spurious results. +/// +/// For explanation how to interpret and work with the output .csv file produced by these +/// tools, see comment on +/// +/// WriteOwnersDiffToCsv +/// +/// Related work: +/// Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher +/// https://github.com/Azure/azure-sdk-tools/pull/5088 /// [TestFixture(Ignore = "Tools to be used manually")] public class CodeownersManualAnalysisTests { - private const string DefaultIgnoredPathPrefixes = Program.DefaultIgnoredPrefixes; private const string OwnersDiffOutputPathSuffix = "_owners_diff.csv"; private const string OwnersDataOutputPathSuffix = "_owners.csv"; - // All these paths assume that appropriate repositories are cloned into the same local - // directory as "azure-sdk-tools" repo, containing this logic. - // So the dir layout is like: - // /azure-sdk-tools/ - // /azure-sdk-for-net/ - // /azure-sdk-for-java/ - // /azure-sdk-for-.../ - // ... - private const string AzureSdkForNetTargetDirPathSuffix = "/../azure-sdk-for-net"; - private const string AzureSdkForPythonTargetDirPathSuffix = "/../azure-sdk-for-python"; - // TODO: add more repos here. + /// + /// Given name of the language langName, returns path to a local clone of "azure-sdk-for-langName" + /// repository. + /// + /// This method assumes you have ensured the local clone is present at appropriate path ahead of time. + /// + /// + private static string LangRepoTargetDirPathSuffix(string langName) => "/../azure-sdk-for-" + langName; private const string CodeownersFilePathSuffix = "/.github/CODEOWNERS"; - // Current dir, ".", is "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0". + // Current dir, ".", is expected to be a dir in local clone of Azure/azure-sdk-tools repo, + // where "." denotes "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0". private const string CurrentDir = "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0"; - #region Owners diff + #region Tests - Owners data + + [Test] // Runtime <1s + public void OwnersForAzureDev() + => WriteOwnersToCsv( + targetDirPathSuffix: "/../azure-dev", + outputFileNamePrefix: "azure-dev", + ignoredPathPrefixes: ".git|artifacts"); + + // @formatter:off + [Test] public void OwnersForAzureSdkForAndroid() => WriteLangRepoOwnersToCsv("android"); // Runtime <1s + [Test] public void OwnersForAzureSdkForC() => WriteLangRepoOwnersToCsv("c"); // Runtime <1s + [Test] public void OwnersForAzureSdkForCpp() => WriteLangRepoOwnersToCsv("cpp"); // Runtime <1s + [Test] public void OwnersForAzureSdkForGo() => WriteLangRepoOwnersToCsv("go"); // Runtime <1s + [Test] public void OwnersForAzureSdkForIos() => WriteLangRepoOwnersToCsv("ios"); // Runtime <1s + [Test] public void OwnersForAzureSdkForJava() => WriteLangRepoOwnersToCsv("java"); // Runtime ~1m 11s + [Test] public void OwnersForAzureSdkForJs() => WriteLangRepoOwnersToCsv("js"); // Runtime ~1m 53s + [Test] public void OwnersForAzureSdkForNet() => WriteLangRepoOwnersToCsv("net"); // Runtime ~30s + [Test] public void OwnersForAzureSdkForPython() => WriteLangRepoOwnersToCsv("python"); // Runtime ~20s + // @formatter:on + + [Test] // Runtime <1s + public void OwnersForAzureSdkTools() + => WriteOwnersToCsv( + targetDirPathSuffix: "", + outputFileNamePrefix: "azure-sdk-tools", + ignoredPathPrefixes: ".git|artifacts"); + + #endregion + #region Tests - Owners diffs for CODEOWNERS with /**/ci.yml and /**/tests.yml paths deleted + + // https://github.com/Azure/azure-sdk-for-android/blob/main/.github/CODEOWNERS + // No build failure notifications are configured for this repo. + // Runtime: <1s + [Test] + public void OwnersDiffForAzureSdkForAndroid() + => WriteLangRepoOwnersDiffToCsv("android", pathsToDelete: new []{ "/**/ci.yml" }); + + // https://github.com/Azure/azure-sdk-for-c/blob/main/.github/CODEOWNERS + // Runtime: <1s + [Test] + public void OwnersDiffForAzureSdkForC() + => WriteLangRepoOwnersDiffToCsv("c", pathsToDelete: new []{ "/**/ci.yml" }); + + // https://github.com/Azure/azure-sdk-for-cpp/blob/main/.github/CODEOWNERS + // Runtime: <1s + [Test] + public void OwnersDiffForAzureSdkForCpp() + => WriteLangRepoOwnersDiffToCsv("cpp", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + + // https://github.com/Azure/azure-sdk-for-go/blob/main/.github/CODEOWNERS + // Runtime: ~2s + [Test] + public void OwnersDiffForAzureSdkForGo() + => WriteLangRepoOwnersDiffToCsv("go", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + + // https://github.com/Azure/azure-sdk-for-ios/blob/main/.github/CODEOWNERS + // No build failure notifications are configured for this repo. + // Runtime: <1s + [Test] + public void OwnersDiffForAzureSdkForIos() + => WriteLangRepoOwnersDiffToCsv("ios", pathsToDelete: new []{ "/**/ci.yml" }); + + // https://github.com/Azure/azure-sdk-for-java/blob/main/.github/CODEOWNERS + // Runtime: ~2m 32s + [Test] + public void OwnersDiffForAzureSdkForJava() + => WriteLangRepoOwnersDiffToCsv("java", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + + // https://github.com/Azure/azure-sdk-for-js/blob/main/.github/CODEOWNERS + // Runtime: ~3m 49s + [Test] + public void OwnersDiffForAzureSdkForJs() + => WriteLangRepoOwnersDiffToCsv("js", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + + // https://github.com/Azure/azure-sdk-for-net/blob/main/.github/CODEOWNERS + // These pathsToDelete have been since deleted by this PR: + // https://github.com/Azure/azure-sdk-for-net/pull/33595 + // Runtime: ~1m 01s + [Test] + public void OwnersDiffForAzureSdkForNet() + => WriteLangRepoOwnersDiffToCsv("net", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + + // https://github.com/Azure/azure-sdk-for-python/blob/main/.github/CODEOWNERS + // Runtime: ~45s [Test] - public void WriteToFileCodeownersMatcherDiffForAzureSdkTools() - { - // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone. - var targetDirPathSuffix = ""; - var codeownersPathSuffix = CodeownersFilePathSuffix; - var ignoredPrefixes = ".git|artifacts"; - WriteToFileOwnersDiff(new[] - { - (targetDirPathSuffix, codeownersPathSuffix, ignoredPrefixes, useRegexMatcher: false), - (targetDirPathSuffix, codeownersPathSuffix, ignoredPrefixes, useRegexMatcher: true) - }, outputFilePrefix: "azure-sdk-tools"); - } + public void OwnersDiffForAzureSdkForPython() + => WriteLangRepoOwnersDiffToCsv("python", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + + #endregion + + #region Tests - Owners diffs for the prefix-based vs regex-based CODEOWNERS matchers [Test] - public void WriteToFileCodeownersMatcherDiffForAzureSdkForNet() - { - WriteToFileOwnersDiff( - new[] - { - (AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: false), - (AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: true) - }, + public void MatcherDiffForAzureSdkTools() + => WriteMatcherDiffToCsv( + // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone, + // which is supposed to contain the code you are reading right now. + targetDirPathSuffix: "", + outputFilePrefix: "azure-sdk-tools", + ignoredPathPrefixes: ".git|artifacts"); + + [Test] // Runtime: ~1m 30s + public void MatcherDiffForAzureSdkForNet() + => WriteMatcherDiffToCsv( + targetDirPathSuffix: LangRepoTargetDirPathSuffix("net"), outputFilePrefix: "azure-sdk-for-net"); - } - [Test] - public void WriteToFileWildcardRemovalDiffForAzureSdkForPython() - { - string codeownersCopyPathSuffix = CreateCodeownersCopyWithPathDeletion( - AzureSdkForPythonTargetDirPathSuffix, - CodeownersFilePathSuffix, - pathsToDelete: new[] {"/**/tests.yml", "/**/ci.yml"}); + #endregion - WriteToFileOwnersDiff( - new[] - { - (AzureSdkForPythonTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: true), - (AzureSdkForPythonTargetDirPathSuffix, codeownersCopyPathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: true) - }, - outputFilePrefix: "azure-sdk-for-python"); - } + #region Parameterized tests - Owners - #endregion - - #region Owners data + private void WriteLangRepoOwnersToCsv(string langName) + => WriteOwnersToCsv( + targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), + outputFileNamePrefix: $"azure-sdk-for-{langName}", + ignoredPathPrefixes: ".git|artifacts"); - [Test] - public void WriteToFileRegexMatcherCodeownersForAzureSdkTools() - { - // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone. - var targetDirPathSuffix = ""; - var codeownersPathSuffix = "/.github/CODEOWNERS"; - var ignoredPrefixes = ".git|artifacts"; - WriteToFileOwnersData( + private void WriteOwnersToCsv( + string targetDirPathSuffix, + string outputFileNamePrefix, + string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) + => WriteOwnersToCsv( targetDirPathSuffix, - codeownersPathSuffix, - ignoredPrefixes, - useRegexMatcher: true, - outputFilePrefix: "azure-sdk-tools"); - } - - [Test] - public void WriteToFileRegexMatcherCodeownersForAzureSdkForNet() - => WriteToFileOwnersData( - AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, + ignoredPathPrefixes, useRegexMatcher: true, - outputFilePrefix: "azure-sdk-for-net"); + outputFileNamePrefix); - [Test] - public void WriteToFileRegexMatcherCodeownersForAzureSdkForPython() - => WriteToFileOwnersData( - AzureSdkForPythonTargetDirPathSuffix, + #endregion + + #region Parameterized tests - Owners diff + + private void WriteLangRepoOwnersDiffToCsv(string langName, params string[] pathsToDelete) + => WriteOwnersDiffToCsv( + targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), + outputFileNamePrefix: $"azure-sdk-for-{langName}", + pathsToDelete, + ignoredPathPrefixes: ".git|artifacts"); + + /// + /// This method is an invocation of: + /// + /// WriteOwnersDiffToCsv + /// + /// with following meanings bound to LEFT and RIGHT: + /// + /// LEFT: RetrieveCodeowners configuration using the the new regex-based wildcard-supporting matcher, + /// and given input repository CODEOWNERS file. + /// + /// RIGHT: RetrieveCodeowners configuration using the the new regex-based wildcard-supporting matcher + /// (same as for LEFT), and given input repository CODEOWNERS file with 'pathsToDelete' paths deleted from it. + /// + /// As such, it is useful to determine how owners will change if paths like "/**/ci.yml" or "/**/tests.yml" + /// are deleted from the CODEOWNERS file. + /// Such owners change will naturally affect both auto-assigned PR reviewers, + /// as well as recipients of build failure notifications. + /// + private void WriteOwnersDiffToCsv( + string targetDirPathSuffix, + string outputFileNamePrefix, + string[] pathsToDelete, + string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) + { + (string codeownersCopyPath, string codeownersCopyPathSuffix) = CreateCodeownersCopyWithPathsDeleted( + targetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, - useRegexMatcher: true, - outputFilePrefix: "azure-sdk-for-python"); + pathsToDelete); + + try + { + WriteOwnersDiffToCsv( + new[] + { + (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true), + (targetDirPathSuffix, codeownersCopyPathSuffix, ignoredPathPrefixes, useRegexMatcher: true) + }, + outputFileNamePrefix); + } + finally + { + if (File.Exists(codeownersCopyPath)) + File.Delete(codeownersCopyPath); + } + } + + /// + /// This method is an invocation of: + /// + /// WriteOwnersDiffToCsv + /// + /// with following meanings bound to LEFT and RIGHT: + /// + /// LEFT: RetrieveCodeowners configuration using given input repository CODEOWNERS file, + /// and using the legacy prefix-based CODEOWNERS paths matcher. + /// + /// RIGHT: RetrieveCodeowners configuration using given input repository CODEOWNERS file, + /// and using the new regex-based wildcard-supporting matcher. + /// prefix-based CODEOWNERS paths matcher. + /// + /// As such, this method is useful for determining how build failure notification + /// recipients will change once the new matcher is enabled. + /// The PR reviewers will remain unchanged, as they use the GitHub CODEOWNERS + /// interpreter which independent of ours and always supported wildcards. + /// + private void WriteMatcherDiffToCsv( + string targetDirPathSuffix, + string outputFilePrefix, + string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) + { + WriteOwnersDiffToCsv( + new[] + { + (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: false), + (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true) + }, + outputFilePrefix); + } #endregion - private static void WriteToFileOwnersData( + #region private static + + /// + /// This method is similar to: + /// + /// WriteOwnersDiffToCsv + /// + /// Except it is not doing any diffing: it just evaluates one invocation of + /// Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main + /// and returns its information, in similar, but simplified table format. + /// + /// If given path, provided in column PATH, did not match any path in CODEOWNERS file, + /// the column PATH EXPRESSION will have a value of _____ . + /// + /// In addition, this method also does an validation of CODEOWNERS paths + /// and if it find a problem with given path, it returns output lines with ISSUE column + /// populated and PATH column empty, as there is no path to speak of - only CODEOWNERS path, + /// provided in PATH EXPRESSION column, is present. + /// + /// The ISSUE column has following codes: + /// + /// INVALID_PATH_SHOULD_START_WITH_SLASH + /// All CODEOWNERS paths must start with "/", but given path doesn't. + /// Such path will still be processed by our CODEOWNERS interpreter, but nevertheless it is + /// invalid and should be fixed. + /// + /// INVALID_PATH_SHOULD_HAVE_SUFFIX_SLASH_TO_DENOTE_DIR + /// CODEOWNERS file contains a simple (i.e. without wildcards) path that is expected to match against + /// a file, as it does not end with "/". However, the repository contains a directory with the same + /// path. This means such CODEOWNERS path will never match any input path. Usually the proper fix + /// is to add the missing suffix "/" to the path to make it correctly match against the existing directory. + /// + /// WILDCARD_FILE_PATH_NEEDS_MANUAL_EVAL + /// Same situation as above, but the CODEOWNERS path is a file path with a wildcard, hence current + /// validation implementation cannot yet determine if it should be a path to directory or not. + /// Hence, this needs to be checked manually by ensuring that the wildcard file path matches + /// at least one file in the repository. + /// + private static void WriteOwnersToCsv( string targetDirPathSuffix, - string codeownersPathSuffix, + string codeownersFilePathSuffix, string ignoredPrefixes, bool useRegexMatcher, string outputFilePrefix) @@ -149,14 +329,14 @@ private static void WriteToFileOwnersData( string rootDir = PathNavigatingToRootDir(CurrentDir); string targetDir = rootDir + targetDirPathSuffix; - Dictionary ownersData = RunMain( + Dictionary ownersData = RetrieveCodeowners( targetDirPathSuffix, - codeownersPathSuffix, + codeownersFilePathSuffix, ignoredPrefixes, useRegexMatcher); List outputLines = - new List { "PATH | PATH EXPRESSION | COMMA-SEPARATED OWNERS" }; + new List { "PATH | PATH EXPRESSION | OWNERS | ISSUE" }; foreach (KeyValuePair kvp in ownersData) { string path = kvp.Key; @@ -167,7 +347,7 @@ private static void WriteToFileOwnersData( $"| {string.Join(",", entry.Owners)}"); } - WriteToFileMissingSuffixSlashesForDirPaths(targetDir, codeownersPathSuffix, outputLines); + outputLines.AddRange(LinesWithIssues(targetDir, codeownersFilePathSuffix)); var outputFilePath = outputFilePrefix + OwnersDataOutputPathSuffix; File.WriteAllLines(outputFilePath, outputLines); @@ -176,13 +356,32 @@ private static void WriteToFileOwnersData( $"Time taken: {stopwatch.Elapsed}."); } - private static void WriteToFileMissingSuffixSlashesForDirPaths( + // Possible future work: + // instead of returning lines with issues, consider returning the modified & fixed CODEOWNERS file. + // It could work by reading all the lines, then replacing the wrong + // lines by using dict replacement. Need to be careful about retaining spaces to not misalign, + // e.g. + // "sdk/ @own1" --> "/sdk/ @own1" // space removed to keep alignment + // but also: + // "sdk/ @own1" --> "/sdk/ @own1" // space not removed, because it would be invalid. + private static List LinesWithIssues( string targetDir, - string codeownersPathSuffix, - List outputLines) + string codeownersPathSuffix) { + List outputLines = new List(); List entries = - CodeownersFile.GetCodeownersEntriesFromFileOrUrl(targetDir + codeownersPathSuffix); + CodeownersFile.GetCodeownersEntriesFromFileOrUrl(targetDir + codeownersPathSuffix) + .Where(entry => !entry.PathExpression.StartsWith("#")) + .ToList(); + + foreach (CodeownersEntry entry in entries.Where(entry => !entry.PathExpression.StartsWith("/"))) + { + outputLines.Add( + "|" + + $"{entry.PathExpression} " + + $"| {string.Join(",", entry.Owners)}" + + "| INVALID_PATH_SHOULD_START_WITH_SLASH"); + } foreach (CodeownersEntry entry in entries.Where(entry => !entry.PathExpression.EndsWith("/"))) { @@ -195,9 +394,10 @@ private static void WriteToFileMissingSuffixSlashesForDirPaths( // For example, /a/**/b could match against /a/foo/b/c, meaning // the path is invalid. outputLines.Add( + "|" + $"{entry.PathExpression} " + - $"| WILDCARD_PATH_NEEDS_MANUAL_EVAL " + - $"| {string.Join(",", entry.Owners)}"); + $"| {string.Join(",", entry.Owners)}" + + "| WILDCARD_FILE_PATH_NEEDS_MANUAL_EVAL"); } else { @@ -207,17 +407,20 @@ private static void WriteToFileMissingSuffixSlashesForDirPaths( if (Directory.Exists(pathToDir)) outputLines.Add( + "|" + $"{entry.PathExpression} " + - $"| INVALID_PATH_SHOULD_HAVE_SUFFIX_SLASH_TO_DENOTE_DIR " + - $"| {string.Join(",", entry.Owners)}"); + $"| {string.Join(",", entry.Owners)}" + + "| INVALID_PATH_SHOULD_HAVE_SUFFIX_SLASH_TO_DENOTE_DIR"); } } + return outputLines; } - private string CreateCodeownersCopyWithPathDeletion( - string targetDirPathSuffix, - string codeownersFilePathSuffix, - string[] pathsToDelete) + private static (string codeownersCopy, string codeownersCopyPathSuffix) + CreateCodeownersCopyWithPathsDeleted( + string targetDirPathSuffix, + string codeownersFilePathSuffix, + string[] pathsToDelete) { string rootDir = PathNavigatingToRootDir(CurrentDir); string targetDir = rootDir + targetDirPathSuffix; @@ -229,26 +432,75 @@ private string CreateCodeownersCopyWithPathDeletion( var codeownersCopyPath = codeownersPath + "-copy"; File.WriteAllLines(codeownersCopyPath, codeownersLines); - return codeownersFilePathSuffix + "-copy"; + return (codeownersCopyPath, codeownersFilePathSuffix + "-copy"); } - private static void WriteToFileOwnersDiff(( - string targetDirPathSuffix, - string codeownersPathSuffix, - string ignoredPrefixes, - bool useRegexMatcher)[] input, + /// + /// Writes to .csv file the difference of owners for all paths in given repository, + /// between two invocations of Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main, + /// denoted as LEFT and RIGHT. RetrieveCodeOwners.Program.Main method reads + /// all files in given input repository, and tries to find owners for them based on + /// CODEOWNERS matching configuration given as its parameters. + /// + /// You can import the test output into Excel, using .csv import wizard and + /// selecting "|" as column separator. + /// + /// The resulting .csv file has following headers: + /// + /// DIFF CODE | PATH | LEFT PATH EXPRESSION | RIGHT PATH EXPRESSION | LEFT OWNERS | RIGHT OWNERS + /// + /// where LEFT denotes the RetrieveCodeOwners.Program.Main configuration as provided by input[0]. + /// and RIGHT denotes the RetrieveCodeOwners.Program.Main configuration as provided by input[1]. + /// + /// The columns have following values and meanings: + /// + /// DIFF CODE: + /// PATH _____-RIGHT + /// A file with given path, given in the column PATH, was not matched to any CODEOWNERS + /// path when using the LEFT configuration but it was matched when using the RIGHT configuration. + /// + /// PATH LEFT -_____ + /// Analogous to the case described above, but LEFT configuration has matched, and RIGHT didn't. + /// + /// PATH _____-_____ + /// A file with given path did not match to any CODEOWNERS path, whether using the LEFT + /// configuration or RIGHT configuration. + /// Such file has effectively no owners assigned, no matter which configuration is used. + /// + /// OWNERS DIFF + /// A file with given path matched both when using LEFT and RIGHT configurations, but + /// the CODEOWNERS path to which it matched has different set of owners. + /// + /// PATH: + /// A path to the file being matched against CODEOWNERS path to determine owners. + /// + /// LEFT PATH EXPRESSION: + /// RIGHT PATH EXPRESSION: + /// A CODEOWNERS path that matched to PATH when using LEFT (or RIGHT, respectively) configuration. + /// + /// LEFT OWNERS: + /// RIGHT OWNERS: + /// The owners assigned to given LEFT PATH EXPRESSION (or RIGHT PATH EXPRESSION, respectively). + /// + private static void WriteOwnersDiffToCsv( + ( + string targetDirPathSuffix, + string codeownersFilePathSuffix, + string ignoredPrefixes, + bool useRegexMatcher + )[] input, string outputFilePrefix) { var stopwatch = Stopwatch.StartNew(); - Dictionary leftOwners = RunMain( + Dictionary leftOwners = RetrieveCodeowners( input[0].targetDirPathSuffix, - input[0].codeownersPathSuffix, + input[0].codeownersFilePathSuffix, input[0].ignoredPrefixes, input[0].useRegexMatcher); - Dictionary rightOwners = RunMain( + Dictionary rightOwners = RetrieveCodeowners( input[1].targetDirPathSuffix, - input[1].codeownersPathSuffix, + input[1].codeownersFilePathSuffix, input[1].ignoredPrefixes, input[1].useRegexMatcher); @@ -261,14 +513,17 @@ private static void WriteToFileOwnersDiff(( $"Time taken: {stopwatch.Elapsed}."); } - private static Dictionary RunMain( + private static Dictionary RetrieveCodeowners( string targetDirPathSuffix, - string codeownersPathSuffixToRootDir, + string codeownersFilePathSuffixToRootDir, string ignoredPathPrefixes, bool useRegexMatcher) { string rootDir = PathNavigatingToRootDir(CurrentDir); string targetDir = rootDir + targetDirPathSuffix; + string codeownersFilePath = targetDir + codeownersFilePathSuffixToRootDir; + Debug.Assert(Directory.Exists(targetDir)); + Debug.Assert(File.Exists(codeownersFilePath)); string actualOutput, actualErr; int returnCode; @@ -277,8 +532,11 @@ private static Dictionary RunMain( // Act returnCode = Program.Main( targetPath: "/**", - codeownersFilePathOrUrl: targetDir + codeownersPathSuffixToRootDir, - excludeNonUserAliases: true, // true because of Contacts.GetMatchingCodeownersEntry() calls ExcludeNonUserAliases(). + codeownersFilePath, + // false because we want to see the full owners diff, but observe that + // for the build failure notification recipients determination it should be true, + // because Contacts.GetMatchingCodeownersEntry() calls ExcludeNonUserAliases(). + excludeNonUserAliases: false, targetDir, ignoredPathPrefixes, useRegexMatcher); @@ -361,4 +619,6 @@ private static string[] PathOwnersDiff( return outputLines.ToArray(); } + + #endregion } diff --git a/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings b/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings index 3d218284c9a..d042e12eb56 100644 --- a/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings +++ b/tools/code-owners-parser/CodeOwnersParser.sln.DotSettings @@ -2,4 +2,5 @@ PR True True + True True \ No newline at end of file From 222adad911b466d637849bf0807c43077327e1b8 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 12:57:28 -0800 Subject: [PATCH 2/8] add support for reporting CODEOWNERS paths with unsupported fragments --- .../CodeownersManualAnalysisTests.cs | 45 +++++++++++++++---- .../MatchedCodeownersEntry.cs | 12 ++--- 2 files changed, 43 insertions(+), 14 deletions(-) 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 91305823ced..e89883944ec 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 @@ -76,7 +76,7 @@ public void OwnersForAzureDev() [Test] public void OwnersForAzureSdkForJava() => WriteLangRepoOwnersToCsv("java"); // Runtime ~1m 11s [Test] public void OwnersForAzureSdkForJs() => WriteLangRepoOwnersToCsv("js"); // Runtime ~1m 53s [Test] public void OwnersForAzureSdkForNet() => WriteLangRepoOwnersToCsv("net"); // Runtime ~30s - [Test] public void OwnersForAzureSdkForPython() => WriteLangRepoOwnersToCsv("python"); // Runtime ~20s + [Test] public void OwnersForAzureSdkForPython() => WriteLangRepoOwnersToCsv("python"); // Runtime ~30s // @formatter:on [Test] // Runtime <1s @@ -301,6 +301,10 @@ private void WriteMatcherDiffToCsv( /// /// The ISSUE column has following codes: /// + /// INVALID_PATH_CONTAINS_UNSUPPORTED_FRAGMENTS + /// All CODEOWNERS paths must not contain unsupported path fragments, as defined by: + /// Azure.Sdk.Tools.CodeOwnersParser.MatchedCodeownersEntry.ContainsUnsupportedFragments + /// /// INVALID_PATH_SHOULD_START_WITH_SLASH /// All CODEOWNERS paths must start with "/", but given path doesn't. /// Such path will still be processed by our CODEOWNERS interpreter, but nevertheless it is @@ -317,6 +321,9 @@ private void WriteMatcherDiffToCsv( /// validation implementation cannot yet determine if it should be a path to directory or not. /// Hence, this needs to be checked manually by ensuring that the wildcard file path matches /// at least one file in the repository. + /// + /// Known limitation: + /// If given CODEOWNERS path has no owners listed on its line, this method will not report such path as invalid. /// private static void WriteOwnersToCsv( string targetDirPathSuffix, @@ -347,7 +354,7 @@ private static void WriteOwnersToCsv( $"| {string.Join(",", entry.Owners)}"); } - outputLines.AddRange(LinesWithIssues(targetDir, codeownersFilePathSuffix)); + outputLines.AddRange(PathsWithIssues(targetDir, codeownersFilePathSuffix)); var outputFilePath = outputFilePrefix + OwnersDataOutputPathSuffix; File.WriteAllLines(outputFilePath, outputLines); @@ -364,7 +371,7 @@ private static void WriteOwnersToCsv( // "sdk/ @own1" --> "/sdk/ @own1" // space removed to keep alignment // but also: // "sdk/ @own1" --> "/sdk/ @own1" // space not removed, because it would be invalid. - private static List LinesWithIssues( + private static List PathsWithIssues( string targetDir, string codeownersPathSuffix) { @@ -374,15 +381,26 @@ private static List LinesWithIssues( .Where(entry => !entry.PathExpression.StartsWith("#")) .ToList(); - foreach (CodeownersEntry entry in entries.Where(entry => !entry.PathExpression.StartsWith("/"))) - { - outputLines.Add( + outputLines.AddRange(PathsWithMissingPrefixSlash(entries)); + outputLines.AddRange(PathsWithMissingSuffixSlash(targetDir, entries)); + outputLines.AddRange(PathsWithUnsupportedFragments(entries)); + + return outputLines; + } + + private static List PathsWithMissingPrefixSlash(List entries) + => entries + .Where(entry => !entry.PathExpression.StartsWith("/")) + .Select(entry => "|" + $"{entry.PathExpression} " + $"| {string.Join(",", entry.Owners)}" + - "| INVALID_PATH_SHOULD_START_WITH_SLASH"); - } + "| INVALID_PATH_SHOULD_START_WITH_SLASH") + .ToList(); + private static List PathsWithMissingSuffixSlash(string targetDir, List entries) + { + List outputLines = new List(); foreach (CodeownersEntry entry in entries.Where(entry => !entry.PathExpression.EndsWith("/"))) { if (entry.ContainsWildcard) @@ -416,6 +434,17 @@ private static List LinesWithIssues( return outputLines; } + private static List PathsWithUnsupportedFragments(List entries) + => entries + .Where(entry => MatchedCodeownersEntry.ContainsUnsupportedFragments(entry.PathExpression)) + .Select( + entry => + "|" + + $"{entry.PathExpression} " + + $"| {string.Join(",", entry.Owners)}" + + "| INVALID_PATH_CONTAINS_UNSUPPORTED_FRAGMENTS") + .ToList(); + private static (string codeownersCopy, string codeownersCopyPathSuffix) CreateCodeownersCopyWithPathsDeleted( string targetDirPathSuffix, diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs index 042185c6984..56db516e998 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs @@ -28,7 +28,7 @@ namespace Azure.Sdk.Tools.CodeOwnersParser /// https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners#codeowners-syntax /// https://git-scm.com/docs/gitignore#_pattern_format /// - internal class MatchedCodeownersEntry + public class MatchedCodeownersEntry { /// /// Token for temporarily substituting "**" in regex, to avoid it being escaped when @@ -48,6 +48,10 @@ internal class MatchedCodeownersEntry /// public bool IsValid => IsCodeownersPathValid(this.Value.PathExpression); + public static bool ContainsUnsupportedFragments(string codeownersPath) + => ContainsUnsupportedCharacters(codeownersPath) + || ContainsUnsupportedSequences(codeownersPath); + /// /// Any CODEOWNERS path with these characters will be skipped. /// Note these are valid parts of file paths, but we are not supporting @@ -114,10 +118,6 @@ private CodeownersEntry GetMatchingCodeownersEntry( private CodeownersEntry NoMatchCodeownersEntry { get; } = new CodeownersEntry(); - private static bool ContainsUnsupportedFragments(string codeownersPath) - => ContainsUnsupportedCharacters(codeownersPath) - || ContainsUnsupportedSequences(codeownersPath); - /// /// See the comment on unsupportedChars. /// @@ -284,7 +284,7 @@ private static string NormalizePath(string codeownersPath) /// - if the path expression does not end with "/", at least one matching /// file exists in the repository. /// - private bool IsCodeownersPathValid(string codeownersPathExpression) + private static bool IsCodeownersPathValid(string codeownersPathExpression) => codeownersPathExpression.StartsWith("/") && !ContainsUnsupportedFragments(codeownersPathExpression); } } From c9abfdbec4da46c733989cce31c3cfcc10457151 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 13:37:55 -0800 Subject: [PATCH 3/8] make output file prefixes for diffs for matcher be unique, to prevent overwriting output files for diffs for wildcard paths removal --- .../CodeownersManualAnalysisTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 e89883944ec..9acf119cdac 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 @@ -158,14 +158,14 @@ public void MatcherDiffForAzureSdkTools() // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone, // which is supposed to contain the code you are reading right now. targetDirPathSuffix: "", - outputFilePrefix: "azure-sdk-tools", + outputFilePrefix: "azure-sdk-tools_matcher", ignoredPathPrefixes: ".git|artifacts"); [Test] // Runtime: ~1m 30s public void MatcherDiffForAzureSdkForNet() => WriteMatcherDiffToCsv( targetDirPathSuffix: LangRepoTargetDirPathSuffix("net"), - outputFilePrefix: "azure-sdk-for-net"); + outputFilePrefix: "azure-sdk-for-net_matcher"); #endregion From d115844af30d9200c05b9cd152c42de3e663d4bf Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 18:16:24 -0800 Subject: [PATCH 4/8] instead of diffing to CODEOWNERS with paths deleted, diff to free-form manually-provided CODEOWNERS2 --- .../CodeownersManualAnalysisTests.cs | 159 +++++++++--------- 1 file changed, 75 insertions(+), 84 deletions(-) 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 9acf119cdac..fc273b29a9f 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 @@ -54,6 +54,15 @@ public class CodeownersManualAnalysisTests private const string CodeownersFilePathSuffix = "/.github/CODEOWNERS"; + /// + /// This file is expected to be manually created by you, in your local repo clone. + /// For details of usage of this file, see: + /// + /// WriteTwoCodeownersFilesOwnersDiffToCsv + /// + /// + private const string SecondaryCodeownersFilePathSuffix = "/.github/CODEOWNERS2"; + // Current dir, ".", is expected to be a dir in local clone of Azure/azure-sdk-tools repo, // where "." denotes "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0". private const string CurrentDir = "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0"; @@ -62,7 +71,7 @@ public class CodeownersManualAnalysisTests [Test] // Runtime <1s public void OwnersForAzureDev() - => WriteOwnersToCsv( + => WriteOwnersToCsvUsingRegexMatcher( targetDirPathSuffix: "/../azure-dev", outputFileNamePrefix: "azure-dev", ignoredPathPrefixes: ".git|artifacts"); @@ -81,7 +90,7 @@ public void OwnersForAzureDev() [Test] // Runtime <1s public void OwnersForAzureSdkTools() - => WriteOwnersToCsv( + => WriteOwnersToCsvUsingRegexMatcher( targetDirPathSuffix: "", outputFileNamePrefix: "azure-sdk-tools", ignoredPathPrefixes: ".git|artifacts"); @@ -93,60 +102,42 @@ public void OwnersForAzureSdkTools() // https://github.com/Azure/azure-sdk-for-android/blob/main/.github/CODEOWNERS // No build failure notifications are configured for this repo. // Runtime: <1s - [Test] - public void OwnersDiffForAzureSdkForAndroid() - => WriteLangRepoOwnersDiffToCsv("android", pathsToDelete: new []{ "/**/ci.yml" }); + [Test] public void OwnersDiffForAzureSdkForAndroid() => WriteLangRepoOwnersDiffToCsv("android"); // https://github.com/Azure/azure-sdk-for-c/blob/main/.github/CODEOWNERS // Runtime: <1s - [Test] - public void OwnersDiffForAzureSdkForC() - => WriteLangRepoOwnersDiffToCsv("c", pathsToDelete: new []{ "/**/ci.yml" }); + [Test] public void OwnersDiffForAzureSdkForC() => WriteLangRepoOwnersDiffToCsv("c"); // https://github.com/Azure/azure-sdk-for-cpp/blob/main/.github/CODEOWNERS // Runtime: <1s - [Test] - public void OwnersDiffForAzureSdkForCpp() - => WriteLangRepoOwnersDiffToCsv("cpp", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + [Test] public void OwnersDiffForAzureSdkForCpp() => WriteLangRepoOwnersDiffToCsv("cpp"); // https://github.com/Azure/azure-sdk-for-go/blob/main/.github/CODEOWNERS // Runtime: ~2s - [Test] - public void OwnersDiffForAzureSdkForGo() - => WriteLangRepoOwnersDiffToCsv("go", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + [Test] public void OwnersDiffForAzureSdkForGo() => WriteLangRepoOwnersDiffToCsv("go"); // https://github.com/Azure/azure-sdk-for-ios/blob/main/.github/CODEOWNERS // No build failure notifications are configured for this repo. // Runtime: <1s - [Test] - public void OwnersDiffForAzureSdkForIos() - => WriteLangRepoOwnersDiffToCsv("ios", pathsToDelete: new []{ "/**/ci.yml" }); + [Test] public void OwnersDiffForAzureSdkForIos() => WriteLangRepoOwnersDiffToCsv("ios"); // https://github.com/Azure/azure-sdk-for-java/blob/main/.github/CODEOWNERS // Runtime: ~2m 32s - [Test] - public void OwnersDiffForAzureSdkForJava() - => WriteLangRepoOwnersDiffToCsv("java", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + [Test] public void OwnersDiffForAzureSdkForJava() => WriteLangRepoOwnersDiffToCsv("java"); // https://github.com/Azure/azure-sdk-for-js/blob/main/.github/CODEOWNERS // Runtime: ~3m 49s - [Test] - public void OwnersDiffForAzureSdkForJs() - => WriteLangRepoOwnersDiffToCsv("js", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + [Test] public void OwnersDiffForAzureSdkForJs() => WriteLangRepoOwnersDiffToCsv("js"); // https://github.com/Azure/azure-sdk-for-net/blob/main/.github/CODEOWNERS // These pathsToDelete have been since deleted by this PR: // https://github.com/Azure/azure-sdk-for-net/pull/33595 // Runtime: ~1m 01s - [Test] - public void OwnersDiffForAzureSdkForNet() - => WriteLangRepoOwnersDiffToCsv("net", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + [Test] public void OwnersDiffForAzureSdkForNet() => WriteLangRepoOwnersDiffToCsv("net"); // https://github.com/Azure/azure-sdk-for-python/blob/main/.github/CODEOWNERS // Runtime: ~45s - [Test] - public void OwnersDiffForAzureSdkForPython() - => WriteLangRepoOwnersDiffToCsv("python", pathsToDelete: new []{ "/**/ci.yml", "/**/tests.yml" }); + [Test] public void OwnersDiffForAzureSdkForPython() => WriteLangRepoOwnersDiffToCsv("python"); #endregion @@ -154,7 +145,7 @@ public void OwnersDiffForAzureSdkForPython() [Test] public void MatcherDiffForAzureSdkTools() - => WriteMatcherDiffToCsv( + => WriteMatcherOwnersDiffToCsv( // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone, // which is supposed to contain the code you are reading right now. targetDirPathSuffix: "", @@ -163,7 +154,7 @@ public void MatcherDiffForAzureSdkTools() [Test] // Runtime: ~1m 30s public void MatcherDiffForAzureSdkForNet() - => WriteMatcherDiffToCsv( + => WriteMatcherOwnersDiffToCsv( targetDirPathSuffix: LangRepoTargetDirPathSuffix("net"), outputFilePrefix: "azure-sdk-for-net_matcher"); @@ -172,31 +163,40 @@ public void MatcherDiffForAzureSdkForNet() #region Parameterized tests - Owners private void WriteLangRepoOwnersToCsv(string langName) - => WriteOwnersToCsv( + => WriteOwnersToCsvUsingRegexMatcher( targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), outputFileNamePrefix: $"azure-sdk-for-{langName}", ignoredPathPrefixes: ".git|artifacts"); - private void WriteOwnersToCsv( + private void WriteOwnersToCsvUsingRegexMatcher( string targetDirPathSuffix, string outputFileNamePrefix, string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) - => WriteOwnersToCsv( + { + string rootDir = PathNavigatingToRootDir(CurrentDir); + string targetDir = rootDir + targetDirPathSuffix; + Debug.Assert(Directory.Exists(targetDir), + $"Ensure you have cloned the repo into '{targetDir}'. " + + "See comments on CodeownersManualAnalysisTests and WriteOwnersToCsv for details."); + Debug.Assert(File.Exists(targetDir + CodeownersFilePathSuffix), + $"Ensure you have cloned the repo into '{targetDir}'. " + + "See comments on CodeownersManualAnalysisTests and WriteOwnersToCsv for details."); + WriteOwnersToCsv( targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true, outputFileNamePrefix); + } #endregion #region Parameterized tests - Owners diff private void WriteLangRepoOwnersDiffToCsv(string langName, params string[] pathsToDelete) - => WriteOwnersDiffToCsv( + => WriteTwoCodeownersFilesOwnersDiffToCsv( targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), outputFileNamePrefix: $"azure-sdk-for-{langName}", - pathsToDelete, ignoredPathPrefixes: ".git|artifacts"); /// @@ -206,43 +206,44 @@ private void WriteLangRepoOwnersDiffToCsv(string langName, params string[] paths /// /// with following meanings bound to LEFT and RIGHT: /// - /// LEFT: RetrieveCodeowners configuration using the the new regex-based wildcard-supporting matcher, - /// and given input repository CODEOWNERS file. + /// LEFT: RetrieveCodeowners configuration using the new regex-based wildcard-supporting matcher, + /// and given input local repository clone CODEOWNERS file. /// - /// RIGHT: RetrieveCodeowners configuration using the the new regex-based wildcard-supporting matcher - /// (same as for LEFT), and given input repository CODEOWNERS file with 'pathsToDelete' paths deleted from it. + /// RIGHT: RetrieveCodeowners configuration using the new regex-based wildcard-supporting matcher + /// (same as for LEFT), and given input repository CODEOWNERS2 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. /// /// As such, it is useful to determine how owners will change if paths like "/**/ci.yml" or "/**/tests.yml" /// are deleted from the CODEOWNERS file. /// Such owners change will naturally affect both auto-assigned PR reviewers, /// as well as recipients of build failure notifications. /// - private void WriteOwnersDiffToCsv( + private void WriteTwoCodeownersFilesOwnersDiffToCsv( string targetDirPathSuffix, string outputFileNamePrefix, - string[] pathsToDelete, string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) { - (string codeownersCopyPath, string codeownersCopyPathSuffix) = CreateCodeownersCopyWithPathsDeleted( - targetDirPathSuffix, - CodeownersFilePathSuffix, - pathsToDelete); + string rootDir = PathNavigatingToRootDir(CurrentDir); + string targetDir = rootDir + targetDirPathSuffix; + Debug.Assert(Directory.Exists(targetDir), + $"Ensure you have cloned the repo into '{targetDir}'. " + + "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 WriteTwoCodeownersFilesOwnersDiffToCsv for details."); + Debug.Assert(File.Exists(targetDir + SecondaryCodeownersFilePathSuffix), + $"Ensure you have created '{Path.GetFullPath(targetDir + SecondaryCodeownersFilePathSuffix)}'. " + + $"See comment on WriteTwoCodeownersFilesOwnersDiffToCsv for details."); - try - { - WriteOwnersDiffToCsv( - new[] - { - (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true), - (targetDirPathSuffix, codeownersCopyPathSuffix, ignoredPathPrefixes, useRegexMatcher: true) - }, - outputFileNamePrefix); - } - finally - { - if (File.Exists(codeownersCopyPath)) - File.Delete(codeownersCopyPath); - } + WriteOwnersDiffToCsv( + new[] + { + (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true), + (targetDirPathSuffix, SecondaryCodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true) + }, + outputFileNamePrefix); } /// @@ -252,10 +253,10 @@ private void WriteOwnersDiffToCsv( /// /// with following meanings bound to LEFT and RIGHT: /// - /// LEFT: RetrieveCodeowners configuration using given input repository CODEOWNERS file, + /// LEFT: RetrieveCodeowners configuration using given input local repository clone CODEOWNERS file, /// and using the legacy prefix-based CODEOWNERS paths matcher. /// - /// RIGHT: RetrieveCodeowners configuration using given input repository CODEOWNERS file, + /// RIGHT: RetrieveCodeowners configuration using given input local repository clone CODEOWNERS file, /// and using the new regex-based wildcard-supporting matcher. /// prefix-based CODEOWNERS paths matcher. /// @@ -264,11 +265,20 @@ private void WriteOwnersDiffToCsv( /// The PR reviewers will remain unchanged, as they use the GitHub CODEOWNERS /// interpreter which independent of ours and always supported wildcards. /// - private void WriteMatcherDiffToCsv( + private void WriteMatcherOwnersDiffToCsv( string targetDirPathSuffix, string outputFilePrefix, string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) { + string rootDir = PathNavigatingToRootDir(CurrentDir); + string targetDir = rootDir + targetDirPathSuffix; + Debug.Assert(Directory.Exists(targetDir), + $"Ensure you have cloned the repo into '{targetDir}'. " + + "See comments on CodeownersManualAnalysisTests and WriteMatcherOwnersDiffToCsv for details."); + Debug.Assert(File.Exists(targetDir + CodeownersFilePathSuffix), + $"Ensure you have cloned the repo into '{targetDir}'. " + + "See comments on CodeownersManualAnalysisTests and WriteMatcherOwnersDiffToCsv for details."); + WriteOwnersDiffToCsv( new[] { @@ -407,7 +417,7 @@ private static List PathsWithMissingSuffixSlash(string targetDir, List PathsWithUnsupportedFragments(List "| INVALID_PATH_CONTAINS_UNSUPPORTED_FRAGMENTS") .ToList(); - private static (string codeownersCopy, string codeownersCopyPathSuffix) - CreateCodeownersCopyWithPathsDeleted( - string targetDirPathSuffix, - string codeownersFilePathSuffix, - string[] pathsToDelete) - { - string rootDir = PathNavigatingToRootDir(CurrentDir); - string targetDir = rootDir + targetDirPathSuffix; - string codeownersPath = targetDir + codeownersFilePathSuffix; - - var codeownersLines = File.ReadAllLines(codeownersPath); - codeownersLines = codeownersLines - .Where(line => !pathsToDelete.Any(line.Contains)).ToArray(); - - var codeownersCopyPath = codeownersPath + "-copy"; - File.WriteAllLines(codeownersCopyPath, codeownersLines); - return (codeownersCopyPath, codeownersFilePathSuffix + "-copy"); - } - /// /// Writes to .csv file the difference of owners for all paths in given repository, /// between two invocations of Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main, From 04a5bd8ec88fc5c9ef732d12bf886233afbf75c1 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 21:54:33 -0800 Subject: [PATCH 5/8] collapse the sets of tests diffing matchers and diffing CODEOWNERS contents into one set of tests --- .../CodeownersManualAnalysisTests.cs | 91 +++---------------- 1 file changed, 15 insertions(+), 76 deletions(-) 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 fc273b29a9f..c4cd2f3f3e2 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 @@ -58,7 +58,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: /// - /// WriteTwoCodeownersFilesOwnersDiffToCsv + /// WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv /// /// private const string SecondaryCodeownersFilePathSuffix = "/.github/CODEOWNERS2"; @@ -97,7 +97,7 @@ public void OwnersForAzureSdkTools() #endregion - #region Tests - Owners diffs for CODEOWNERS with /**/ci.yml and /**/tests.yml paths deleted + #region Tests - Owners diffs for the prefix-based vs regex-based CODEOWNERS matchers, plus differing contents. // https://github.com/Azure/azure-sdk-for-android/blob/main/.github/CODEOWNERS // No build failure notifications are configured for this repo. @@ -141,25 +141,6 @@ public void OwnersForAzureSdkTools() #endregion - #region Tests - Owners diffs for the prefix-based vs regex-based CODEOWNERS matchers - - [Test] - public void MatcherDiffForAzureSdkTools() - => WriteMatcherOwnersDiffToCsv( - // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone, - // which is supposed to contain the code you are reading right now. - targetDirPathSuffix: "", - outputFilePrefix: "azure-sdk-tools_matcher", - ignoredPathPrefixes: ".git|artifacts"); - - [Test] // Runtime: ~1m 30s - public void MatcherDiffForAzureSdkForNet() - => WriteMatcherOwnersDiffToCsv( - targetDirPathSuffix: LangRepoTargetDirPathSuffix("net"), - outputFilePrefix: "azure-sdk-for-net_matcher"); - - #endregion - #region Parameterized tests - Owners private void WriteLangRepoOwnersToCsv(string langName) @@ -194,7 +175,7 @@ private void WriteOwnersToCsvUsingRegexMatcher( #region Parameterized tests - Owners diff private void WriteLangRepoOwnersDiffToCsv(string langName, params string[] pathsToDelete) - => WriteTwoCodeownersFilesOwnersDiffToCsv( + => WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), outputFileNamePrefix: $"azure-sdk-for-{langName}", ignoredPathPrefixes: ".git|artifacts"); @@ -206,21 +187,21 @@ private void WriteLangRepoOwnersDiffToCsv(string langName, params string[] paths /// /// with following meanings bound to LEFT and RIGHT: /// - /// LEFT: RetrieveCodeowners configuration using the new regex-based wildcard-supporting matcher, + /// LEFT: RetrieveCodeowners configuration using the legacy prefix-based CODEOWNERS paths matcher, /// and given input local repository clone CODEOWNERS file. /// - /// RIGHT: RetrieveCodeowners configuration using the new regex-based wildcard-supporting matcher - /// (same as for LEFT), and given input repository CODEOWNERS2 file. + /// RIGHT: RetrieveCodeowners configuration using the new regex-based wildcard-supporting matcher, + /// and 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. /// - /// As such, it is useful to determine how owners will change if paths like "/**/ci.yml" or "/**/tests.yml" - /// are deleted from the CODEOWNERS file. - /// Such owners change will naturally affect both auto-assigned PR reviewers, - /// as well as recipients of build failure notifications. + /// 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 + /// build definition .yml files. /// - private void WriteTwoCodeownersFilesOwnersDiffToCsv( + private void WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( string targetDirPathSuffix, string outputFileNamePrefix, string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) @@ -229,65 +210,23 @@ private void WriteTwoCodeownersFilesOwnersDiffToCsv( string targetDir = rootDir + targetDirPathSuffix; Debug.Assert(Directory.Exists(targetDir), $"Ensure you have cloned the repo into '{targetDir}'. " + - "See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesOwnersDiffToCsv for details."); + "See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details."); Debug.Assert(File.Exists(targetDir + CodeownersFilePathSuffix), $"Ensure you have cloned the repo into '{targetDir}'. " + - "See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesOwnersDiffToCsv for details."); + "See comments on CodeownersManualAnalysisTests and WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details."); Debug.Assert(File.Exists(targetDir + SecondaryCodeownersFilePathSuffix), $"Ensure you have created '{Path.GetFullPath(targetDir + SecondaryCodeownersFilePathSuffix)}'. " + - $"See comment on WriteTwoCodeownersFilesOwnersDiffToCsv for details."); + $"See comment on WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details."); WriteOwnersDiffToCsv( new[] { - (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true), + (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: false), (targetDirPathSuffix, SecondaryCodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true) }, outputFileNamePrefix); } - /// - /// This method is an invocation of: - /// - /// WriteOwnersDiffToCsv - /// - /// with following meanings bound to LEFT and RIGHT: - /// - /// LEFT: RetrieveCodeowners configuration using given input local repository clone CODEOWNERS file, - /// and using the legacy prefix-based CODEOWNERS paths matcher. - /// - /// RIGHT: RetrieveCodeowners configuration using given input local repository clone CODEOWNERS file, - /// and using the new regex-based wildcard-supporting matcher. - /// prefix-based CODEOWNERS paths matcher. - /// - /// As such, this method is useful for determining how build failure notification - /// recipients will change once the new matcher is enabled. - /// The PR reviewers will remain unchanged, as they use the GitHub CODEOWNERS - /// interpreter which independent of ours and always supported wildcards. - /// - private void WriteMatcherOwnersDiffToCsv( - string targetDirPathSuffix, - string outputFilePrefix, - string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) - { - string rootDir = PathNavigatingToRootDir(CurrentDir); - string targetDir = rootDir + targetDirPathSuffix; - Debug.Assert(Directory.Exists(targetDir), - $"Ensure you have cloned the repo into '{targetDir}'. " + - "See comments on CodeownersManualAnalysisTests and WriteMatcherOwnersDiffToCsv for details."); - Debug.Assert(File.Exists(targetDir + CodeownersFilePathSuffix), - $"Ensure you have cloned the repo into '{targetDir}'. " + - "See comments on CodeownersManualAnalysisTests and WriteMatcherOwnersDiffToCsv for details."); - - WriteOwnersDiffToCsv( - new[] - { - (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: false), - (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true) - }, - outputFilePrefix); - } - #endregion #region private static From 22e295dbe5dbb86c446d06ef7eccab8863bdb243 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 22:35:49 -0800 Subject: [PATCH 6/8] add support for recognizing when path matches dir or file name prefix --- .../CodeownersManualAnalysisTests.cs | 62 +++++++++++++++---- 1 file changed, 50 insertions(+), 12 deletions(-) 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 c4cd2f3f3e2..f06cb03a958 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 @@ -259,11 +259,24 @@ private void WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( /// Such path will still be processed by our CODEOWNERS interpreter, but nevertheless it is /// invalid and should be fixed. /// - /// INVALID_PATH_SHOULD_HAVE_SUFFIX_SLASH_TO_DENOTE_DIR + /// INVALID_PATH_MATCHES_DIR_EXACTLY + /// INVALID_PATH_MATCHES_DIR_EXACTLY_AND_NAME_PREFIX + /// INVALID_PATH_MATCHES_NAME_PREFIX /// CODEOWNERS file contains a simple (i.e. without wildcards) path that is expected to match against - /// a file, as it does not end with "/". However, the repository contains a directory with the same - /// path. This means such CODEOWNERS path will never match any input path. Usually the proper fix + /// a file, as it does not end with "/". However, the repository contains one or more of the following: + /// - a directory with the same path + /// - a directory with such path being its name prefix: e.g. the path is /foobar and the dir is /foobarbaz/ + /// - a file with such path being its name prefix: e.g. the path is /foobar and the file is /foobarbaz.txt + /// + /// Such paths are invalid because they ambiguous and need to be disambiguated. + /// If the match is only to exact directory, then such CODEOWNERS path will never match any input path. + /// Usually the proper fix /// is to add the missing suffix "/" to the path to make it correctly match against the existing directory. + /// If the match is to directory prefix, then this can be solved by appending "*/". This will match both + /// exact directories, and directory prefixes. + /// If the match is to file name prefix only, this can be fixed by appending "*". + /// If the match is both to directory and file name prefixes, possibly multiple paths need to be used, + /// one with "*/" suffix and one with "*" suffix. /// /// WILDCARD_FILE_PATH_NEEDS_MANUAL_EVAL /// Same situation as above, but the CODEOWNERS path is a file path with a wildcard, hence current @@ -303,7 +316,7 @@ private static void WriteOwnersToCsv( $"| {string.Join(",", entry.Owners)}"); } - outputLines.AddRange(PathsWithIssues(targetDir, codeownersFilePathSuffix)); + outputLines.AddRange(PathsWithIssues(targetDir, codeownersFilePathSuffix, paths: ownersData.Keys.ToArray())); var outputFilePath = outputFilePrefix + OwnersDataOutputPathSuffix; File.WriteAllLines(outputFilePath, outputLines); @@ -322,7 +335,8 @@ private static void WriteOwnersToCsv( // "sdk/ @own1" --> "/sdk/ @own1" // space not removed, because it would be invalid. private static List PathsWithIssues( string targetDir, - string codeownersPathSuffix) + string codeownersPathSuffix, + string[] paths) { List outputLines = new List(); List entries = @@ -331,7 +345,7 @@ private static List PathsWithIssues( .ToList(); outputLines.AddRange(PathsWithMissingPrefixSlash(entries)); - outputLines.AddRange(PathsWithMissingSuffixSlash(targetDir, entries)); + outputLines.AddRange(PathsWithMissingSuffixSlash(targetDir, entries, paths)); outputLines.AddRange(PathsWithUnsupportedFragments(entries)); return outputLines; @@ -347,7 +361,10 @@ private static List PathsWithMissingPrefixSlash(List en "| INVALID_PATH_SHOULD_START_WITH_SLASH") .ToList(); - private static List PathsWithMissingSuffixSlash(string targetDir, List entries) + private static List PathsWithMissingSuffixSlash( + string targetDir, + List entries, + string[] paths) { List outputLines = new List(); foreach (CodeownersEntry entry in entries.Where(entry => !entry.PathExpression.EndsWith("/"))) @@ -368,21 +385,42 @@ private static List PathsWithMissingSuffixSlash(string targetDir, List paths.Any( + path => + path.TrimStart('/').StartsWith(trimmedPathExpression) + && path.TrimStart('/').Length != trimmedPathExpression.Length + && !path.TrimStart('/').Substring(trimmedPathExpression.Length).StartsWith('/')); + + private static bool MatchesDirExactly(string targetDir, string trimmedPathExpression) + { + string pathToDir = Path.Combine( + targetDir, + trimmedPathExpression.Replace('/', Path.DirectorySeparatorChar)); + return Directory.Exists(pathToDir); + } + private static List PathsWithUnsupportedFragments(List entries) => entries .Where(entry => MatchedCodeownersEntry.ContainsUnsupportedFragments(entry.PathExpression)) From 87292d6f714c7da08892d6887e4a32b4de118fa7 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 23:13:07 -0800 Subject: [PATCH 7/8] add "owners diff" test for "azure-dev" and remove unused pathsToDelete param --- .../CodeownersManualAnalysisTests.cs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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 f06cb03a958..443536b7818 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 @@ -99,6 +99,13 @@ public void OwnersForAzureSdkTools() #region Tests - Owners diffs for the prefix-based vs regex-based CODEOWNERS matchers, plus differing contents. + [Test] // Runtime <1s + public void OwnersDiffForAzureDev() + => WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( + targetDirPathSuffix: "/../azure-dev", + outputFileNamePrefix: "azure-dev", + ignoredPathPrefixes: ".git|artifacts"); + // https://github.com/Azure/azure-sdk-for-android/blob/main/.github/CODEOWNERS // No build failure notifications are configured for this repo. // Runtime: <1s @@ -130,8 +137,6 @@ public void OwnersForAzureSdkTools() [Test] public void OwnersDiffForAzureSdkForJs() => WriteLangRepoOwnersDiffToCsv("js"); // https://github.com/Azure/azure-sdk-for-net/blob/main/.github/CODEOWNERS - // These pathsToDelete have been since deleted by this PR: - // https://github.com/Azure/azure-sdk-for-net/pull/33595 // Runtime: ~1m 01s [Test] public void OwnersDiffForAzureSdkForNet() => WriteLangRepoOwnersDiffToCsv("net"); @@ -174,7 +179,7 @@ private void WriteOwnersToCsvUsingRegexMatcher( #region Parameterized tests - Owners diff - private void WriteLangRepoOwnersDiffToCsv(string langName, params string[] pathsToDelete) + private void WriteLangRepoOwnersDiffToCsv(string langName) => WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), outputFileNamePrefix: $"azure-sdk-for-{langName}", From cbc7df1afab219dcedd03f384f30cd89984fc77c Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Mon, 30 Jan 2023 23:15:19 -0800 Subject: [PATCH 8/8] add "misalign" to dict. --- .../notification-configuration.sln.DotSettings | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/notification-configuration/notification-configuration.sln.DotSettings b/tools/notification-configuration/notification-configuration.sln.DotSettings index 8c1c0ec65f1..3769d45356b 100644 --- a/tools/notification-configuration/notification-configuration.sln.DotSettings +++ b/tools/notification-configuration/notification-configuration.sln.DotSettings @@ -1,4 +1,5 @@  AAD PR - True \ No newline at end of file + True + True \ No newline at end of file