From 6da835e2d1193af6d0d65550d7e64a27ace14aac Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Wed, 1 Feb 2023 12:28:37 -0800 Subject: [PATCH] Add manual tools for reporting information required to enable regex-based wildcard-supporting `CODEOWNERS` matcher on all remaining repositories in scope. (#5266) --- .../CodeownersManualAnalysisTests.cs | 532 +++++++++++++----- .../CodeOwnersParser.sln.DotSettings | 1 + .../MatchedCodeownersEntry.cs | 12 +- ...notification-configuration.sln.DotSettings | 3 +- 4 files changed, 406 insertions(+), 142 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..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 @@ -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,278 @@ 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". + /// + /// This file is expected to be manually created by you, in your local repo clone. + /// For details of usage of this file, see: + /// + /// WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv + /// + /// + 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"; - #region Owners diff + #region Tests - Owners data + + [Test] // Runtime <1s + public void OwnersForAzureDev() + => WriteOwnersToCsvUsingRegexMatcher( + 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 ~30s + // @formatter:on + + [Test] // Runtime <1s + public void OwnersForAzureSdkTools() + => WriteOwnersToCsvUsingRegexMatcher( + targetDirPathSuffix: "", + outputFileNamePrefix: "azure-sdk-tools", + ignoredPathPrefixes: ".git|artifacts"); - [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"); - } + #endregion - [Test] - public void WriteToFileCodeownersMatcherDiffForAzureSdkForNet() - { - WriteToFileOwnersDiff( - new[] - { - (AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: false), - (AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: true) - }, - outputFilePrefix: "azure-sdk-for-net"); - } + #region Tests - Owners diffs for the prefix-based vs regex-based CODEOWNERS matchers, plus differing contents. - [Test] - public void WriteToFileWildcardRemovalDiffForAzureSdkForPython() - { - string codeownersCopyPathSuffix = CreateCodeownersCopyWithPathDeletion( - AzureSdkForPythonTargetDirPathSuffix, - CodeownersFilePathSuffix, - pathsToDelete: new[] {"/**/tests.yml", "/**/ci.yml"}); + [Test] // Runtime <1s + public void OwnersDiffForAzureDev() + => WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( + targetDirPathSuffix: "/../azure-dev", + outputFileNamePrefix: "azure-dev", + ignoredPathPrefixes: ".git|artifacts"); - WriteToFileOwnersDiff( - new[] - { - (AzureSdkForPythonTargetDirPathSuffix, CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: true), - (AzureSdkForPythonTargetDirPathSuffix, codeownersCopyPathSuffix, - DefaultIgnoredPathPrefixes, useRegexMatcher: true) - }, - outputFilePrefix: "azure-sdk-for-python"); - } + // 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"); + + // https://github.com/Azure/azure-sdk-for-c/blob/main/.github/CODEOWNERS + // Runtime: <1s + [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"); + + // https://github.com/Azure/azure-sdk-for-go/blob/main/.github/CODEOWNERS + // Runtime: ~2s + [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"); + + // https://github.com/Azure/azure-sdk-for-java/blob/main/.github/CODEOWNERS + // Runtime: ~2m 32s + [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"); + + // https://github.com/Azure/azure-sdk-for-net/blob/main/.github/CODEOWNERS + // Runtime: ~1m 01s + [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"); #endregion - - #region Owners data - [Test] - public void WriteToFileRegexMatcherCodeownersForAzureSdkTools() + #region Parameterized tests - Owners + + private void WriteLangRepoOwnersToCsv(string langName) + => WriteOwnersToCsvUsingRegexMatcher( + targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), + outputFileNamePrefix: $"azure-sdk-for-{langName}", + ignoredPathPrefixes: ".git|artifacts"); + + private void WriteOwnersToCsvUsingRegexMatcher( + string targetDirPathSuffix, + string outputFileNamePrefix, + string ignoredPathPrefixes = Program.DefaultIgnoredPrefixes) { - // 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( + 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, - codeownersPathSuffix, - ignoredPrefixes, + CodeownersFilePathSuffix, + ignoredPathPrefixes, useRegexMatcher: true, - outputFilePrefix: "azure-sdk-tools"); + outputFileNamePrefix); } - [Test] - public void WriteToFileRegexMatcherCodeownersForAzureSdkForNet() - => WriteToFileOwnersData( - AzureSdkForNetTargetDirPathSuffix, - CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, - useRegexMatcher: true, - outputFilePrefix: "azure-sdk-for-net"); + #endregion - [Test] - public void WriteToFileRegexMatcherCodeownersForAzureSdkForPython() - => WriteToFileOwnersData( - AzureSdkForPythonTargetDirPathSuffix, - CodeownersFilePathSuffix, - DefaultIgnoredPathPrefixes, - useRegexMatcher: true, - outputFilePrefix: "azure-sdk-for-python"); + #region Parameterized tests - Owners diff + + private void WriteLangRepoOwnersDiffToCsv(string langName) + => WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( + targetDirPathSuffix: LangRepoTargetDirPathSuffix(langName), + outputFileNamePrefix: $"azure-sdk-for-{langName}", + ignoredPathPrefixes: ".git|artifacts"); + + /// + /// This method is an invocation of: + /// + /// WriteOwnersDiffToCsv + /// + /// 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. + /// + /// 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. + /// + /// 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 WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv( + string targetDirPathSuffix, + string outputFileNamePrefix, + 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 WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details."); + Debug.Assert(File.Exists(targetDir + CodeownersFilePathSuffix), + $"Ensure you have cloned the repo into '{targetDir}'. " + + "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 WriteTwoCodeownersFilesAndMatcherOwnersDiffToCsv for details."); + + WriteOwnersDiffToCsv( + new[] + { + (targetDirPathSuffix, CodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: false), + (targetDirPathSuffix, SecondaryCodeownersFilePathSuffix, ignoredPathPrefixes, useRegexMatcher: true) + }, + outputFileNamePrefix); + } #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_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 + /// invalid and should be fixed. + /// + /// 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 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 + /// 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, - string codeownersPathSuffix, + string codeownersFilePathSuffix, string ignoredPrefixes, bool useRegexMatcher, string outputFilePrefix) @@ -149,14 +303,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 +321,7 @@ private static void WriteToFileOwnersData( $"| {string.Join(",", entry.Owners)}"); } - WriteToFileMissingSuffixSlashesForDirPaths(targetDir, codeownersPathSuffix, outputLines); + outputLines.AddRange(PathsWithIssues(targetDir, codeownersFilePathSuffix, paths: ownersData.Keys.ToArray())); var outputFilePath = outputFilePrefix + OwnersDataOutputPathSuffix; File.WriteAllLines(outputFilePath, outputLines); @@ -176,79 +330,179 @@ 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 PathsWithIssues( string targetDir, string codeownersPathSuffix, - List outputLines) + string[] paths) { + List outputLines = new List(); List entries = - CodeownersFile.GetCodeownersEntriesFromFileOrUrl(targetDir + codeownersPathSuffix); + CodeownersFile.GetCodeownersEntriesFromFileOrUrl(targetDir + codeownersPathSuffix) + .Where(entry => !entry.PathExpression.StartsWith("#")) + .ToList(); + + outputLines.AddRange(PathsWithMissingPrefixSlash(entries)); + outputLines.AddRange(PathsWithMissingSuffixSlash(targetDir, entries, paths)); + 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") + .ToList(); + + private static List PathsWithMissingSuffixSlash( + string targetDir, + List entries, + string[] paths) + { + List outputLines = new List(); foreach (CodeownersEntry entry in entries.Where(entry => !entry.PathExpression.EndsWith("/"))) { if (entry.ContainsWildcard) { // We do not support "the path is to file while it should be to directory" validation for paths // with wildcards yet. To do that, we would first need to resolve the path and see if there exists - // a concrete path that includes the the CODEOWNERS paths supposed-file-name as + // a concrete path that includes the CODEOWNERS paths supposed-file-name as // infix dir. // 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 { - string pathToDir = Path.Combine( - targetDir, - entry.PathExpression.TrimStart('/').Replace('/', Path.DirectorySeparatorChar)); + var trimmedPathExpression = entry.PathExpression.TrimStart('/'); + + bool matchesDirExactly = MatchesDirExactly(targetDir, trimmedPathExpression); + bool matchesNamePrefix = MatchesNamePrefix(paths, trimmedPathExpression); + + if (matchesDirExactly || matchesNamePrefix) + { + string msgCode = matchesDirExactly && matchesNamePrefix ? "MATCHES_DIR_EXACTLY_AND_NAME_PREFIX" : + matchesDirExactly ? "MATCHES_DIR_EXACTLY" : "MATCHES_NAME_PREFIX"; - 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_{msgCode}"); + } } } + return outputLines; } - private string CreateCodeownersCopyWithPathDeletion( - 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(); + private static bool MatchesNamePrefix(string[] paths, string trimmedPathExpression) + => paths.Any( + path => + path.TrimStart('/').StartsWith(trimmedPathExpression) + && path.TrimStart('/').Length != trimmedPathExpression.Length + && !path.TrimStart('/').Substring(trimmedPathExpression.Length).StartsWith('/')); - var codeownersCopyPath = codeownersPath + "-copy"; - File.WriteAllLines(codeownersCopyPath, codeownersLines); - return codeownersFilePathSuffix + "-copy"; + private static bool MatchesDirExactly(string targetDir, string trimmedPathExpression) + { + string pathToDir = Path.Combine( + targetDir, + trimmedPathExpression.Replace('/', Path.DirectorySeparatorChar)); + return Directory.Exists(pathToDir); } - private static void WriteToFileOwnersDiff(( - string targetDirPathSuffix, - string codeownersPathSuffix, - string ignoredPrefixes, - bool useRegexMatcher)[] input, + 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(); + + /// + /// 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 +515,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 +534,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 +621,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 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); } } 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