Code reviews, CODEOWNERS, and notification spam

[tylerbutler]

Now that we've lived with CODEOWNERS-based review assignment for a few days, I want to take everyone's pulse on the changes. I've heard some feedback about review spam in particular, so I want to open discussion about ways we can reduce it, or any other topic related to code reviews and the PR process.

Goals

These are the goals for the code review/PR process.

• Ensure code changes are properly reviewed and approved by folks most knowledgeable about the code being changed.
• Automate the reviewer assignment to help prevent ignored PRs.
• Use GitHub features where possible.

The current implementation meets the goals, but it's far from perfect.

Some options for spam reduction

Use CODEOWNERS only for stable APIs

With the current config, almost any change will loop in a review group, because every file in the repo has an owner. I think in practice we're not ready for that level of review automation.

Instead of assigning ownership to everything, we could only assign ownership to parts of the codebase that are pretty stable. But maybe nothing is stable at this point.

Adjust the code review settings for the team

The code review teams can be configured to do a load-balancing or round-robin review assignment. I think this works better with more people in a team; most of our review teams only have 1-3 people in them.

Still, this is an option to keep in mind, especially in conjunction with some of the other options.

Note that I removed myself from most of the teams, so if you want to adjust these settings you'll need to ask the review team owner to do it.

Exclude package.json (done in #9024)

One thing I've considered is excluding package.json from CODEOWNERS. This would mean that the PRs that mostly touch a project just to update a dep wouldn't ask the whole world for a review. Take #9013 as an example. That PR requested reviews from:

@microsoft/fluid-cr-core
@microsoft/fluid-cr-dds
@microsoft/fluid-cr-framework
@microsoft/fluid-cr-loader
@microsoft/fluid-cr-runtime
@microsoft/fluid-cr-tree

But if package.json changes were excluded, then only @microsoft/fluid-cr-framework and @microsoft/fluid-cr-runtime would have been added.

Only include source files

Another alternative is to get much more precise in the definitions. For example, the whole /packages/framework/ folder is owned by fluid-cr-framework. We could instead change this to be

/packages/framework/**/src/**

That would effectively reduce review requests to only code changes.

---

There are other options, too, but I wanted to get the discussion started. What say you all? :)