Update Access Controls - SIMD-0007 #320
Conversation
There was a problem hiding this comment.
Why not just use the built-in support for GitHub teams on the platform itself? This script seems like a major burden to keep up-to-date, and it's also missing a few reviewers.
It would be a lot better to have two teams:
- Anza core devs
- Firedancer core devs
And then we could go a step further and use some teams for SME-related stuff, depending how annoying this gets:
- Fees/tx pipeline
- Alpenglow
- VM
- etc.
You could add the two core dev team reviewers automatically as required, and then just overhaul the Type field to use keywords than can bring in other SME-based reviewers (ie. Type: VM).
I think defining SME (subject matter expert) teams is a good idea but sometimes SIMDs don't fall into specific categories so I don't think we need to try to tie the SIMD "type" to a certain SME team just yet. Getting the appropriate SME approval should be a responsibility of either the SIMD author and core team approvers |
Agreed, so we could just have that field be omitted if there isn't a focus. Maybe just label it "SME". |
The intention is to keep the list of reviewers to the same folks that are doing backport reviews at Anza and similarly small list at Jump. They will be responsible for making sure the appropriate SME's have signed off. I do not think there will be any significant update burden in this case. Trivial to update a purposely small list if needed.
GitHub's built-in codeowners and branch protection rules don't currently support enforcing the logic we need: specifically, ensuring there's at least one approval from Jump Crypto and one approval from Anza. They only let you specify how many total approvals you need not which teams or groups those approvals must come from. If GitHub ever adds the ability to enforce "at least one approval from each of these separate teams," I'd happily drop this script entirely. Until then, this is the simplest way I found to enforce the desired policy and avoid the Foundation being a merge bottleneck. If you're aware of any simpler native GitHub method I might have missed, let me know |
We have this feature in Agave. It looks like we just combine the "Require approvals from codeowners" setting with a folder-based codeowners setup: https://github.com/anza-xyz/agave/blob/master/.github/CODEOWNERS |
Yes, this is the first approach I looked at but as I mentioned they only let you specify how many total approvals you need not which teams or groups those approvals must come from. |
| Object.entries(status).forEach(([u, s]) => | ||
| core.info(`${u}: approved=${s.approved}, blocked=${s.blocked}`)); | ||
|
|
||
| // 4. final approved list (approved && not blocked) |
There was a problem hiding this comment.
I think a single block from a group member should override all approvers from that group. Right now you could have a situation where a group member approves and another member of the same group requests changes and the result is that the group check passes.
|
Also I just noticed that after I comment on this PR, I get a notification that the approval checks are failing. Pretty annoying so we should fix that. |
We are updating SIMD-0007 to reflect that Foundation no longer needs to approve every PR; simplifying the access policy removes the Foundation as a bottleneck.
Summary of changes
CI:
Add github CI requirement to have at least one approval from Jump and at least one approval from Anza
SIMD-0007:
Collapse three access tiers into two: Write and Maintain.
State that every PR requires one approval from jump and one from anza (enforced by CI).
Clarify privileges:
Write – open PRs, self-merge after checks pass.
Maintain – merge any PR, manage repo settings, grant/revoke access.
Update vouching and removal rules to fit the two-tier model.