Add support for wildcard handling of file matching in codeowners library

[weshaggard]

See comment Azure/azure-sdk-for-js#20354 (comment).

It would be helpful to support some basic wildcard matching in our code owners file library. Maybe we cannot easily support all wildcard patterns but we could probably support ** for something like /sdk/**/ci.mgmt.yml.

[konrad-jamrozik]

@weshaggard I analyzed the relevant code and here is my plan of action. Sharing with you in case you have any concerns or suggestions. Also CC @sima-zhu as I believe Sima has recent experience working with the relevant code :)

What code needs to change

Principally, there are two places that need to be modified:

1. CodeOwnersFile.FindOwnersForClosestMatch:

// We want to find the match closest to the bottom of the codeowners file.
// CODEOWNERS sorts the paths in order of 'RepoPath', 'ServicePath' and then 'PackagePath'.
for (int i = codeOwnerEntries.Count - 1; i >= 0; i--)
{
    string codeOwnerPath = codeOwnerEntries[i].PathExpression.Trim('/');
    // Note that this only matches on paths without glob patterns which is good enough
    // for our current scenarios but in the future might need to support globs
    if (targetPath.StartsWith(codeOwnerPath, StringComparison.OrdinalIgnoreCase))
    {
        return codeOwnerEntries[i];
    }
}

2. PullRequestLabelFolderCapability.ReadConfigurationFromFile:

// Entries with wildcards are not yet supported
if (entries[i].ContainsWildcard)
{
    // log a warning there

    if (entries[i].PRLabels.Any())
    {
        Colorizer.WriteLine("[Yellow!Warning]: The path '[Cyan!{0}]' contains a wildcard and a label '[Magenta!{1}]' which is not supported!", entries[i].PathExpression, string.Join(',', entries[i].PRLabels));
    }

    continue; //TODO: regex expressions are not yet supported
}

Accepted modifications proposal, Dec 28, 2022

Per the follow-up discussion seen in comments below, we care only about case 1, not 2. Case 1 is processing input paths one by one, and looking for a first match in CODEOWNERS file. Hence all we need to do is to ensure that if a candidate path from CODEOWNERS contains * or **, we correctly interpret such path matching against the input path. For details, see this comment below.

Rejected modifications proposal, Dec 5, 2022

This proposal was rejected in favor of proposal from 12/28/2022. This rejected proposal would have required expanding each wildcard path against all paths in repo that match it, which would be a lot of work only to then later discard almost all of it, and would require reading the repository structure while parsing CODEOWNERS file. The accepted proposal from 12/27 doesn't have this issues.

In the two places that require modification listed above, I will make a check for CodeOwnerEntry.ContainsWildcard. If it is true, I will call a newly introduced method, like CodeOwnerEntry.ExpandWildcard that will return a List of CodeOwnerEntry, after expanding the wildcards.

Such expanded non-wildcard entries will be matched reverse-alphabetically, to retain the property of entries being matched bottom-to-top [1], as defined in CODEOWNERS. Here I am making an implicit assumption that wildcard means all the entries represented by wildcard would have been listed alphabetically in the CODEOWNERS file. If the author wants different order of matching, they need to list the entries explicitly.

Expanding the wildcards will require for the logic to be able to access the contents of the repository in which the CODEOWNERS file is located, to pattern-match the file paths against the wildcards. Having a handle to that file only will no longer be enough.

Because of that, as a design alternative, I am considering recomputing the list of all paths represented by the wildcard. I.e. I will compute them as the CODEOWNERS file is parsed. This means that if a CodeOwnerEntry.ContainsWildcard will be true, the entry newly-added field, like CodeOwnerEntry.ExpandedEntries, will represent all the expanded entries. The CodeOwnersFile.GetContents method will return a List of CodeOwnerEntry instances with these expanded entries already precomputed, instead of doing the expansion just-in-time by CodeOwnerEntry.ExpandWildcard.

Footnotes

[1] per this comment:

// We want to find the match closest to the bottom of the codeowners file.

and our CODEOWNERS documentation:

Place more general rules higher in the file and more specific rules lower in the file as GitHub uses the last matching expression

Which patterns I will support

The GitHub doc on CODEOWNERS syntax states:

A CODEOWNERS file uses a pattern that follows most of the same rules used in gitignore files.

Per this issue description, only this pattern has to be supported, as explained in the gitignore doc:

A slash followed by two consecutive asterisks then a slash matches zero or more directories. For example, "a/**/b" matches "a/b", "a/x/b", "a/x/y/b" and so on.

However, if it will turn out that ROI on supporting other patterns too will be good, I will do so too.

Why do I think these are the places to modify, and no other?

The parsing logic lives in CodeOwnersFile.ParseContent. It returns a value of type List<CodeOwnerEntry>. Each CODEOWNERS entry is parsed into an aptly named class CodeOwnerEntry.

CodeOwnerEntry CODEOWNERS file line parsing logic already supports reading paths with wildcards into CodeOwnerEntry.PathExpression, as seen in CodeOwnerEntry.ParsePath, which is called from CodeOwnersFile.ParseOwnersAndPath, which is called from CodeOwnersFile.ParseContent.

This means that any limitations of supporting wildcards need to be present downstream of a call to CodeOwnersFile.ParseContent and hence I analyzed where the value output of this method flows to.

I made the analysis by loading in VS Code the entire repository and doing Find all references on CodeOwnersFile.ParseContent, and also by using GitHub web code browser navigation.

In addition, I also looked at references to CodeOwnerEntry.PathExpression and CodeOwnerEntry.ContainsWildcard.

Here are all the use cases I have identified:

Use case 1: The code-owners-parser

CodeOwnersFile.ParseContent is called by
CodeOwnersFile.ParseFile which is called by
CodeOwnersFile.ParseAndFindOwnersForClosestMatch which is called by
Azure.Sdk.Tools.RetrieveCodeOwners.Program.Main
which, in turn, ends up taking one CodeOwnerEntry (not a list, just one), serializing it to JSON, and outputting it to STDOUT, as seen in the Program.cs, here:

var codeOwnerJson = JsonSerializer.Serialize<CodeOwnerEntry>(codeOwnerEntry, new JsonSerializerOptions { WriteIndented = true });
Console.WriteLine(codeOwnerJson);

This tool is also run via the pipeline azure-sdk / internal / tools / tools - code-owners-parser.

This pipeline can be used to publish a NuGet package, e.g.:

Pushing Azure.Sdk.Tools.RetrieveCodeOwners.1.0.0-dev.20221205.3.nupkg to 'https://pkgs.dev.azure.com/azure-sdk/29ec6040-b234-4e31-b139-33dc4287b756/_packaging/f9f4dcc6-8e73-7147-a0d8-8c14278093ff/nuget/v2/'...
  PUT https://pkgs.dev.azure.com/azure-sdk/29ec6040-b234-4e31-b139-33dc4287b756/_packaging/f9f4dcc6-8e73-7147-a0d8-8c14278093ff/nuget/v2/

The call to ParseAndFindOwnersForClosestMatch does the following:

public static CodeOwnerEntry ParseAndFindOwnersForClosestMatch(string codeOwnersFilePathOrUrl, string targetPath)
{
    var codeOwnerEntries = ParseFile(codeOwnersFilePathOrUrl);
    return FindOwnersForClosestMatch(codeOwnerEntries, targetPath);
}

where FindOwnersForClosestMatch has the first of two places that require modification, as mentioned in the beginning of this post. This method is also public, so it may be depended upon by the consumers of the NuGet package.

Use case 2: notification-configuration

CodeOwnersFile.ParseContent is called by
CodeOwnersFile.ParseFile which is called by
GitHubService.GetCodeownersFileImpl which is called by
GitHubService.GetCodeownersFile which is called by
NotificationConfigurator.SyncTeamWithCodeOwnerFile

This method then ends up calling later in the same method the CodeOwnersFile.FindOwnersForClosestMatch, for each processed input build definition pipeline. The execution of this tool is triggered by the pipeline internal / automation /
automation - build-failure-notification-subscriptions. It processes the pipeline to hook-up ADO email configurations on build failure. I elaborated on that in #4859 (comment)

Use case 3: pipeline-owners-extraction

CodeOwnersFile.ParseContent is called by
CodeOwnersFile.ParseFile which is called by
GitHubService.GetCodeownersFileImpl which is called by
GitHubService.GetCodeownersFile which is called by
PipelineOwnersExtractor.Processor.GetCodeOwnerEntriesAsync which is called by
PipelineOwnersExtractor.Processor.ExecuteAsync.

The data returned to ExecuteAsync then flows to AssociateOwnersToPipelinesAsync which invokes CodeOwnersFile.FindOwnersForClosestMatch from this line.

Cursory analysis tells me the situation is similar to use case 2, i.e. each pipeline is being used as input to return relevant CodeOwnersEntry. The relevant pipeline triggering it is internal / automation / automation - pipeline-owners-extraction.

Use case 4: FabricBot - PR labels

CodeOwnersFile.ParseContent is called by
CodeOwnersFile.ParseFile which is called by
CreateRuleFabricBot.Rules.PullRequestLabel.PullRequestLabelFolderCapability.ReadConfigurationFromFile

which is the second code location that needs to be modified.

The FabricBot NuGet package is published by the pipeline internal / tools / tools - ConfigureFabricBot. One can then execute FabricBot according to its README, setting taskType to PullRequestLabel to trigger this functionality.

Use case 5: FabricBot - service routing labels

CodeOwnersFile.ParseContent is called by
CodeOwnersFile.ParseFile which is called by
CreateRuleFabricBot.Rules.IssueRouting.IssueRoutingCapability.ReadConfigurationFromFile which then doesn't do anything related to wildcard matching.

Behavior change of existing CODEOWNERS paths with wildcards

Section added on 12/28/2022

Observe that once we add wildcard support in the CodeOwnersFile.FindOwnersForClosestMatch, our parser will start matching against the existing, currently ignored, wildcard paths in our CODEOWNERS files in our language repos. For example, in azure-sdk-for-net CODEOWNERS we can see close to the bottom of the file (and hence parsed first):

# ######## Eng Sys ######## 
/eng/           @hallipr @weshaggard @benbp
/eng/mgmt/      @ArthurMa1978 @m-nash @markcowl
/**/tests.yml   @hallipr @benbp
/**/ci.yml      @hallipr @benbp

This means that these paths:

/**/tests.yml   @hallipr @benbp
/**/ci.yml      @hallipr @benbp

will start matching. This needs to be accounted for and possibly adjusted before the wildcard support comes into effect.

[weshaggard]

Expanding the wildcards will require for the logic to be able to access the contents of the repository in which the CODEOWNERS file is located, to pattern-match the file paths against the wildcards. Having a handle to that file only will no longer be enough.

What do you mean by expand here? Do you plan to use some sort of regex patterns to replace the gitignore patterns so you can do a match? That is sort of what I had in mind for implementing this.

Other than the one clarification the other logic and searching seems correct. Thanks for the detailed investigation.

[konrad-jamrozik]

What do you mean by expand here? Do you plan to use some sort of regex patterns to replace the gitignore patterns so you can do a match? That is sort of what I had in mind for implementing this.

Other than the one clarification the other logic and searching seems correct. Thanks for the detailed investigation.

@weshaggard let me summarize our in-person follow-up discussion here. I also discovered another limitation, and I would appreciate your input.

We observed that the usage in CodeOwnersFile.FindOwnersForClosestMatch is matching specific file names against the CODEOWNERS entry, so indeed changing the logic from "does this fileName start with this codeownersEntry path" to "does this fileName match the codeowners path-with-wildcards regex" should cover all the cases we care about, which are all the cases that involve * or ** but not ! or ?.

However!

The second use case is FabricBot, and there is a problem with it, as it does not support wildcards. The CODEOWNERS path entry gets converted to FabricBot's pathFilter. For example, azure-sdk-net-for-net/CODEOWNERS has this entry:

# PRLabel: %Azure.Core
/sdk/core/                                      @JoshLove-msft @christothes @annelo-msft @KrzysztofCwalina

which ends up being converted to this pathFilter in fabricbot.json:

"label": "Azure.Core",
"pathFilter": [
  "sdk/core/"
],

Our relevant code doing this is in CreateRuleFabricBot/Rules/PullRequestLabel/PathConfig.cs and around it, in PullRequestLabelFolderCapability class.

I believe the FabricBot logic matching paths against pathFilter is this (Microsoft-internal):

/**
 * Checks whether the given file name matches any of the given set of path filters.
 * @param fileName File name to check.
 * @param pathFilters Optional set of path filters.
 */
export function doesFileNameMatchPathFilters(fileName: string, pathFilters?: string[]): boolean {
  if (!pathFilters) {
    return false;
  }

  for (const pathFilter of pathFilters) {
    if (fileName.startsWith(pathFilter)) {
      return true;
    }
  }

  return false;
}

as it appears to be called from here (Microsoft-internal).

As you can see, this logic is doing plain fileName.startsWith(pathFilter).

I see two options going forward:

1. Clarify that paths in CODEOWNERS that have wildcards on them cannot have # PRLabel: comments. This could be enforced by the upcoming validation:

• Implement CODEOWNERS file changes validation (gating), with baselining #4859

3. Reach out to FabricBot maintainers and ask them if it would be feasible for us to implement support of wildcards into this method. I know who to reach out to.

Given that we plan to move away from FabricBot anyway, per:

• Implement existing fabricbot rules using github actions. #4478
• Delete CreateRuleFabricBot tool #4974

I am leaning towards option no 1.

@weshaggard let me know what you think. Also CC @JimSuplizio as possibly relevant to his work.

[weshaggard]

I agree that for now we can ignore fabricbot and any wildcard paths will just not work there. Once @JimSuplizio does his work to move to github actions for this work he will be using our library here so any file path match should work.

[JimSuplizio]

@konrad-jamrozik , Ignore the FabricBot work, right now they have what they have hard coded inside of the repository specific FabricBot.json files. I need similar things but what I'd ended up doing was including the code-owners-parser to my project and writing some utils. We should, however, have a chat about how I'm using it.

[konrad-jamrozik]

This functionality has been enabled as of:

• Update NotificationConfiguration NuGet package version to 1.0.0-dev.20230216.1 to enable CODEOWNERS regex matcher everywhere #5488