Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the way ServiceLabel parses with AzureSdkOwners #7754

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,21 @@
],
"IsValid": true
},
{
"PathExpression": "",
"ContainsWildcard": false,
"SourceOwners": [],
"PRLabels": [],
"ServiceLabels": [
"TestLabel4"
],
"ServiceOwners": [],
"AzureSdkOwners": [
"TestOwner2",
"TestOwner0"
],
"IsValid": false
},
{
"PathExpression": "/sdk/someFakePath5/",
"ContainsWildcard": false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,3 @@
# AzureSdkOwners: @TestOwnerBad

# AzureSdkOwners:

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# A ServiceLabel block with only AzureSdkOwners:
# ServiceLabel: %TestLabel3
# AzureSdkOwners: @TestOwner0 @TestOwner4 @Azure/TestTeam3

# AzureSdkOwners: @TestOwner2 @Azure/TestTeam2 @TestOwner4
# ServiceLabel: %TestLabel2
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
26 changes: 21 additions & 5 deletions tools/codeowners-utils/METADATA.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 /&lt;NotInRepo&gt;/ 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 /&lt;NotInRepo&gt;/ moniker. If the ServiceLabel is part of a block that ends in the source path/owner line, the service owners are inferred from that.
JimSuplizio marked this conversation as resolved.
Show resolved Hide resolved
- **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.
- **/&lt;NotInRepo&gt;/** - 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.

Expand All @@ -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

Expand All @@ -42,6 +42,9 @@ OR
# AzureSdkOwners: @fakeUser3 @fakeUser4
# ServiceLabel: %fakeLabel12
# ServiceOwners: @fakeUser1 @fakeUser2
OR
# AzureSdkOwners: @fakeUser3 @fakeUser4
# ServiceLabel: %fakeLabel12

```

Expand All @@ -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 /&lt;NotInRepo&gt;/. 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 /&lt;NotInRepo&gt;/. New entries should use ServiceOwners.

```text

Expand All @@ -72,10 +75,16 @@ OR
OR
# ServiceLabel: %Label1 %Label2
# /&lt;NotInRepo&gt;/ @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

Expand All @@ -93,3 +102,10 @@ OR
# ServiceOwners: @fakeUser1 @Azure/fakeTeam1

```

```text

# AzureSdkOwners: @fakeUser1 @Azure/fakeTeam1
# ServiceLabel: %Label1 %Label2

```