Manage CODEOWNERS programmatically (and regenerate it)#9529
Conversation
At the time of writing we have 97 (!!) packages in this monorepo, which are owned by various teams across MetaMask. However, managing codeownership via the CODEOWNERS file is tedious and error-prone. We do use Yarn constraints to ensure that each package is configured in CODEOWNERS correctly, so that helps. However, in a future commit, we would like to update the set of per-package files that are owned by Core Platform to include more than just `package.json` and `CHANGELOG`, and at the moment, we need to do that manually; Yarn constraints won't allow us to automatically apply those updates. To address this problem, this commit redefines `CODEOWNERS` as a TypeScript file (`codeowners.ts` in the root) and adds a script to generate CODEOWNERS from this file. The script also comes with a `check` command which is plugged into the lint pipeline to ensure that CODEOWNERS is always up to date. The primary "feature" of `codeowners.ts` is that it contains a configuration object which maps of teams to packages that are owned by those teams (along with some other metadata). Ideally, when a new package is added, all a team needs to do is update this map and then the appropriate codeowner rules will be automatically added to support that package. To ensure minimal changes between the current version of CODEOWNERS and the new version in this PR, we needed to make some compromises, and `codeowners.ts` is not as dynamic or simple as it could be. We plan on making further changes in the future to make it more useful.
83ad642 to
a6936c4
Compare
| * | ||
| * @returns The team sections. | ||
| */ | ||
| function buildTeamSections(): CodeownersSection[] { |
There was a problem hiding this comment.
As I alluded to in the PR description, this file is way longer than it should be, mainly because I wanted the diff for CODEOWNERS to be relatively easy to read. In the future we should be able to generate most of this purely from PACKAGES (with a few exceptions).
| # Lines starting with '#' are comments. | ||
| # Each line is a file pattern followed by one or more owners. | ||
|
|
||
| # Note: Please keep this synchronized with the \`teams.json\` file in the repository root. |
There was a problem hiding this comment.
Using a TypeScript file to generate CODEOWNERS also lets us automatically check this in the future. That should allow us to remove this comment.
There was a problem hiding this comment.
To read this diff, you'll probably want to turn off whitespace. (I could have coded whitespace settings into codeowners.ts but I didn't want to make that file worse than it already was.)
| /packages/account-tree-controller/package.json @MetaMask/accounts-engineers @MetaMask/core-platform | ||
| /packages/account-tree-controller/CHANGELOG.md @MetaMask/accounts-engineers @MetaMask/core-platform | ||
| /packages/accounts-controller/package.json @MetaMask/accounts-engineers @MetaMask/core-platform | ||
| /packages/accounts-controller/CHANGELOG.md @MetaMask/accounts-engineers @MetaMask/core-platform |
There was a problem hiding this comment.
For some reason accounts-controller/CHANGELOG.md was listed further down than it should have been.
| /packages/assets-controller/package.json @MetaMask/metamask-assets @MetaMask/core-platform | ||
| /packages/assets-controller/CHANGELOG.md @MetaMask/metamask-assets @MetaMask/core-platform | ||
| /packages/config-registry-controller/package.json @MetaMask/networks @MetaMask/core-platform | ||
| /packages/config-registry-controller/CHANGELOG.md @MetaMask/networks @MetaMask/core-platform |
There was a problem hiding this comment.
The files for config-registry-controller were listed in the wrong order.
Explanation
At the time of writing we have 97 (!!) packages in this monorepo, which are owned by various teams across MetaMask. However, managing codeownership via the CODEOWNERS file is tedious and error-prone.
We do use Yarn constraints to ensure that each package is configured in CODEOWNERS correctly, so that helps. However, in a future commit, we would like to update the set of per-package files that are owned by Core Platform to include more than just
package.jsonandCHANGELOG. at the moment, we need to do that manually — Yarn constraints won't allow us to automatically apply those updates — and it would be painful.To address this problem, this commit redefines
CODEOWNERSas a TypeScript file (codeowners.tsin the root) and adds a script to generate CODEOWNERS from this file. The script also comes with acheckcommand which is plugged into the lint pipeline to ensure that CODEOWNERS is always up to date.The primary "feature" of
codeowners.tsis that it contains a configuration object which maps of teams to packages that are owned by those teams (along with some other metadata). Ideally, when a new package is added, all a team needs to do is update this map and then the appropriate codeowner rules will be automatically added to support that package.That said, to ensure minimal changes between the current version of CODEOWNERS and the new version in this PR, some compromises needed to be made, and as a result,
codeowners.tsis not as dynamic or simple as it could be. In the future, we plan on making further changes to makecodeowners.tseasier to maintain.References
To merge #8384, I had to enable admin powers to bypass codeowner requirements. This is because tsconfig files are not co-owned by Core Platform. We could of course change CODEOWNERS to do this but it would be a lot of work and would add complexity to CODEOWNERS. This PR came out of that observation.
Manual testing
yarn codeowners:check. It should not print any output, and should exit with 0.yarn codeowners:generate. It should also not print any output, it should not change.github/CODEOWNERS, and should exit with 0.codeowners.tsand add@MetaMask/core-platformto the list of teams foraccount-tree-controller.yarn codeowners:check. It should print an error.yarn codeowners:generate..github/CODEOWNERSshould change (the list of owners foraccount-tree-controllershould include @MetaMask/core-platform`)..github/CODEOWNERS.codeowners.tsand removeinitializationPathfromaddress-book-controller.yarn codeowners:check. It should print an error.yarn codeowners:generate..github/CODEOWNERSshould change (address-book-controllershould be removed from the "Initialization" section).Checklist
Note
Low Risk
Behavior change is tooling and regenerated CODEOWNERS content intended to match prior rules; risk is mainly a mistaken owner mapping in the new config affecting required reviewers.
Overview
CODEOWNERS is now generated from a new root
codeowners.tsconfig instead of being edited by hand. ThePACKAGESmap is the source of truth for team ownership (plus optional walletinitializationPath); builders assemble team sections, joint ownership, wallet initialization paths, and releasepackage.json/CHANGELOG.mdrules (with Core Platform appended for releases).A
scripts/manage-codeownersCLI addscheck(fail if.github/CODEOWNERSdiffers) andgenerate(rewrite the file with padded columns).yarn codeowners:checkis wired intolintand the CI lint matrix;yarn lint:fixrunscodeowners:generate. The checked-in.github/CODEOWNERSdiff is largely regeneration/formatting of the same rules, not a broad ownership policy change.Reviewed by Cursor Bugbot for commit a6936c4. Bugbot is set up for automated code reviews on this repo. Configure here.