/check-enforcer override

Sorry I guess had missed some of the context from Azure/azure-sdk-tools#5088 (comment), does this mean we won't get notified on ci/tests yml file changes, or that we are replacing that with a new codeowner or other config entry?

Sorry I guess had missed some of the context from Azure/azure-sdk-tools#5088 (comment), does this mean we won't get notified on ci/tests yml file changes, or that we are replacing that with a new codeowner or other config entry?

@benbp see the Excel diff I linked to in this comment:

• Enable the new, regex-based, wildcard-supporting CODEOWNERS matcher azure-sdk-tools#5088 (comment)

The tab with _2 suffix.

PATH column is the file path considered. LEFT PATH EXPRESSION is the rule that matched before this change, and corresponding owners are in LEFT OWNERS. These people are currently getting build failure notification for given path. After this PR is merged, instead the RIGHT OWNERS will be notified.

@konrad-jamrozik I see. I haven't been involved in these discussions so I don't want to block anything, but I did rely on being able to monitor pipeline yaml changes across all azure sdk pull requests. Is the plan to get rid of Wes and I as codeowners for those files permanently?

@konrad-jamrozik I see. I haven't been involved in these discussions so I don't want to block anything, but I did rely on being able to monitor pipeline yaml changes across all azure sdk pull requests. Is the plan to get rid of Wes and I as codeowners for those files permanently?

@benbp I believe so, but I am not sure if Wes was aware you rely on these rules. @weshaggard can you chip-in?

@benbp we have a conflict of interest here when it comes to supporting wildcards and using the ci/test.yml file to identify pipeline notification owners. Once we add the wildcard support these patterns will make us the owners for all pipelines and get notified as opposed to the area owners. I agree it is somewhat nice to be notified when folks change those files but I also think at this point most folks simply copy/paste those and not really a lot for us to review and give it gives us lots of noise. We will of course still get notified for anything under /eng/ but not necessarily for every ci/test.yml file. If you feel strongly about still getting notified we can try to come up with another mechanism to make that work.

@weshaggard I don't think my minority case is important enough to get in the way of the larger goal here. I'll figure out some automation for my own review workflows just wanted to clarify whether this functionality was going away.

@benbp we can talk about other workflow options next week in our 1:1.

@weshaggard @benbp we do own the CODEOWNERS interpreter we use for routing build failure notifications, and the notification configurator too, so I would be happy to adjust our logic to make Ben's life easier. If we really want to, it could be as simple as hardcoding some rules.

@konrad-jamrozik is my understanding correct that codeowners determines both who GitHub assigns as reviewers and that we use it for setting up build notifications? In this case it's the former functionality that I relied on, which I don't think we get in the middle of.

Maybe there's some sort of comment syntax we could parse to exclude certain codeowners directives from the notification parser? I don't know how strict the syntax is.

@konrad-jamrozik is my understanding correct that codeowners determines both who GitHub assigns as reviewers and that we use it for setting up build notifications? In this case it's the former functionality that I relied on, which I don't think we get in the middle of.

@benbp Yes.

It works like that:

CODEOWNERS -> GitHub interpreter -> reviewers
CODEOWNERS -> our interpreter -> build failure notifications

Maybe there's some sort of comment syntax we could parse to exclude certain codeowners directives from the notification parser? I don't know how strict the syntax is.

We could do that; we already have support for interpreting comments, which are ignored by GitHub. We were using it for FabricBot labelling logic, albeit right now it is unused.

But in such case we will have to keep /**/ci.yml and /**/tests.yml for GitHub's PR reviewers assignment, meaning we would have only one set of owners (including you) for each of these. If we are OK with that, then 👍.

@konrad-jamrozik I'm happy to be marked as owner for those files. I wouldn't say block the current work on that but if we could add a notification parser ignore comment in the future (and it doesn't complicate things) it would be helpful!

@benbp @weshaggard

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

Another option I was thinking is we could fake this by asking for the owner of a non-existent file like \sdk\service\PipelineOwner where PipelineOwner would be in the same path as ci.yml but not be a real file but we could use it to find the owner of files in that path.

Another option I was thinking is we could fake this by asking for the owner of a non-existent file like \sdk\service\PipelineOwner where PipelineOwner would be in the same path as ci.yml but not be a real file but we could use it to find the owner of files in that path.

@weshaggard I am not sure I understand how this would help. To my understanding we have following requirements:

1. At least one user (here: @benbp) needs to be added as a reviewer to all build definitions (ci.yml and tests.yml files)
2. The users added as reviewers for the build definitions do not want to be notified of build failures of builds instantiated from these definitions; instead, the respective area owners should be notified.

Because of 1. we must keep the /**/ci.yml and /**/tests.yml rules, otherwise GitHub won't add the relevant users as reviewers. Because of 2. we must ignore these paths for the purpose of build failure email routing determination. The next eligible path should match instead.

If we would add a "fake" path, as you suggest, it would not change any of the above - we still would have to keep the /**/ci|tests.yml paths and we would still need to ignore them for build failure notification determination.

Adding such "fake" path would help separate the build failure notification recipients from reviewers of the path that will eventually match. Consider:

/sdk/service/foo/ @owner2
/sdk/service/foo/PipelineOwner @owner3

# #build-failure-notification-ignore
/**/ci.yml @owner1

With your proposal, when a build from /sdk/service/foo/ci.yml fails, the notification will be routed to @owner3 instead of @owner2. But we could achieve the same effect with no special syntax;
instead of doing:
/sdk/service/foo/PipelineOwner @owner3
we could do:
/sdk/service/foo/ci.yml @owner3

and it would work. This is because the /sdk/service/foo/ci.yml reviewer is @owner1 in either case.

Additional notes on path unrolling

Strictly speaking we do not need /**/ci.yml - instead, we could "unroll" this rule to each instance of ** match. We could have /sdk/service/foo/ci.yml, /sdk/service/bar/ci.yml, and so on. Then we could add @benbp to all such paths, and also have different set of other owners for each of them, if this is flexibility we need. Now we are forced to have the same set of owners/reviewers for all ci.yml files. In such case we could also possibly avoid adding special-case handling of build failure notification routing, i.e. we wouldn't have to selectively ignore any paths. But of course it is more work to keep all such "unrolled" paths up to date.

@konrad-jamrozik I wasn't suggesting we would add /sdk/service/foo/PipelineOwner to the codeowners file. I'm suggesting in our pipeline notification tool we would find the owner of that non-existing file and it would then map to owner2 in your example instead of owner1. Today our pipeline notification looks for the owner of /sdk/service/foo/ci.yml but with my proposal it would look for /sdk/service/foo/PipelineOwner instead.

@weshaggard ah, I see, thx. So basically in this line, i.e.:

var codeOwnerEntry = CodeownersFile.GetMatchingCodeownersEntry(process.YamlFilename, codeOwnerEntries);

replace this:

process.YamlFilename

with something like:

GetSiblingFile(target: process.YamlFilename, siblingName: "PipelineOwner").

With this, we can keep the /**/ci.yml paths and there is no need to add "ignore this path for build failure notifications" comment directive logic. I like this approach more, so I have update the work item to reflect that:

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

and also submitted a PR:

• Send build failure notifications to build definition .yml file synthetic sibling CODEOWNERS owners, instead of the owners of the .yml file itself. azure-sdk-tools#5194

Yes that was the proposal. However @benbp and I just got done chatting and for now we don't want to do anything special to support this we will just remove /**/ci.yml patterns in the codeowners files like you did with this PR.