From 4cf0beba898be962e6bc48c52bafda7995040cd9 Mon Sep 17 00:00:00 2001 From: Konrad Jamrozik Date: Fri, 13 Jan 2023 17:14:10 -0800 Subject: [PATCH] Add to the `retrieve-codeowners` tool support for returning owners for all paths matching given glob path. (#5134) This PR implements for the `retrieve-codeowners` tool the ability to return not only owners for a single path, but a list of all owners for all paths resolved when matching a glob path given as input. As such, this PR contributes to: - #5135 This work is necessary to be able to compare and diff the owners of all files in repository before and after the regex matcher is turned on, per this comment: - https://github.com/Azure/azure-sdk-tools/pull/5088#pullrequestreview-1241376684 In other words, this PR contributes to unblocking to the following PR: - #5088 And as such, also contributes to: - #2770 This PR will require some upstream changes, which are captured by: - #5103 ### Additional changes - Removed logger from `CodeOwnersParser`; replaced with `Console.Out` and `Console.Error`. This addresses the following comment: - https://github.com/Azure/azure-sdk-tools/pull/5063#discussion_r1066019443 - Prevented the new regex matcher from matching paths that have `*` in them - such paths are malformed anyway (at least on Windows). - A lot of assorted changes to surrounding production & test code - please see the file diff for details. ### Implementation notes This PR leverages [`Microsoft.Extensions.FileSystemGlobbing`](https://www.nuget.org/packages/Microsoft.Extensions.FileSystemGlobbing). Doc: https://learn.microsoft.com/en-us/dotnet/core/extensions/file-globbing --- eng/common/scripts/get-codeowners.ps1 | 2 +- .../CodeownersFileTests.cs | 10 +- ....Sdk.Tools.RetrieveCodeOwners.Tests.csproj | 10 +- .../ConsoleOutput.cs | 45 ++++-- .../ProgramGlobPathTests.cs | 150 ++++++++++++++++++ ...gramTests.cs => ProgramSimplePathTests.cs} | 17 +- .../TestData/InputDir/a.txt | 0 .../TestData/InputDir/b.txt | 12 ++ .../TestData/InputDir/foo/a.txt | 12 ++ .../TestData/InputDir/foo/b.txt | 12 ++ .../TestData/InputDir/foo/bar/a.txt | 0 .../TestData/InputDir/foo/bar/b.txt | 0 .../TestData/glob_path_CODEOWNERS | 9 ++ .../simple_path_CODEOWNERS} | 0 .../Azure.Sdk.Tools.RetrieveCodeOwners.csproj | 3 + .../Program.cs | 147 ++++++++++++----- .../Azure.Sdk.Tools.CodeOwnersParser.csproj | 7 +- .../CodeOwnersParser/CodeownersEntry.cs | 29 ++++ .../CodeOwnersParser/CodeownersFile.cs | 33 +++- .../CodeOwnersParser/GlobFilePath.cs | 42 +++++ .../MatchedCodeownersEntry.cs | 35 ++-- .../CodeOwnersParser/PathExtensions.cs | 7 + 22 files changed, 490 insertions(+), 92 deletions(-) create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs rename tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/{ProgramTests.cs => ProgramSimplePathTests.cs} (78%) create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/a.txt create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/b.txt create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/a.txt create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/b.txt create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/bar/a.txt create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/bar/b.txt create mode 100644 tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS rename tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/{CODEOWNERS => TestData/simple_path_CODEOWNERS} (100%) create mode 100644 tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs create mode 100644 tools/code-owners-parser/CodeOwnersParser/PathExtensions.cs diff --git a/eng/common/scripts/get-codeowners.ps1 b/eng/common/scripts/get-codeowners.ps1 index 76a4eca866c..87271a6230d 100644 --- a/eng/common/scripts/get-codeowners.ps1 +++ b/eng/common/scripts/get-codeowners.ps1 @@ -81,7 +81,7 @@ function TestGetCodeOwner([string]$targetDirectory, [string]$codeOwnerFileLocati } if($Test) { - $testFile = (Resolve-Path $PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CODEOWNERS) + $testFile = (Resolve-Path $PSScriptRoot/../../../tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS) TestGetCodeOwner -targetDirectory "sdk" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person1", "person2") TestGetCodeOwner -targetDirectory "sdk/noPath" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person1", "person2") TestGetCodeOwner -targetDirectory "/sdk/azconfig" -codeOwnerFileLocation $testFile -includeNonUserAliases $true -expectReturn @("person3", "person4") diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs index 7259bed77fa..f5b5765aa19 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.CodeOwnersParser.Tests/CodeownersFileTests.cs @@ -78,6 +78,8 @@ public class CodeownersFileTests new( "/a]b" , "a]b" , true , false ), new( "/a?b" , "a?b" , true , false ), new( "/a?b" , "axb" , false , false ), + new( "/a" , "*" , false , false ), + new( "/*" , "*" , true , false ), new( "/*" , "a" , false , true ), new( "/*" , "a/" , false , false ), new( "/*" , "a/b" , false , false ), @@ -152,20 +154,20 @@ public void TestGetMatchingCodeownersEntry(TestCase testCase) List? codeownersEntries = CodeownersFile.GetCodeownersEntries(testCase.CodeownersPath + "@owner"); - VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useNewImpl: false, testCase.ExpectedLegacyMatch); - VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useNewImpl: true, testCase.ExpectedNewMatch); + VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: false, testCase.ExpectedLegacyMatch); + VerifyGetMatchingCodeownersEntry(testCase, codeownersEntries, useRegexMatcher: true, testCase.ExpectedNewMatch); } private static void VerifyGetMatchingCodeownersEntry( TestCase testCase, List codeownersEntries, - bool useNewImpl, + bool useRegexMatcher, bool expectedMatch) { CodeownersEntry? entryLegacy = // Act CodeownersFile.GetMatchingCodeownersEntry(testCase.TargetPath, - codeownersEntries, useNewImpl: useNewImpl); + codeownersEntries, useRegexMatcher); Assert.That(entryLegacy.Owners, Has.Count.EqualTo(expectedMatch ? 1 : 0)); } diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj index 87d23caffe9..961cc1d70f9 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/Azure.Sdk.Tools.RetrieveCodeOwners.Tests.csproj @@ -2,6 +2,7 @@ net6.0 + 10.0 enable Nullable false @@ -20,8 +21,15 @@ - + PreserveNewest + + PreserveNewest + + + + + diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ConsoleOutput.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ConsoleOutput.cs index 434129f87c4..8b1803391a3 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ConsoleOutput.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ConsoleOutput.cs @@ -4,40 +4,53 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests { /// - /// The class is to redirect console output to string writer. + /// The class is to redirect console STDOUT and STDERR to string writer. /// public class ConsoleOutput : IDisposable { - private readonly StringWriter stringWriter; - private readonly TextWriter originalOutput; + private readonly StringWriter stdoutWriter, stderrWriter; + private readonly TextWriter originalStdout, originalStderr; /// /// The constructor is where we take in the console output and output to string writer. /// public ConsoleOutput() { - this.stringWriter = new StringWriter(); - this.originalOutput = Console.Out; - Console.SetOut(this.stringWriter); + this.stdoutWriter = new StringWriter(); + this.stderrWriter = new StringWriter(); + this.originalStdout = Console.Out; + this.originalStderr = Console.Error; + Console.SetOut(this.stdoutWriter); + Console.SetError(this.stderrWriter); } /// /// Writes the text representation of a string builder to the string. /// /// The string from console output. - public string GetOutput() - { - return this.stringWriter.ToString(); - } + public string GetStdout() + => this.stdoutWriter.ToString(); + + public string[] GetStdoutLines() + => this.stdoutWriter.ToString().Split(Environment.NewLine); + + public string GetStderr() + => this.stderrWriter.ToString(); + + public string[] GetStderrLines() + => this.stderrWriter.ToString().Split(Environment.NewLine); /// /// Releases all resources used by the originalOutput and stringWriter object. /// public void Dispose() { - Console.SetOut(this.originalOutput); - this.stringWriter.Dispose(); - this.originalOutput.Dispose(); + Console.SetOut(this.originalStdout); + Console.SetError(this.originalStderr); + this.stdoutWriter.Dispose(); + this.stderrWriter.Dispose(); + this.originalStdout.Dispose(); + this.originalStderr.Dispose(); // https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1816 GC.SuppressFinalize(this); } @@ -47,8 +60,10 @@ public void Dispose() /// public void Close() { - this.stringWriter.Close(); - this.originalOutput.Close(); + this.stdoutWriter.Close(); + this.stderrWriter.Close(); + this.originalStderr.Close(); + this.originalStdout.Close(); } } diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs new file mode 100644 index 00000000000..9ee3f2a00bc --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramGlobPathTests.cs @@ -0,0 +1,150 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text.Json; +using Azure.Sdk.Tools.CodeOwnersParser; +using NUnit.Framework; + +namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests; + +/// +/// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main(), +/// for scenario in which targetPath is a glob path, i.e. +/// targetPath.IsGlobPath() returns true. +/// +[TestFixture] +public class ProgramGlobPathTests +{ + /// + /// Given: + ///

+ /// codeownersFilePathOrUrl contents of: + /// + /// + /// /** @2star + /// /* @star + /// /foo/**/a.txt @foo_2star_a + /// /foo/*/a.txt @foo_star_a_1 @foo_star_a_2 + /// + /// + /// + /// targetDir contents of: + /// + /// + /// + /// /a.txt + /// /b.txt + /// /foo/a.txt + /// /foo/b.txt + /// /foo/bar/a.txt + /// /foo/bar/b.txt + /// + /// + /// + /// targetPath of: + /// + /// + /// + /// /foo/** + /// + /// + /// + /// excludeNonUserAliases set to false and useRegexMatcher set to true. + ///

+ /// When: + /// The retrieve-codeowners tool is executed on these inputs. + ///

+ /// Then: + /// The tool should return on STDOUT owners matched in following way: + /// + /// + /// /a.txt @star + /// /b.txt @star + /// /foo/a.txt @foo_2star_a + /// /foo/b.txt @2star + /// /foo/bar/a.txt @foo_star_a_1 @foo_star_a_2 + /// /foo/bar/b.txt @2star + /// + /// + [Test] + public void OutputsCodeownersForGlobPath() + { + const string targetDir = "./TestData/InputDir"; + const string targetPath = "/**"; + const string codeownersFilePathOrUrl = "./TestData/glob_path_CODEOWNERS"; + const bool excludeNonUserAliases = false; + const bool useRegexMatcher = true; + + var expectedEntries = new Dictionary + { + // @formatter:off + ["a.txt"] = new CodeownersEntry("/*", new List { "star" }), + ["b.txt"] = new CodeownersEntry("/*", new List { "star" }), + ["foo/a.txt"] = new CodeownersEntry("/foo/**/a.txt", new List { "foo_2star_a" }), + ["foo/b.txt"] = new CodeownersEntry("/**", new List { "2star" }), + ["foo/bar/a.txt"] = new CodeownersEntry("/foo/*/a.txt", new List { "foo_star_a_1", "foo_star_a_2" }), + ["foo/bar/b.txt"] = new CodeownersEntry("/**", new List { "2star" }), + // @formatter:on + }; + + string actualOutput, actualErr; + int returnCode; + using (var consoleOutput = new ConsoleOutput()) + { + // Act + returnCode = Program.Main( + targetPath, + codeownersFilePathOrUrl, + excludeNonUserAliases, + targetDir, + useRegexMatcher); + + actualOutput = consoleOutput.GetStdout(); + actualErr = consoleOutput.GetStderr(); + } + + var actualEntries = TryDeserializeActualEntries(actualOutput, actualErr); + + Assert.Multiple(() => + { + AssertEntries(actualEntries, expectedEntries); + Assert.That(returnCode, Is.EqualTo(0)); + Assert.That(actualErr, Is.EqualTo(string.Empty)); + }); + } + + private static Dictionary TryDeserializeActualEntries( + string actualOutput, + string actualErr) + { + Dictionary actualEntries; + try + { + actualEntries = + JsonSerializer.Deserialize>(actualOutput)!; + } + catch (Exception e) + { + Console.WriteLine(e); + Console.WriteLine("actualOutput: " + actualOutput); + Console.WriteLine("actualErr: " + actualErr); + throw; + } + + return actualEntries; + } + + private static void AssertEntries( + Dictionary actualEntries, + Dictionary expectedEntries) + { + foreach (KeyValuePair kvp in actualEntries) + { + string path = kvp.Key; + CodeownersEntry actualEntry = kvp.Value; + Assert.That(expectedEntries[path], Is.EqualTo(actualEntry), $"path: {path}"); + } + + Assert.That(actualEntries.Count, Is.EqualTo(expectedEntries.Count)); + } +} diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramTests.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramSimplePathTests.cs similarity index 78% rename from tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramTests.cs rename to tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramSimplePathTests.cs index 87fe0a32d5d..d493836184d 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramTests.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/ProgramSimplePathTests.cs @@ -6,12 +6,14 @@ namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests { /// - /// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program. + /// Test class for Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main(), + /// for scenario in which targetPath is a simple path, i.e. + /// targetPath.IsGlobPath() returns false. /// [TestFixture] - public class ProgramTests + public class ProgramSimplePathTests { - private const string CodeownersFilePath = "CODEOWNERS"; + private const string CodeownersFilePath = "./TestData/simple_path_CODEOWNERS"; private static readonly object[] sourceLists = { @@ -25,21 +27,24 @@ public class ProgramTests }; [TestCaseSource(nameof(sourceLists))] - public void TestOnNormalOutput(string targetPath, bool excludeNonUserAliases, List expectedOwners) + public void OutputsCodeownersForSimplePath( + string targetPath, + bool excludeNonUserAliases, + List expectedOwners) { using var consoleOutput = new ConsoleOutput(); // Act Program.Main(targetPath, CodeownersFilePath, excludeNonUserAliases); - string actualOutput = consoleOutput.GetOutput(); + string actualOutput = consoleOutput.GetStdout(); AssertOwners(actualOutput, expectedOwners); } [TestCase("PathNotExist")] [TestCase("http://testLink")] [TestCase("https://testLink")] - public void TestOnError(string codeownersPath) + public void ErrorsOutOnInvalidInputs(string codeownersPath) { Assert.That(Program.Main("sdk", codeownersPath), Is.EqualTo(1)); } diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/a.txt b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/a.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/b.txt b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/b.txt new file mode 100644 index 00000000000..30b72c114c7 --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/b.txt @@ -0,0 +1,12 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests.TestData +{ + class b + { + } +} diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/a.txt b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/a.txt new file mode 100644 index 00000000000..0b1b706b30d --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/a.txt @@ -0,0 +1,12 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests.TestData.foo +{ + class a + { + } +} diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/b.txt b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/b.txt new file mode 100644 index 00000000000..e1bac95f81c --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/b.txt @@ -0,0 +1,12 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Azure.Sdk.Tools.RetrieveCodeOwners.Tests.TestData.foo +{ + class b + { + } +} diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/bar/a.txt b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/bar/a.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/bar/b.txt b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/InputDir/foo/bar/b.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS new file mode 100644 index 00000000000..c09c06a2cc9 --- /dev/null +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/glob_path_CODEOWNERS @@ -0,0 +1,9 @@ +# This file is a test resoure for the test: +# +# Azure.Sdk.Tools.RetrieveCodeOwners.Tests.WildcardTests.TestWildcard +# + +/** @2star +/* @star +/foo/**/a.txt @foo_2star_a +/foo/*/a.txt @foo_star_a_1 @foo_star_a_2 \ No newline at end of file diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CODEOWNERS b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS similarity index 100% rename from tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CODEOWNERS rename to tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/TestData/simple_path_CODEOWNERS diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Azure.Sdk.Tools.RetrieveCodeOwners.csproj b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Azure.Sdk.Tools.RetrieveCodeOwners.csproj index a34a31f19c1..49942461c4c 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Azure.Sdk.Tools.RetrieveCodeOwners.csproj +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Azure.Sdk.Tools.RetrieveCodeOwners.csproj @@ -3,6 +3,9 @@ Exe net6.0 + 10.0 + enable + Nullable true retrieve-codeowners 1.0.0 diff --git a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs index 10f8df3486f..352f50e8976 100644 --- a/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs +++ b/tools/code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners/Program.cs @@ -1,54 +1,119 @@ using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Linq; using System.Text.Json; using Azure.Sdk.Tools.CodeOwnersParser; -namespace Azure.Sdk.Tools.RetrieveCodeOwners +namespace Azure.Sdk.Tools.RetrieveCodeOwners; + +/// +/// See Program.Main comment. +/// +public static class Program { /// - /// See Program.Main comment. + /// Given targetPath and CODEOWNERS file path or https url codeownersFilePathOrUrl, + /// prints out to stdout owners of the targetPath as determined by the CODEOWNERS data. /// - public static class Program + /// The path whose owners are to be determined. Can be a glob path. + /// The https url or path to the CODEOWNERS file. + /// Whether owners that aren't users should be excluded from the + /// returned owners. + /// + /// The directory to search for file paths in case targetPath is a glob path Unused otherwise. + /// + /// + /// Whether to use the new matcher for CODEOWNERS file paths, that supports matching + /// entries with * and **, instead of silently ignoring them. + /// + /// + /// On STDOUT: The JSON representation of the matched CodeownersEntry. + /// "new CodeownersEntry()" if no path in the CODEOWNERS data matches. + ///

+ /// From the Main method: exit code. 0 if successful, 1 if error. + /// + public static int Main( + string targetPath, + string codeownersFilePathOrUrl, + bool excludeNonUserAliases = false, + string? targetDir = null, + bool useRegexMatcher = false) { - /// - /// Given targetPath and CODEOWNERS file path or https url codeownersFilePathOrUrl, - /// prints out to stdout owners of the targetPath as determined by the CODEOWNERS data. - /// - /// The path whose owners are to be determined. - /// The https url or path to the CODEOWNERS file. - /// Whether owners that aren't users should be excluded from the - /// returned owners. - /// - /// On STDOUT: The JSON representation of the matched CodeownersEntry. - /// "new CodeownersEntry()" if no path in the CODEOWNERS data matches. - ///

- /// From the Main method: exit code. 0 if successful, 1 if error. - /// - public static int Main( - string targetPath, - string codeownersFilePathOrUrl, - bool excludeNonUserAliases = false) + try { targetPath = targetPath.Trim(); - try - { - var codeownersEntry = CodeownersFile.GetMatchingCodeownersEntry(targetPath, codeownersFilePathOrUrl); - if (excludeNonUserAliases) - { - codeownersEntry.ExcludeNonUserAliases(); - } - - var codeownersJson = JsonSerializer.Serialize( - codeownersEntry, - new JsonSerializerOptions { WriteIndented = true }); - - Console.WriteLine(codeownersJson); - return 0; - } - catch (Exception e) - { - Console.Error.WriteLine(e.Message); - return 1; - } + targetDir = targetDir?.Trim(); + codeownersFilePathOrUrl = codeownersFilePathOrUrl.Trim(); + + Trace.Assert(!string.IsNullOrWhiteSpace(targetPath)); + Trace.Assert(!string.IsNullOrWhiteSpace(codeownersFilePathOrUrl)); + Trace.Assert(!targetPath.IsGlobFilePath() + || (targetDir != null && Directory.Exists(targetDir))); + + object codeownersData = targetPath.IsGlobFilePath() + ? GetCodeownersForGlobPath( + new GlobFilePath(targetPath), + targetDir!, + codeownersFilePathOrUrl, + excludeNonUserAliases, + useRegexMatcher) + : GetCodeownersForSimplePath( + targetPath, + codeownersFilePathOrUrl, + excludeNonUserAliases, + useRegexMatcher); + + string codeownersJson = JsonSerializer.Serialize( + codeownersData, + new JsonSerializerOptions { WriteIndented = true }); + + Console.WriteLine(codeownersJson); + return 0; } + catch (Exception e) + { + Console.Error.WriteLine(e); + return 1; + } + } + + private static object GetCodeownersForGlobPath( + GlobFilePath targetPath, + string targetDir, + string codeownersFilePathOrUrl, + bool excludeNonUserAliases, + bool useRegexMatcher = false) + { + Dictionary codeownersEntries = + CodeownersFile.GetMatchingCodeownersEntries( + targetPath, + targetDir, + codeownersFilePathOrUrl, + useRegexMatcher); + + if (excludeNonUserAliases) + codeownersEntries.Values.ToList().ForEach(entry => entry.ExcludeNonUserAliases()); + + return codeownersEntries; + } + + private static object GetCodeownersForSimplePath( + string targetPath, + string codeownersFilePathOrUrl, + bool excludeNonUserAliases, + bool useRegexMatcher = false) + { + CodeownersEntry codeownersEntry = + CodeownersFile.GetMatchingCodeownersEntry( + targetPath, + codeownersFilePathOrUrl, + useRegexMatcher); + + if (excludeNonUserAliases) + codeownersEntry.ExcludeNonUserAliases(); + + return codeownersEntry; } } diff --git a/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj b/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj index 03e676e6387..d3ec46ae0ec 100644 --- a/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj +++ b/tools/code-owners-parser/CodeOwnersParser/Azure.Sdk.Tools.CodeOwnersParser.csproj @@ -1,13 +1,16 @@ - false net6.0 + 10.0 + enable + Nullable + false - + diff --git a/tools/code-owners-parser/CodeOwnersParser/CodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/CodeownersEntry.cs index ce5558c9882..5eff1b30514 100644 --- a/tools/code-owners-parser/CodeOwnersParser/CodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/CodeownersEntry.cs @@ -33,6 +33,16 @@ public class CodeownersEntry public bool IsValid => !string.IsNullOrWhiteSpace(PathExpression); + public CodeownersEntry() + { + } + + public CodeownersEntry(string pathExpression, List owners) + { + PathExpression = pathExpression; + Owners = owners; + } + private static string[] SplitLine(string line, char splitOn) => line.Split(new char[] { splitOn }, StringSplitOptions.RemoveEmptyEntries); @@ -157,5 +167,24 @@ private static bool IsGitHubUserAlias(string alias) } return true; } + + protected bool Equals(CodeownersEntry other) + => PathExpression == other.PathExpression + && Owners.SequenceEqual(other.Owners) + && PRLabels.SequenceEqual(other.PRLabels) + && ServiceLabels.SequenceEqual(other.ServiceLabels); + + public override bool Equals(object? obj) + { + // @formatter:off + if (ReferenceEquals(null, obj)) return false; + if (ReferenceEquals(this, obj)) return true; + if (obj.GetType() != this.GetType()) return false; + return Equals((CodeownersEntry)obj); + // @formatter:on + } + + public override int GetHashCode() + => HashCode.Combine(PathExpression, Owners, PRLabels, ServiceLabels); } } diff --git a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs index bb77161aaae..30899be335f 100644 --- a/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs +++ b/tools/code-owners-parser/CodeOwnersParser/CodeownersFile.cs @@ -2,12 +2,16 @@ using System.Collections.Generic; using System.Diagnostics; using System.IO; +using System.Linq; namespace Azure.Sdk.Tools.CodeOwnersParser { public static class CodeownersFile { - public static List GetCodeownersEntriesFromFileOrUrl(string codeownersFilePathOrUrl) + private const bool UseRegexMatcherDefault = false; + + public static List GetCodeownersEntriesFromFileOrUrl( + string codeownersFilePathOrUrl) { string content = FileHelpers.GetFileOrUrlContents(codeownersFilePathOrUrl); return GetCodeownersEntries(content); @@ -35,19 +39,38 @@ public static List GetCodeownersEntries(string codeownersConten public static CodeownersEntry GetMatchingCodeownersEntry( string targetPath, string codeownersFilePathOrUrl, - bool useNewImpl = false) + bool useRegexMatcher = UseRegexMatcherDefault) + { + var codeownersEntries = GetCodeownersEntriesFromFileOrUrl(codeownersFilePathOrUrl); + return GetMatchingCodeownersEntry(targetPath, codeownersEntries, useRegexMatcher); + } + + public static Dictionary GetMatchingCodeownersEntries( + GlobFilePath targetPath, + string targetDir, + string codeownersFilePathOrUrl, + bool useRegexMatcher = UseRegexMatcherDefault) { var codeownersEntries = GetCodeownersEntriesFromFileOrUrl(codeownersFilePathOrUrl); - return GetMatchingCodeownersEntry(targetPath, codeownersEntries, useNewImpl); + + Dictionary codeownersEntriesByPath = targetPath + .ResolveGlob(targetDir).ToDictionary( + path => path, + path => GetMatchingCodeownersEntry( + path, + codeownersEntries, + useRegexMatcher)); + + return codeownersEntriesByPath; } public static CodeownersEntry GetMatchingCodeownersEntry( string targetPath, List codeownersEntries, - bool useNewImpl = false) + bool useRegexMatcher = UseRegexMatcherDefault) { Debug.Assert(targetPath != null); - return useNewImpl + return useRegexMatcher ? new MatchedCodeownersEntry(targetPath, codeownersEntries).Value : GetMatchingCodeownersEntryLegacyImpl(targetPath, codeownersEntries); } diff --git a/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs b/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs new file mode 100644 index 00000000000..ffbf68313a6 --- /dev/null +++ b/tools/code-owners-parser/CodeOwnersParser/GlobFilePath.cs @@ -0,0 +1,42 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Linq; +using Microsoft.Extensions.FileSystemGlobbing; + +namespace Azure.Sdk.Tools.CodeOwnersParser; + +public class GlobFilePath +{ + private readonly string filePath; + + public GlobFilePath(string globFilePath) + { + Debug.Assert(globFilePath.IsGlobFilePath()); + this.filePath = globFilePath; + } + + /// + /// The '*' is the only character that can denote glob pattern + /// in the used globbing library, per: + /// - https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.filesystemglobbing.matcher?view=dotnet-plat-ext-7.0#remarks + /// - https://learn.microsoft.com/en-us/dotnet/core/extensions/file-globbing#pattern-formats + /// + public static bool IsGlobFilePath(string path) + => path.Contains('*'); + + public List ResolveGlob(string directoryPath) + { + var globMatcher = new Matcher(StringComparison.Ordinal); + globMatcher.AddInclude(this.filePath); + + List matchedPaths = globMatcher.GetResultsInFullPath(directoryPath).ToList(); + + matchedPaths = matchedPaths + .Select(path => Path.GetRelativePath(directoryPath, path).Replace("\\", "/")) + .ToList(); + + return matchedPaths; + } +} diff --git a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs index d828f93ea15..267975065b1 100644 --- a/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs +++ b/tools/code-owners-parser/CodeOwnersParser/MatchedCodeownersEntry.cs @@ -1,7 +1,7 @@ +using System; using System.Collections.Generic; using System.Linq; using System.Text.RegularExpressions; -using Microsoft.Extensions.Logging; namespace Azure.Sdk.Tools.CodeOwnersParser { @@ -41,9 +41,9 @@ internal class MatchedCodeownersEntry public readonly CodeownersEntry Value; /// - /// See comment on IsCodeOwnersPathValid + /// See comment on IsCodeownersPathValid /// - public bool IsValid => IsCodeOwnersPathValid(this.Value.PathExpression); + public bool IsValid => IsCodeownersPathValid(this.Value.PathExpression); /// /// Any CODEOWNERS path with these characters will be skipped. @@ -52,20 +52,11 @@ internal class MatchedCodeownersEntry /// private static readonly char[] unsupportedChars = { '[', ']', '!', '?' }; - private readonly ILogger log; - public MatchedCodeownersEntry(string targetPath, List codeownersEntries) { - this.log = CreateLog(); this.Value = GetMatchingCodeownersEntry(targetPath, codeownersEntries); } - private ILogger CreateLog() - { - var loggerFactory = LoggerFactory.Create(builder => { builder.AddSimpleConsole(); }); - return loggerFactory.CreateLogger(); - } - /// /// Returns a CodeownersEntry from codeOwnerEntries that matches targetPath /// per algorithm described in the GitHub CODEOWNERS reference, @@ -77,6 +68,14 @@ private CodeownersEntry GetMatchingCodeownersEntry( string targetPath, List codeownersEntries) { + if (targetPath.Contains('*')) + { + Console.Error.WriteLine( + $"Target path \"{targetPath}\" contains star ('*') which is not supported. " + + "Returning no match without checking for ownership."); + return NoMatchCodeownersEntry; + } + // targetPath is assumed to be absolute w.r.t. repository root, hence we ensure // it starts with "/" to denote that. if (!targetPath.StartsWith("/")) @@ -99,11 +98,13 @@ private CodeownersEntry GetMatchingCodeownersEntry( .FirstOrDefault( entry => Matches(targetPath, entry), // assert: none of the codeownersEntries matched targetPath - new CodeownersEntry()); + NoMatchCodeownersEntry); return matchedEntry; } + private CodeownersEntry NoMatchCodeownersEntry { get; } = new CodeownersEntry(); + /// /// See the comment on unsupportedChars. /// @@ -112,7 +113,7 @@ private bool ContainsUnsupportedCharacters(string codeownersPath) var contains = unsupportedChars.Any(codeownersPath.Contains); if (contains) { - log.LogWarning( + Console.Error.WriteLine( $"CODEOWNERS path \"{codeownersPath}\" contains unsupported characters: " + string.Join(' ', unsupportedChars) + " Because of that this path will never match."); @@ -142,7 +143,7 @@ private Regex ConvertToRegex(string targetPath, string codeownersPath) if (codeownersPath.Contains(DoubleStar) || pattern.Contains(SingleStar)) { - log.LogWarning( + Console.Error.WriteLine( $"CODEOWNERS path \"{codeownersPath}\" contains reserved phrases: " + $"\"{DoubleStar}\" or \"{SingleStar}\""); } @@ -211,7 +212,7 @@ private static string SetPatternSuffix(string targetPath, string pattern) /// /// CODEOWNERS paths that do not start with "/" are relative and considered invalid, - /// See comment on "IsCodeOwnersPathValid" for definition of "valid". + /// See comment on "IsCodeownersPathValid" for definition of "valid". /// However, here we handle such cases to accomodate for parsing CODEOWNERS file /// paths that somehow slipped through that validation. We do so by instead treating /// such paths as if they were absolute to repository root, i.e. starting with "/". @@ -239,7 +240,7 @@ private string NormalizePath(string codeownersPath) /// - if the Value.PathExpression does not end with "/", at least one corresponding /// file exists in the repository. /// - private bool IsCodeOwnersPathValid(string codeownersPath) + private bool IsCodeownersPathValid(string codeownersPath) => codeownersPath.StartsWith("/") && !ContainsUnsupportedCharacters(codeownersPath); } } diff --git a/tools/code-owners-parser/CodeOwnersParser/PathExtensions.cs b/tools/code-owners-parser/CodeOwnersParser/PathExtensions.cs new file mode 100644 index 00000000000..4167b65d7f1 --- /dev/null +++ b/tools/code-owners-parser/CodeOwnersParser/PathExtensions.cs @@ -0,0 +1,7 @@ +namespace Azure.Sdk.Tools.CodeOwnersParser; + +public static class PathExtensions +{ + public static bool IsGlobFilePath(this string path) + => GlobFilePath.IsGlobFilePath(path); +}