From d24687e88f2c9acb3d43522df701c4ae638a6037 Mon Sep 17 00:00:00 2001 From: James Suplizio Date: Tue, 27 Feb 2024 06:10:08 -0800 Subject: [PATCH] Change the way ServiceLabel parses with AzureSdkOwners (#7754) * Change the way ServiceLabel parses with AzureSdkOwners * Update tools/codeowners-utils/METADATA.md Co-authored-by: Wes Haggard --------- Co-authored-by: Wes Haggard --- .../CodeownersTestFiles/EndToEnd/NoErrors | 4 +++ .../EndToEnd/NoErrorsExpectedEntries.json | 15 +++++++++++ .../VerifyBlock/AzureSdkOwnersAndServiceLabel | 1 - .../VerifyBlock/ServiceLabelAndAzureSdkOwners | 6 +++++ .../Verification/CodeownersLinterTests.cs | 2 ++ .../Constants/ErrorMessageConstants.cs | 4 +-- .../Parsing/CodeownersParser.cs | 2 +- .../Verification/CodeownersLinter.cs | 6 ++--- tools/codeowners-utils/METADATA.md | 26 +++++++++++++++---- 9 files changed, 54 insertions(+), 12 deletions(-) create mode 100644 tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/ServiceLabelAndAzureSdkOwners diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrors b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrors index 93ef0fed6d4..05043945205 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrors +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrors @@ -27,6 +27,10 @@ # ServiceLabel: %TestLabel3 /sdk/someFakePath4/ @TestOwner2 @TestOwner4 +# AzureSdkOwners with only a ServiceLabel entry +# AzureSdkOwners: @TestOwner2 @TestOwner0 +# ServiceLabel: %TestLabel4 + # Every moniker that can be grouped together and end in a source path/owner line. # AzureSdkOwners: @TestOwner0 # PRLabel: %TestLabel2 diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrorsExpectedEntries.json b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrorsExpectedEntries.json index 5103586bae7..c754fde88f2 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrorsExpectedEntries.json +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/EndToEnd/NoErrorsExpectedEntries.json @@ -105,6 +105,21 @@ ], "IsValid": true }, + { + "PathExpression": "", + "ContainsWildcard": false, + "SourceOwners": [], + "PRLabels": [], + "ServiceLabels": [ + "TestLabel4" + ], + "ServiceOwners": [], + "AzureSdkOwners": [ + "TestOwner2", + "TestOwner0" + ], + "IsValid": false + }, { "PathExpression": "/sdk/someFakePath5/", "ContainsWildcard": false, diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/AzureSdkOwnersAndServiceLabel b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/AzureSdkOwnersAndServiceLabel index 568753ae04d..798da9d5f23 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/AzureSdkOwnersAndServiceLabel +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/AzureSdkOwnersAndServiceLabel @@ -8,4 +8,3 @@ # AzureSdkOwners: @TestOwnerBad # AzureSdkOwners: - diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/ServiceLabelAndAzureSdkOwners b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/ServiceLabelAndAzureSdkOwners new file mode 100644 index 00000000000..9497e1e645b --- /dev/null +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/ServiceLabelAndAzureSdkOwners @@ -0,0 +1,6 @@ +# A ServiceLabel block with only AzureSdkOwners: +# ServiceLabel: %TestLabel3 +# AzureSdkOwners: @TestOwner0 @TestOwner4 @Azure/TestTeam3 + +# AzureSdkOwners: @TestOwner2 @Azure/TestTeam2 @TestOwner4 +# ServiceLabel: %TestLabel2 diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Verification/CodeownersLinterTests.cs b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Verification/CodeownersLinterTests.cs index 854ad675621..97026952d44 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Verification/CodeownersLinterTests.cs +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Verification/CodeownersLinterTests.cs @@ -210,6 +210,8 @@ public void TestVerifySingleLine(string line, [TestCase("CodeownersTestFiles/VerifyBlock/ServiceLabelAndSourcePath", 1, 2)] [TestCase("CodeownersTestFiles/VerifyBlock/ServiceLabelAndMissingPath", 1, 2)] [TestCase("CodeownersTestFiles/VerifyBlock/ServiceLabelAndServiceOwners", 1, 2)] + [TestCase("CodeownersTestFiles/VerifyBlock/ServiceLabelAndAzureSdkOwners", 1, 2)] + [TestCase("CodeownersTestFiles/VerifyBlock/ServiceLabelAndAzureSdkOwners", 4, 5)] [TestCase("CodeownersTestFiles/VerifyBlock/MonikersEndsInSourcePath", 1, 4)] // AzureSdkOwners needs to be part of a block that contains ServiceLabel [TestCase("CodeownersTestFiles/VerifyBlock/AzureSdkOwnersAndServiceLabel", 3, 3, 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 dc574deafa5..8ebf905a052 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Constants/ErrorMessageConstants.cs +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Constants/ErrorMessageConstants.cs @@ -51,9 +51,9 @@ public class ErrorMessageConstants // Codeowners glob is syntactically but doesn't match anything public const string GlobHasNoMatchesInRepoPartial = " glob does not have any matches in repository."; - // Block formatting errors. These errors are specifically around validation blocks. For example, the AzureSdkOwner moniker needs + // Block formatting errors. These errors are specifically around validation blocks. For example, the ServiceOwner moniker needs // to part of a block that ends in a source path/owners line so it's known what they own. - public const string ServiceLabelNeedsOwners = $"{MonikerConstants.ServiceLabel} needs to be followed by, {MonikerConstants.MissingFolder} or {MonikerConstants.ServiceOwners} with owners, or a source path/owner line."; + public const string ServiceLabelNeedsOwners = $"{MonikerConstants.ServiceLabel} needs to be part of a block containing {MonikerConstants.AzureSdkOwners} or followed by, {MonikerConstants.MissingFolder} or {MonikerConstants.ServiceOwners} with owners, or part of a block that ends in a source path/owner line."; public const string ServiceLabelHasTooManyOwners = $"{MonikerConstants.ServiceLabel} cannot be part of a block with, {MonikerConstants.MissingFolder} or {MonikerConstants.ServiceOwners}, and a source path/owner line."; public const string ServiceLabelHasTooManyOwnerMonikers = $"{MonikerConstants.ServiceLabel} cannot be part of a block with both {MonikerConstants.ServiceOwners} and {MonikerConstants.MissingFolder}."; public const string MissingServiceLabelPartial = $" needs to be part of a block with a {MonikerConstants.ServiceLabel} entry."; diff --git a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Parsing/CodeownersParser.cs b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Parsing/CodeownersParser.cs index b1bbbde92a4..eee8d09d030 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Parsing/CodeownersParser.cs +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Parsing/CodeownersParser.cs @@ -94,7 +94,7 @@ public static CodeownersEntry ParseCodeownersEntryFromBlock(OwnerDataUtils owner List codeownersFile) { CodeownersEntry codeownersEntry = new CodeownersEntry(); - // If the block ends with a source path/owner line then any owner moniker line that are empty + // If the block ends with a source path/owner line then any owner moniker lines that are empty // will be set to the same list as the source owners. bool endsWithSourceOwnerLine = ParsingUtils.IsSourcePathOwnerLine(codeownersFile[endBlockLineNumber]); // These are used in the case where the AzureSdkOwners and/or ServiceOwners are empty and part of a 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 55d1acb75c2..4bba1c36f39 100644 --- a/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/CodeownersLinter.cs +++ b/tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Verification/CodeownersLinter.cs @@ -234,11 +234,11 @@ public static void VerifyBlock(DirectoryUtils directoryUtils, blockErrorStrings.Add($"{MonikerConstants.PRLabel}{ErrorMessageConstants.NeedsToEndWithSourceOwnerPartial}"); } - // ServiceLabel needs to be part of a block that has one of, ServiceOwners or #// (MonikerConstants.MissingFolder), - // or ends in a source path/owner line both not both. + // ServiceLabel needs to be part of a block that has AzureSdkOwners or one of; ServiceOwners or #// + // (MonikerConstants.MissingFolder), or ends in a source path/owner line both not both. if (blockHasServiceLabel) { - if (!endsWithSourceOwnerLine && !blockHasServiceOwners && !blockHasMissingFolder) + if (!endsWithSourceOwnerLine && !blockHasServiceOwners && !blockHasMissingFolder && !blockHasAzureSdkOwners) { blockErrorStrings.Add(ErrorMessageConstants.ServiceLabelNeedsOwners); } diff --git a/tools/codeowners-utils/METADATA.md b/tools/codeowners-utils/METADATA.md index 6b74d44b052..348052c8699 100644 --- a/tools/codeowners-utils/METADATA.md +++ b/tools/codeowners-utils/METADATA.md @@ -10,7 +10,7 @@ Azure-sdk* repository CODEOWNERS files contain data beyond the normal source pat - **AzureSdkOwners:** - This moniker is used to denote Azure Sdk owners that have triage responsibility for a given service label. This moniker must be part of a block containing a ServiceLabel entry. - **PRLabel:** - This moniker is used by workflows to determine what label(s) will get added to a pull request based upon the file paths of the files in the pull request. This moniker must be part of a block that ends in a source path/owner line. -- **ServiceLabel:** - This moniker contains the service label that's used to figure out what users need to be @ mentioned in an issue when the Service Attention label is added. This moniker must be part of a block that either ends in a source path/owner line, the ServiceOwners moniker or the /<NotInRepo>/ moniker. If the ServiceLabel is part of a block that ends in the source path/owner line, the service owners are inferred from that. +- **ServiceLabel:** - This moniker contains the service label that's used to figure out what users should be assigned at triage to own issues, whether part of the Azure SDK team or service partners. If the label is owned by a service partner or has a set of service owners included, these users would be `@` mentioned in an issue when the Service Attention label is added. This moniker must be part of a block that either ends in a source path/owner line, the AzureSdkOwners moniker, ServiceOwners moniker, or the /<NotInRepo>/ moniker. If the ServiceLabel is part of a block that ends in the source path/owner line, the service owners are inferred from that. - **ServiceOwners:** - The moniker is used to identify the owners associated with a service label if the service label isn't part of a block ending in a source path/owner line. This moniker cannot be part of a source path/owner line. - **/<NotInRepo>/** - This is the existing moniker used to denote service owners in CODEOWNERS files. This will ultimately be replaced by the ServiceOwners moniker, which more clearly defines what it actually is, but right now the parser and linter will handle both. This moniker cannot be part of a source path/owner line. Also, this is the only moniker that doesn't have a colon separator after the moniker, before the labels. @@ -27,7 +27,7 @@ This list of examples is exhaustive. If an example isn't in here then it won't w ``` -- `AzureSdkOwners` must be part of a block that contains a ServiceLabel entry. If that block ends in a source path/owner line, and the AzureSdkOwners entry is empty, it'll have the same owners that are only the source path/owner line. If it's part of block that contains a ServiceLabel/ServiceOwner combination, then it must have it's own owners defined. +- `AzureSdkOwners` must be part of a block that contains a ServiceLabel entry. If that block ends in a source path/owner line, and the AzureSdkOwners entry is empty, it'll have the same owners that are only the source path/owner line. If it's part of block that contains ServiceLabel/AzureSdkOwners combination OR a ServiceLabel/ServiceOwner/AzureSdkOwners combination, then it must have it's own owners defined. ```text @@ -42,6 +42,9 @@ OR # AzureSdkOwners: @fakeUser3 @fakeUser4 # ServiceLabel: %fakeLabel12 # ServiceOwners: @fakeUser1 @fakeUser2 +OR +# AzureSdkOwners: @fakeUser3 @fakeUser4 +# ServiceLabel: %fakeLabel12 ``` @@ -63,7 +66,7 @@ OR ``` -- If a `ServiceLabel` is not part of a block that ends in a source path/owner line, then it must be part of a two line block consisting only of a ServiceLabel and either a ServiceOwners or /<NotInRepo>/. New entries should use ServiceOwners. +- If a `ServiceLabel` is not part of a block that ends in a source path/owner line, then it must be part of a block consisting only of a ServiceLabel and one or both of the following; AzureSdkOwners, ServiceOwners or /<NotInRepo>/. New entries should use ServiceOwners. ```text @@ -72,10 +75,16 @@ OR OR # ServiceLabel: %Label1 %Label2 # /<NotInRepo>/ @fakeUser1 @Azure/fakeTeam1 - +OR +# AzureSdkOwners: @fakeUser1 @Azure/fakeTeam1 +# ServiceLabel: %Label1 %Label2 +OR +# AzureSdkOwners: @fakeUser1 @Azure/fakeTeam1 +# ServiceLabel: %Label1 %Label2 +# ServiceOwners: @fakeUser1 @Azure/fakeTeam1 ``` -- This might look complex but there are really only 2 types of blocks. The first ends with a source path/owner line and may have AzureSdkOwners, ServiceLabel and PRLabel entries. The second is ServiceLabel/ServiceOwner block which may have AzureSdkOwners. +- This might look complex but there are really only 3 types of blocks. The first ends with a source path/owner line and may have AzureSdkOwners, ServiceLabel and PRLabel entries. The second is a ServiceLabel/ServiceOwner block which may have AzureSdkOwners. The third is an AzureSdkOwner/ServiceLabel block. ```text @@ -93,3 +102,10 @@ OR # ServiceOwners: @fakeUser1 @Azure/fakeTeam1 ``` + +```text + +# AzureSdkOwners: @fakeUser1 @Azure/fakeTeam1 +# ServiceLabel: %Label1 %Label2 + +```