Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup codeowners file with new team structure #1864

Merged
merged 4 commits into from
Oct 22, 2024
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
@@ -1 +1,13 @@
/src/lib/gadgets @o1-labs/crypto-eng-reviewers @mitschabaude
# By default, review is required by o1js-admins
* @o1-labs/o1js-admins

# 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
Copy link
Contributor Author

@45930 45930 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.