Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher

[konrad-jamrozik]

This PR enables the parser introduced in:

• Replace the glob-based CODEOWNERS parser with regex-based #5063

And as such, this PR contributes to:

• Add support for wildcard handling of file matching in codeowners library #2770

Before a package published out of this PR is depended upon, a review of affected CODEOWNERS files is necessary, as explained in Behavior change of existing CODEOWNERS paths with wildcards section of this comment:

• Add support for wildcard handling of file matching in codeowners library #2770 (comment)

Full behavior difference between the old and new parser can be determined by comparing the two rightmost columns of this test battery.

This parser will be executed by the automation - build-failure-notification-subscriptions pipeline, once the package it depends on is published and updated, similar as it is done in this PR:

• Update versions of packages RetrieveCodeOwners, NotificationConfiguration and PipelineOwnersExtractor to most recent as of 1/8/2023 #5085

As a result, the following CODEOWNERS files are expected to be affected, and thus require a review:

• https://github.com/Azure/azure-dev/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-c/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-cpp/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-go/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-java/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-js/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-net/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-python/blob/main/.github/CODEOWNERS

Following files won't be affected, as they are not triggered by the pipeline, whose source is notifications.yml:

• https://github.com/Azure/azure-sdk-for-android/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-for-ios/blob/main/.github/CODEOWNERS
• https://github.com/Azure/azure-sdk-tools/blob/main/.github/CODEOWNERS

In addition, following work needs to be implemented and applied to the CODEOWNERS files before we can enable support for wildcards:

• Add support for sending build failure notifications to different CODEOWNERS owners than the origin build definition owners. #5181

Related work

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

[konrad-jamrozik]

@weshaggard as I mentioned in this PR description - a review of the listed CODEOWNERS files is necessary, to see how support of wildcards changes the owners. Could you provide me with guidance what algorithm I should apply here?

Should I reach out first to all the folks who will now start getting ADO notifications for build failures because of wildcards now matching, and ask them if they want to opt out before I deploy this change?

Or maybe I should comment out all wildcard-supporting paths and ask the assigned owners to let me know if I should uncomment them?

Due to wildcards now matching possibly some set of people will stop getting notifications, but I think figuring this set out would be a lot of work, so I am not sure if I should try to do it. I would have to check if each wildcard path would match against any path listed above it, and if so, if there is at least one file matching both these paths, and if so, if the owners are different, and if so, if the owner change is in fact a problem.

Thanks!

[weshaggard]

My biggest concern is the **/ci.yml and **/test.yml patterns that are setup at the bottom of the files which point to engsys folks. That is going to cause all pipeline notifications to come to those folks instead of the service teams which is not what we want to happen. I suggest we simply remove those patterns from the Codeowners files.

I would suggest doing a test where you look at the owners of all the *.yml files in the repo before and after your changes and compare the results to see what changed.

Might consider that for any other patterns you see in the codeowners where ** patterns exist.

[weshaggard]

I don't want this merged until we verify the yml file mappings as I believe this will break all our notification mappings.

[konrad-jamrozik]

My biggest concern is the **/ci.yml and **/test.yml patterns that are setup at the bottom of the files which point to engsys folks. That is going to cause all pipeline notifications to come to those folks instead of the service teams which is not what we want to happen. I suggest we simply remove those patterns from the Codeowners files.

I would suggest doing a test where you look at the owners of all the *.yml files in the repo before and after your changes and compare the results to see what changed.

Might consider that for any other patterns you see in the codeowners where ** patterns exist.

@weshaggard OK, cool. I am going to make all the tests as you proposed.

My plan is to augment the retrieve-codeowners tool (which is wrapped by get-codeowners.ps1 script) to allow it to not only target specific directory to check its ownership, but to be able to target a path with wildcards, find all concrete paths matching that path-with-wildcards, and return ownership information for all of them.

I will then run this tool with the old and new parser and report here the ownership diff here.

[konrad-jamrozik]

I have confirmed that commenting out the following lines in CODEOWNERS for azure-sdk-for-net:

#/**/*Management*/                               @fengzhou-msft @archerzz @ArcturusZhang @Yao725 @ArthurMa1978
#/**/Azure.ResourceManager*/                     @fengzhou-msft @archerzz @ArcturusZhang @Yao725 @ArthurMa1978

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

results in exactly the same owners being matched using the new regex matcher as the obsolete prefix-based matcher.

I'll add more details here and link to the tool I used a bit later.

[weshaggard]

I think it is fine to remove the **/*.yml entries for engsys but we should talk with the mgmt folks about those before we remove them. We could also consider moving some of these wildcard entries to the top of the file so they will still catch anything missed but allow the more specific paths to override.

[konrad-jamrozik]

I think it is fine to remove the **/*.yml entries for engsys but we should talk with the mgmt folks about those before we remove them. We could also consider moving some of these wildcard entries to the top of the file so they will still catch anything missed but allow the more specific paths to override.

@weshaggard the diff is actually more complex, as I discovered when working on:

• Implement tools for diffing sets of owners of repo paths; update the regex-based CODEOWNERS matching logic #5165

Sorry about the confusion.

I think dealing with paths having wildcards in them is a bit tricky: if I move the rules around, it will change the owners as interpreted by GitHub itself. For the same reason I also cannot just comment-out all wildcard paths to allow graceful, gradual migration to the regex-based matcher.

Not sure yet what is the best way to approach this. I could ask the affected folks to review the owners diff I get from #5165 and ask all of them if they are OK with getting the build failure notifications now.

In the meantime I made this PR:

• Fix invalid paths in CODEOWNERS azure-sdk-for-net#33584

Here is the full diff on the azure-sdk-for-net CODEOWNERS. LEFT is current prefix-based matcher, while RIGHT is the new regex-based matcher AND including the PR 33584 changes applied. Meaning that once the new matcher is enabled, ADO build failures will be routed to RIGHT OWNERS instead of LEFT OWNERS. However, PR reviewers are already being assigned to RIGHT OWNERS, because GitHub supports wildcards.

[weshaggard]

Not sure yet what is the best way to approach this. I could ask the affected folks to review the owners diff I get from #5165 and ask all of them if they are OK with getting the build failure notifications now.

Looks like that diff still has the **/ci.yml and **/tests.yml paths in the codeowners file going to EngSys I know that is definitely going to be wrong so I would like to see a diff with those patterns removed.

As for the mgmt codeowners entries I think it is probably safe to keep those and let the folks still be labeled as the owners for all the mgmt libraries.

[weshaggard]

With our new approach we might also want to consider removing the /sdk/ pattern which was all about having the leads be the owners so they can push to get any missing owners figured out but once we get our pipeline setup we can have the pipeline identify new code paths that don't have owners.

[konrad-jamrozik]

Looks like that diff still has the **/ci.yml and **/tests.yml paths in the codeowners file going to EngSys I know that is definitely going to be wrong so I would like to see a diff with those patterns removed.

@weshaggard I added the diff you requested to the Excel diff, in the tab named azure-sdk-for-net_owners_diff_2 (note the _2 at the end).

With our new approach we might also want to consider removing the /sdk/ pattern

You mean only the the line with exactly /sdk/, yes? But not paths like /sdk/objectanchors/?

Here is my thinking:

• If that diff looks OK to you, I will remove the **/ci.yml and **/tests.yml paths from the azure-sdk-for-net. Done in this PR: Remove CODEOWNERS rules /**/ci.yml and /**/tests.yml azure-sdk-for-net#33595
• then I'll enable the new regex-based matcher for azure-sdk-for-net repo only, keeping the old matcher for other repos (I will hardcode some rules to make the matcher distinguish on which repo it runs and decide which matching logic to use).
• After some cooking time, I will then repeat this process for each of our other language repos.

Re the removal of /sdk/ I see following paths matching to it in the diff:

So, in a separate PR, I could remove /sdk/ and add a bunch of rules like /sdk/maps/, /sdk/quantum/ etc., with the same owners assigned. Then let the two owners, pallavit and jsquire know, to determine exact owners for these paths. I made a PR for this:

• Add missing /sdk/<dir> rules azure-sdk-for-net#33597

Thoughts?

[konrad-jamrozik]

@weshaggard I have updated all the CODEOWNERS files in scope of the new matcher. You can see what I did by reviewing the PRs I linked to this issue. There are still few PRs open, as can be seen above and below. But as soon as I get your approval on them, I will merge them, and then we can proceed with merging this PR.

I do have few outstanding questions about ownership, but I don't think they should be blocking. Here is the full list of unanswered questions:

• (java) Waiting for sragnar reply about java repo perf dir ownership
  • https://github.com/Azure/azure-sdk-for-java/pull/33221/files#r1095281680
• (java) Waiting for jfree to reply about java build failure notif. recipients. I also sent him an email.
  • Restore Eng Sys rules in CODEOWNERS + assorted fixes azure-sdk-for-java#33309 (comment)
• (net) Waiting for Arthur Ma reply about storage owners
  • Revert changes to CODEOWNERS that caused build failure notifications to be routed to wrong people + assorted fixes azure-sdk-for-net#33826 (comment)
• (python) Waiting for jsquire reply about build failure notif. recipients
  • Restore Eng Sys rules in CODEOWNERS + assorted fixes azure-sdk-for-python#28622 (comment)