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 <[email protected]>

---------

Co-authored-by: Wes Haggard <[email protected]>

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

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,

tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/CodeownersTestFiles/VerifyBlock/AzureSdkOwnersAndServiceLabel

@@ -8,4 +8,3 @@
 # AzureSdkOwners: @TestOwnerBad
 
 # AzureSdkOwners:
-

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

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,

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.";

tools/codeowners-utils/Azure.Sdk.Tools.CodeownersUtils/Parsing/CodeownersParser.cs

@@ -94,7 +94,7 @@ public static CodeownersEntry ParseCodeownersEntryFromBlock(OwnerDataUtils owner
                                                                     List<string> 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

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 #/<NotInRepo>/ (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 #/<NotInRepo>/
+            // (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);
                 }

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
+
+```