From b3668aa2de7226307c2e3e859d9b9462aa010d66 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Fri, 27 Jan 2023 15:20:35 -0800 Subject: [PATCH] Add detection of `CODEOWNERS` paths that are missing suffix slash; add support for diffing to `CODEOWNERS` version with some paths removed; add tools for analyzing azure-sdk-for-python `CODEOWNERS`. (#5245) --- ...ts.cs => CodeownersManualAnalysisTests.cs} | 118 ++++++++++++++++-- .../CodeOwnersParser/CodeownersFile.cs | 3 +- 2 files changed, 107 insertions(+), 14 deletions(-) rename tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/{CodeownersDiffTests.cs => CodeownersManualAnalysisTests.cs} (66%) diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersDiffTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs similarity index 66% rename from tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersDiffTests.cs rename to tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs index 79afb4f7a24..f25d7d4ee13 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersDiffTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs @@ -27,7 +27,7 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; /// selecting "|" as column separator. /// [TestFixture(Ignore = "Tools to be used manually")] -public class CodeownersDiffTests +public class CodeownersManualAnalysisTests { private const string DefaultIgnoredPathPrefixes = Program.DefaultIgnoredPrefixes; private const string OwnersDiffOutputPathSuffix = "_owners_diff.csv"; @@ -42,9 +42,14 @@ public class CodeownersDiffTests // /azure-sdk-for-.../ // ... private const string AzureSdkForNetTargetDirPathSuffix = "/../azure-sdk-for-net"; - private const string AzureSdkForNetCodeownersPathSuffix = AzureSdkForNetTargetDirPathSuffix + "/.github/CODEOWNERS"; + private const string AzureSdkForPythonTargetDirPathSuffix = "/../azure-sdk-for-python"; // TODO: add more repos here. + private const string CodeownersFilePathSuffix = "/.github/CODEOWNERS"; + + // Current dir, ".", is "/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 [Test] @@ -52,7 +57,7 @@ public void WriteToFileCodeownersMatcherDiffForAzureSdkTools() { // Empty string here means to just use the root directory of the local "azure-sdk-tools" clone. var targetDirPathSuffix = ""; - var codeownersPathSuffix = "/.github/CODEOWNERS"; + var codeownersPathSuffix = CodeownersFilePathSuffix; var ignoredPrefixes = ".git|artifacts"; WriteToFileOwnersDiff(new[] { @@ -67,14 +72,33 @@ public void WriteToFileCodeownersMatcherDiffForAzureSdkForNet() WriteToFileOwnersDiff( new[] { - (AzureSdkForNetTargetDirPathSuffix, AzureSdkForNetCodeownersPathSuffix, + (AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, DefaultIgnoredPathPrefixes, useRegexMatcher: false), - (AzureSdkForNetTargetDirPathSuffix, AzureSdkForNetCodeownersPathSuffix, + (AzureSdkForNetTargetDirPathSuffix, CodeownersFilePathSuffix, DefaultIgnoredPathPrefixes, useRegexMatcher: true) }, outputFilePrefix: "azure-sdk-for-net"); } + [Test] + public void WriteToFileWildcardRemovalDiffForAzureSdkForPython() + { + string codeownersCopyPathSuffix = CreateCodeownersCopyWithPathDeletion( + AzureSdkForPythonTargetDirPathSuffix, + CodeownersFilePathSuffix, + pathsToDelete: new[] {"/**/tests.yml", "/**/ci.yml"}); + + WriteToFileOwnersDiff( + new[] + { + (AzureSdkForPythonTargetDirPathSuffix, CodeownersFilePathSuffix, + DefaultIgnoredPathPrefixes, useRegexMatcher: true), + (AzureSdkForPythonTargetDirPathSuffix, codeownersCopyPathSuffix, + DefaultIgnoredPathPrefixes, useRegexMatcher: true) + }, + outputFilePrefix: "azure-sdk-for-python"); + } + #endregion #region Owners data @@ -98,11 +122,20 @@ public void WriteToFileRegexMatcherCodeownersForAzureSdkTools() public void WriteToFileRegexMatcherCodeownersForAzureSdkForNet() => WriteToFileOwnersData( AzureSdkForNetTargetDirPathSuffix, - AzureSdkForNetCodeownersPathSuffix, + CodeownersFilePathSuffix, DefaultIgnoredPathPrefixes, useRegexMatcher: true, outputFilePrefix: "azure-sdk-for-net"); + [Test] + public void WriteToFileRegexMatcherCodeownersForAzureSdkForPython() + => WriteToFileOwnersData( + AzureSdkForPythonTargetDirPathSuffix, + CodeownersFilePathSuffix, + DefaultIgnoredPathPrefixes, + useRegexMatcher: true, + outputFilePrefix: "azure-sdk-for-python"); + #endregion private static void WriteToFileOwnersData( @@ -113,6 +146,8 @@ private static void WriteToFileOwnersData( string outputFilePrefix) { var stopwatch = Stopwatch.StartNew(); + string rootDir = PathNavigatingToRootDir(CurrentDir); + string targetDir = rootDir + targetDirPathSuffix; Dictionary ownersData = RunMain( targetDirPathSuffix, @@ -122,7 +157,7 @@ private static void WriteToFileOwnersData( List outputLines = new List { "PATH | PATH EXPRESSION | COMMA-SEPARATED OWNERS" }; - foreach (var kvp in ownersData) + foreach (KeyValuePair kvp in ownersData) { string path = kvp.Key; CodeownersEntry entry = kvp.Value; @@ -132,6 +167,8 @@ private static void WriteToFileOwnersData( $"| {string.Join(",", entry.Owners)}"); } + WriteToFileMissingSuffixSlashesForDirPaths(targetDir, codeownersPathSuffix, outputLines); + var outputFilePath = outputFilePrefix + OwnersDataOutputPathSuffix; File.WriteAllLines(outputFilePath, outputLines); Console.WriteLine($"DONE writing out owners. " + @@ -139,6 +176,62 @@ private static void WriteToFileOwnersData( $"Time taken: {stopwatch.Elapsed}."); } + private static void WriteToFileMissingSuffixSlashesForDirPaths( + string targetDir, + string codeownersPathSuffix, + List outputLines) + { + List entries = + CodeownersFile.GetCodeownersEntriesFromFileOrUrl(targetDir + codeownersPathSuffix); + + 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 + // 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)}"); + } + else + { + string pathToDir = Path.Combine( + targetDir, + entry.PathExpression.TrimStart('/').Replace('/', Path.DirectorySeparatorChar)); + + if (Directory.Exists(pathToDir)) + outputLines.Add( + $"{entry.PathExpression} " + + $"| INVALID_PATH_SHOULD_HAVE_SUFFIX_SLASH_TO_DENOTE_DIR " + + $"| {string.Join(",", entry.Owners)}"); + } + } + } + + 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(); + + var codeownersCopyPath = codeownersPath + "-copy"; + File.WriteAllLines(codeownersCopyPath, codeownersLines); + return codeownersFilePathSuffix + "-copy"; + } + private static void WriteToFileOwnersDiff(( string targetDirPathSuffix, string codeownersPathSuffix, @@ -174,9 +267,8 @@ private static Dictionary RunMain( string ignoredPathPrefixes, bool useRegexMatcher) { - // Current dir, ".", is "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0". - const string currentDir = "/artifacts/bin/Azure.Sdk.Tools.CodeOwnersParser.Tests/Debug/net6.0"; - string rootDir = PathNavigatingToRootDir(currentDir); + string rootDir = PathNavigatingToRootDir(CurrentDir); + string targetDir = rootDir + targetDirPathSuffix; string actualOutput, actualErr; int returnCode; @@ -185,9 +277,9 @@ private static Dictionary RunMain( // Act returnCode = Program.Main( targetPath: "/**", - codeownersFilePathOrUrl: rootDir + codeownersPathSuffixToRootDir, - excludeNonUserAliases: false, - targetDir: rootDir + targetDirPathSuffix, + codeownersFilePathOrUrl: targetDir + codeownersPathSuffixToRootDir, + excludeNonUserAliases: true, // true because of Contacts.GetMatchingCodeownersEntry() calls ExcludeNonUserAliases(). + targetDir, ignoredPathPrefixes, useRegexMatcher); diff --git a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs index 502319580c4..8b7cbf7d436 100644 --- a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs +++ b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs @@ -57,7 +57,8 @@ public static Dictionary GetMatchingCodeownersEntries( var codeownersEntries = GetCodeownersEntriesFromFileOrUrl(codeownersFilePathOrUrl); Dictionary codeownersEntriesByPath = targetPath - .ResolveGlob(targetDir, ignoredPathPrefixes).ToDictionary( + .ResolveGlob(targetDir, ignoredPathPrefixes) + .ToDictionary( path => path, path => GetMatchingCodeownersEntry( path,