Fix missing owners for path error to include path (#8450)

* Fix missing owners for path error to include path * update for feedback * update for feedback
Azure · Jun 14, 2024 · 8951f15 · 8951f15
1 parent a2e1a56
commit 8951f15
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 16 deletions.
diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Verification/OwnersTests.cs b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Verification/OwnersTests.cs
@@ -36,43 +36,47 @@ public void InitRepoLabelData()
         [Category("SourceOwners")]
         [Category("Verification")]
         // Source path/owner line with no errors
-        [TestCase($"/sdk/SomePath @{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}0\t@{TestHelpers.TestOwnerNamePartial}2",true)]
+        [TestCase($"/sdk/SomePath @{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}0\t@{TestHelpers.TestOwnerNamePartial}2",true, true)]
         // Moniker Owner lines with no errors
-        [TestCase($"# {MonikerConstants.AzureSdkOwners}: @{TestHelpers.TestOwnerNamePartial}0 @{TestHelpers.TestOwnerNamePartial}4", true)]
-        [TestCase($"# {MonikerConstants.ServiceOwners}: @{TestHelpers.TestOwnerNamePartial}4 @{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}1\t\t@{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}3", true)]
+        [TestCase($"# {MonikerConstants.AzureSdkOwners}: @{TestHelpers.TestOwnerNamePartial}0 @{TestHelpers.TestOwnerNamePartial}4", true, false)]
+        [TestCase($"# {MonikerConstants.ServiceOwners}: @{TestHelpers.TestOwnerNamePartial}4 @{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}1\t\t@{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}3", true, false)]
         // AzureSdkOwners, with no owner defined is legal for a block that ends in a source path/owner line.
-        [TestCase($"# {MonikerConstants.AzureSdkOwners}:", false)]
+        [TestCase($"# {MonikerConstants.AzureSdkOwners}:", false, false)]
         // Source path/owner line with no owners should complain
-        [TestCase($"/sdk/SomePath", true, ErrorMessageConstants.NoOwnersDefined)]
+        // ATTENTION: If ErrorMessageConstants.PathEntryMissingOwners changes, this error needs to change by hand.
+        // The reason being is that string.Format(ErrorMessageConstants.PathEntryMissingOwners, "/sdk/SomePath")
+        // can't be in a TestCase declaration, only a constant.
+        [TestCase($"/sdk/SomePath", true, true, "Path entry, /sdk/SomePath, is missing owners")]
         // AzureSdkOwners, with no owner defined is not legal if the block doesn't end in a source path/owner line.
-        [TestCase($"# {MonikerConstants.AzureSdkOwners}:", true, ErrorMessageConstants.NoOwnersDefined)]
+        [TestCase($"# {MonikerConstants.AzureSdkOwners}:", true, false, ErrorMessageConstants.NoOwnersDefined)]
         // At this point whether or not the line is a moniker or source path/owner line is irrelevant.
         // Test each error individually.
         // Invalid team
-        [TestCase($"# {MonikerConstants.ServiceOwners}: @{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}12", true,
+        [TestCase($"# {MonikerConstants.ServiceOwners}: @{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}12", true, false,
             $"{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}12{ErrorMessageConstants.InvalidTeamPartial}")]
         // Invalid User
-        [TestCase($"# {MonikerConstants.ServiceOwners}: @{TestHelpers.TestOwnerNamePartial}456", true,
+        [TestCase($"# {MonikerConstants.ServiceOwners}: @{TestHelpers.TestOwnerNamePartial}456", true, false,
             $"{TestHelpers.TestOwnerNamePartial}456{ErrorMessageConstants.InvalidUserPartial}")]
         // Non-public member
-        [TestCase($"# {MonikerConstants.ServiceOwners}: @{TestHelpers.TestOwnerNamePartial}1", true,
+        [TestCase($"# {MonikerConstants.ServiceOwners}: @{TestHelpers.TestOwnerNamePartial}1", true, false,
             $"{TestHelpers.TestOwnerNamePartial}1{ErrorMessageConstants.NotAPublicMemberOfAzurePartial}")]
         // Malformed team entry (missing @Azure/) but team otherwise exists in the azure-sdk-write dictionary
-        [TestCase($"/sdk/SomePath @{TestHelpers.TestTeamNamePartial}0", true,
+        [TestCase($"/sdk/SomePath @{TestHelpers.TestTeamNamePartial}0", true, true,
             $"{TestHelpers.TestTeamNamePartial}0{ErrorMessageConstants.MalformedTeamEntryPartial}")]
         // All the owners errors on a single line (except no owners errors)
         [TestCase($"/sdk/SomePath @{TestHelpers.TestTeamNamePartial}0\t@{TestHelpers.TestOwnerNamePartial}1  @{TestHelpers.TestOwnerNamePartial}456\t\t\t@{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}12", 
+            true,
             true,
             $"{TestHelpers.TestTeamNamePartial}0{ErrorMessageConstants.MalformedTeamEntryPartial}",
             $"{TestHelpers.TestOwnerNamePartial}1{ErrorMessageConstants.NotAPublicMemberOfAzurePartial}",
             $"{TestHelpers.TestOwnerNamePartial}456{ErrorMessageConstants.InvalidUserPartial}",
             $"{OrgConstants.Azure}/{TestHelpers.TestTeamNamePartial}12{ErrorMessageConstants.InvalidTeamPartial}")]
-        public void TestVerifyOwners(string line, bool expectOwners, params string[] expectedErrorMessages)
+        public void TestVerifyOwners(string line, bool expectOwners, bool isSourcePathOwnerLine, params string[] expectedErrorMessages)
         {
             // Convert the array to List
             var expectedErrorList = expectedErrorMessages.ToList();
             List<string> actualErrorList = new List<string>();
-            Owners.VerifyOwners(_ownerDataUtils, line, expectOwners, actualErrorList);
+            Owners.VerifyOwners(_ownerDataUtils, line, isSourcePathOwnerLine, expectOwners, actualErrorList);
             if (!TestHelpers.StringListsAreEqual(actualErrorList, expectedErrorList))
             {
                 string expectedErrors = "Empty List";

diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Constants/ErrorMessageConstants.cs b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Constants/ErrorMessageConstants.cs
@@ -23,6 +23,7 @@ public class ErrorMessageConstants
         public const string InvalidTeamPartial = " is an invalid team. Ensure the team exists and has write permissions.";
         public const string InvalidUserPartial = " is an invalid user. Ensure the user exists, is public member of Azure and has write permissions.";
         public const string MalformedTeamEntryPartial = " is a malformed team entry and should start with '@Azure/'.";
+        public const string PathEntryMissingOwners = "Path entry, {0}, is missing owners";
         public const string NoOwnersDefined = "There are no owners defined for CODEOWNERS entry.";
         public const string NotAPublicMemberOfAzurePartial = " is not a public member of Azure.";
 

diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/CodeownersLinter.cs b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/CodeownersLinter.cs
@@ -288,7 +288,7 @@ public static void VerifySingleLine(DirectoryUtils directoryUtils,
             {
                 // Verify the source path and owners
                 directoryUtils.VerifySourcePathEntry(line, errorStrings);
-                Owners.VerifyOwners(ownerData, line, true, errorStrings);
+                Owners.VerifyOwners(ownerData, line, isSourcePathOwnerLine, true, errorStrings);
             }
             else
             {
@@ -307,7 +307,7 @@ public static void VerifySingleLine(DirectoryUtils directoryUtils,
                         case MonikerConstants.MissingFolder:
                         case MonikerConstants.ServiceOwners:
                         case MonikerConstants.AzureSdkOwners:
-                            Owners.VerifyOwners(ownerData, line, expectOwnersIfMoniker, errorStrings);
+                            Owners.VerifyOwners(ownerData, line, isSourcePathOwnerLine, expectOwnersIfMoniker, errorStrings);
                             break;
                         default:
                             // This shouldn't get here unless someone adds a new moniker and forgets to add it to the switch statement

diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/Owners.cs b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/Owners.cs
@@ -21,7 +21,7 @@ public static class Owners
         /// <param name="line">The CODEOWNERS line being parsed</param>
         /// <param name="expectOwners">Whether or not owners are expected. Some monikers may or may not have owners if their block ends in a source path/owner line.</param>
         /// <param name="errorStrings">List of errors belonging to the current line. New errors are added to the list.</param>
-        public static void VerifyOwners(OwnerDataUtils ownerData, string line, bool expectOwners, List<string> errorStrings)
+        public static void VerifyOwners(OwnerDataUtils ownerData, string line, bool isSourcePathOwnerLine, bool expectOwners, List<string> errorStrings)
         {
             List<string> ownerList = ParsingUtils.ParseOwnersFromLine(ownerData, line, false /* teams aren't expanded for linting */);
             // Some CODEOWNERS lines require owners to be on the line, like source path/owners lines. Some CODEOWNERS
@@ -32,7 +32,14 @@ public static void VerifyOwners(OwnerDataUtils ownerData, string line, bool expe
             {
                 if (expectOwners)
                 {
-                    errorStrings.Add(ErrorMessageConstants.NoOwnersDefined);
+                    if (isSourcePathOwnerLine)
+                    {
+                        errorStrings.Add(string.Format(ErrorMessageConstants.PathEntryMissingOwners, line.Trim()));
+                    }
+                    else
+                    {
+                        errorStrings.Add(ErrorMessageConstants.NoOwnersDefined);
+                    }
                 }
                 return;
             }