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 index b32ca4e39e4..cb11ecaf30e 100644 --- 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 actualErrorList = new List(); - 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 index 8ebf905a052..93121195369 100644 --- 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 index 4bba1c36f39..95ff978f7e8 100644 --- 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 index fd339477364..c3e49c816da 100644 --- 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 /// The CODEOWNERS line being parsed /// Whether or not owners are expected. Some monikers may or may not have owners if their block ends in a source path/owner line. /// List of errors belonging to the current line. New errors are added to the list. - public static void VerifyOwners(OwnerDataUtils ownerData, string line, bool expectOwners, List errorStrings) + public static void VerifyOwners(OwnerDataUtils ownerData, string line, bool isSourcePathOwnerLine, bool expectOwners, List errorStrings) { List 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; }