From a29f5518b9c7340debbb2e746791f8397c4aa62c Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Fri, 14 Jun 2024 10:00:38 -0700 Subject: [PATCH 1/3] Fix missing owners for path error to include path --- .../Verification/OwnersTests.cs | 26 ++++++++++--------- .../Constants/ErrorMessageConstants.cs | 2 ++ .../Verification/CodeownersLinter.cs | 4 +-- .../Verification/Owners.cs | 11 ++++++-- 4 files changed, 27 insertions(+), 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 index b32ca4e39e4..cee245ffbc6 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,45 @@ 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)] + // JRS - need to change this error + [TestCase($"/sdk/SomePath", true, true, $"{ErrorMessageConstants.PathEntryMissingOwnersPartialStart}/sdk/SomePath{ErrorMessageConstants.PathEntryMissingOwnersPartialEnd}")] // 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..cbed62303d9 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,8 @@ 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 PathEntryMissingOwnersPartialStart = "Path entry, "; + public const string PathEntryMissingOwnersPartialEnd = ", 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..d52009daa02 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($"{ErrorMessageConstants.PathEntryMissingOwnersPartialStart}{line.Trim()}{ErrorMessageConstants.PathEntryMissingOwnersPartialEnd}"); + } + else + { + errorStrings.Add(ErrorMessageConstants.NoOwnersDefined); + } } return; } From 9966ecfe3e17be9252bbb1ece537657ac53440d5 Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Fri, 14 Jun 2024 10:30:27 -0700 Subject: [PATCH 2/3] update for feedback --- .../Verification/OwnersTests.cs | 6 ++++-- .../Constants/ErrorMessageConstants.cs | 1 + .../Azure.Sdk.Tools.CodeownersUtils/Verification/Owners.cs | 2 +- 3 files changed, 6 insertions(+), 3 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 index cee245ffbc6..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 @@ -43,8 +43,10 @@ public void InitRepoLabelData() // AzureSdkOwners, with no owner defined is legal for a block that ends in a source path/owner line. [TestCase($"# {MonikerConstants.AzureSdkOwners}:", false, false)] // Source path/owner line with no owners should complain - // JRS - need to change this error - [TestCase($"/sdk/SomePath", true, true, $"{ErrorMessageConstants.PathEntryMissingOwnersPartialStart}/sdk/SomePath{ErrorMessageConstants.PathEntryMissingOwnersPartialEnd}")] + // 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, false, ErrorMessageConstants.NoOwnersDefined)] // At this point whether or not the line is a moniker or source path/owner line is irrelevant. 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 cbed62303d9..bf36093d625 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 PathEntryMissingOwnersPartialStart = "Path entry, "; public const string PathEntryMissingOwnersPartialEnd = ", is missing owners"; public const string NoOwnersDefined = "There are no owners defined for CODEOWNERS entry."; 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 d52009daa02..c3e49c816da 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/Owners.cs +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/Owners.cs @@ -34,7 +34,7 @@ public static void VerifyOwners(OwnerDataUtils ownerData, string line, bool isSo { if (isSourcePathOwnerLine) { - errorStrings.Add($"{ErrorMessageConstants.PathEntryMissingOwnersPartialStart}{line.Trim()}{ErrorMessageConstants.PathEntryMissingOwnersPartialEnd}"); + errorStrings.Add(string.Format(ErrorMessageConstants.PathEntryMissingOwners, line.Trim())); } else { From 2b35d789b51c2eb9d3fa5e300727d92e909838ec Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Fri, 14 Jun 2024 10:31:22 -0700 Subject: [PATCH 3/3] update for feedback --- .../Constants/ErrorMessageConstants.cs | 2 -- 1 file changed, 2 deletions(-) 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 bf36093d625..93121195369 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Constants/ErrorMessageConstants.cs +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Constants/ErrorMessageConstants.cs @@ -24,8 +24,6 @@ public class ErrorMessageConstants 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 PathEntryMissingOwnersPartialStart = "Path entry, "; - public const string PathEntryMissingOwnersPartialEnd = ", 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.";