Setup codeowners file with new team structure #1864
Conversation
| # Changes to the following paths should be reviewed by o1js-admins, but we will tag current and future zkapps teams as well (incorrect use of codeowners) | ||
| # We are relying on good faith to wait for admin approval on these PRs | ||
| # Dangerous PRs that could release a new version, new consensus rules, or anything like that will necessarily touch more than `/src/lib`, so this is minimally risky | ||
| /src/lib @o1-labs/o1js-admins @o1-labs/exploration-team @o1-labs/future-zkapps |
There was a problem hiding this comment.
This incorrect use of codeowners is still a no-less-secure subset of our current rules AFAICT. Right now any o1 eng can approve o1js PRs. This will require a review from a subset of that group.
Reviews from crypto, protocol, or node specialists may also be required from time to time, which can be requested manually. We probably also want to ensure that a github admin can override and merge an emergency PR if needed.
|
@mrmr1993 With the updated comments, does this procedure work for you? |
CODEOWNERS
Outdated
|
|
||
| # Changes to the following paths can be safely owned by the current and future zkapps teams (correct use of codeowners) | ||
| /src/examples @o1-labs/exploration-team @o1-labs/future-zkapps | ||
| /src/lib/util @o1-labs/exploration-team @o1-labs/future-zkapps |
There was a problem hiding this comment.
I would be in favor of leaving these specifications out of the file until we see an actual need for it - right now it seems too arbitrary (other than the examples folder maybe). If we decide to make exceptions like these, we would have to add more than just this particular folder. On the other hands, it's hard to differentiate between changes that might seem trivial but are actually non-trivial
There was a problem hiding this comment.
Ok, I will rm this for now.
Just so you know what I'm going for, codeowners can be used to require review of specific files by specific teams. In this case (and thus reduce the load of review from other teams). If a big PR includes API changes, new data types, tests, docs, and an example, maybe we only need admin review on the API changes and data types. The other changes still need a review, but that review can come from one of the teams instead.
I will go with the simplest version to start, which is just o1js-admin is required by default and current and future zkapps teams will be required on changed to /src/lib since that's basically the current model.
|
@Trivo25 I'll merge but we may still need admins to set up the round robin assignment. |
This PR cleans up the
CODEOWNERSfile and lays the groundwork for a new round-robin code review process that will give the exploration team and future zkapps team exposure to o1js PR reviews.Ultimately, it is the
o1js-adminsjob to manage what code gets into o1js. This process aims to get more people to the level that they can be added to the list of admins.