Github collaboration features: tools for organizational scaling

[alice-i-cecile]

Motivation

The following features are listed with the permissions level required to perform the action in parentheses.

• there are many useful features that are limited to team members with repo write access
  • mass-select issues and PRs (Maintain)
  • edit PR and issue titles (Write)
  • manage labels (Write)
  • edit PR and issue descriptions (Write)
  • delete genuine spam and abuse (Admin)
• we may be able to coordinate this with bors (or replace bors), in order to allow genuine "scoped access" for domain experts to improve organizational scaling

Dangerous powers

There are several powers that are moderately dangerous to give to Bevy org members, that we should be careful about handing out:

• the ability to delete issues and PRs, particularly in an unlogged fashion (Admin)
• delete discussions (Triage)
• the ability to edit others' comments (Write, but anyone inspect edit history)

There are several seriously dangerous powers, that we should avoid handing out if at all possible:

• the ability to cut a new release (requires Write-level permissions)
• the ability to delete the repository (requires
• the ability to add new members to the Bevy org

Testing

I've created a test org and test repo to experiment with features. Feel free to message me (or comment below) if you want access to experiment.

General settings

When merging pull requests, you can allow any combination of merge commits, squashing, or rebasing. At least one option must be enabled. If you have linear history requirement enabled on any protected branch, you must enable squashing or rebasing.

If we move off of bors (see below), we should enable squash commits only.

Always suggest updating pull request branches
Whenever there are new changes available in the base branch, present an “update branch” option in the pull request.

This should be enabled.

Requiring 2FA

We should strongly consider enabling this for the Bevy org, especially if and when we grant additional powers.

Role-based access control

By default, there are 5 permissions levels. These roles mostly fit our needs, although there are some questionable choices.

We could, in theory, create custom roles. However, this seems to be restricted to Enterprise users, at a steep $210 / yr / seat...

I'll reach out to support, to see if they would be open to granting this permission to an open-source team. We could find a rather deep-pocketed sponsor to support this, but TBH this is a high price to pay and would force us to be much more restrictive about org membership.

Test setup

This testing was done on a test repo using a second dummy account. Please feel free to message me asking for permissions if you would also like to experiment.

Test results

Branch protection

Branch protection: You can protect important branches by setting branch protection rules, which define whether collaborators can delete or force push to the branch and set requirements for any pushes to the branch, such as passing status checks or a linear commit history.

Rule analysis

There are several rules we could turn on for main:

• Require a pull request before merging: Yes, this is standard practice.
  • *Require approvals: Yes, but set the number of required approvals to 1.
  • Dismiss stale pull request approvals when new commits are pushed: No, too much friction for a volunteer team.
  • Require review from Code Owners: Maybe! See above.
• Require status checks to pass before merging: Yes, we're doing this with bors already. It's unclear if we can get the "always green" behavior we need with this alone. Enable for everything other than maybe dead-links.
  • Require branches to be up to date before merging: it seems like we could enable this, and remove bors!
• Require conversation resolution before merging: No, many conversations are useful context that will be helpful to future readers, and may not be correct to resolve.
• Require signed commits: No: way too high of a barrier to entry.
• Require linear history: No: not worth it when we're using squashed commits
• Include administrators: Yes, Cart's broken CI several times due to violating these rules 😂
• Allow force pushes: Yes, this is useful for rebasing, which is generally easier and cleaner than merging.
• Allow deletions: No idea.

Code owners

Code owners: You can use a CODEOWNERS file to define individuals or teams that are responsible for code in a repository.

PRs will not be able to be merged, unless they have approval from all code owners.

bors will obey code owners restrictions directly.

[alice-i-cecile]

Pattern 1: Domain-based subteams

1. Assign crates to each team.
2. Require many approvals (3-5?) + code owner approval to allow merging PRs.
3. Hand out Write-level permissions to team members.

Pros

• Empowers team members directly
• More robust to absence of critical members
  • Reduces pressure on individual volunteers
• Gives org members several extremely useful powers:
  • Rebasing
  • Editing PR and issue titles and contents.

Cons

• Increases risk of low-level vandalism (deleting issue tags, closing issues, editing comments and titles)
• Increases rate of breaking changes
• Risks code-quality slippage
• Eventually requires defining teams and code owners for all areas of the code base

[alice-i-cecile]

This is my favorite solution: I think it reduces risk while empowering team members, and avoids creating secondary bottlenecks.

[IceSentry]

So, just to make sure I understand this pattern. If someone makes a PR that only touches the bevy_pbr crate, that would mean that if X amount of members of the rendering team approves the PR, it will be merged in without cart ever needing to look at it?

I think I like this, but I feel like we are going to need the X to be pretty high just to make sure nobody can force their vision in a PR, other than cart obviously.

I assume all the teams will also be able to write to the examples folder, otherwise the vast majority of PRs won't benefit from this.

Are the low-level vandalism risk really increased compared to what we have currently? The vast majority of people in the org are already in the triage team and can already delete tags and close issues.

[james7132]

Code quality slippage isn't too much of a concern given we would need even more eyes on each PR being merged here. 3 independent approvals is already pretty heavy in itself, and one team member should give enough domain specific understanding to ensure code quality doesn't degrade significantly. Assuming every actor here acts in good faith, of course. Though malicious internal actors is largely mitigated by the number of approvals needed here. There would need to be concerted team-internal and general groups of bad-faith approvers to abuse this structure.

This does, however, mean we have to be more selective as to who is on which team, but that's an administrative burden largely left to Cart here, and is a non-recurring time investment.

[james7132]

Likewise, combining this with the unstable features being proposed should minimize the breaking change surface area to normal users.

[alice-i-cecile]

Are the low-level vandalism risk really increased compared to what we have currently? The vast majority of people in the org are already in the triage team and can already delete tags and close issues.

Yes IMO: currently they can only delete tags from a specific issue. This change would allow them to delete whole categories of tags, or modify comments in hard-to-detect ways.

This does, however, mean we have to be more selective as to who is on which team, but that's an administrative burden largely left to Cart here, and is a non-recurring time investment.

Agreed here. IMO we'd want to scrap the existing teams and start again from scratch at a higher bar of "proven expertise".

[alice-i-cecile]

Pattern 2: Team leads

1. Assign crates to each team.
2. Require 2 approvals to allow merging PRs.
3. Hand-out write level permissions to team leads.

Pros

• natural extension of existing model, but with additional convenient features for TLs
• avoids having to set up code-owners

Cons

• relies on honor system / logging to ensure appropriate scope of actions
• more pressure on individual volunteers
• less robust
• team members don't have useful project-management permissions granted by Write role

[alice-i-cecile]

This could work, but I'm nervous about the added pressure on the team leads.

It also limits our ability to distribute useful but tedious work across the organization.

[alice-i-cecile]

Pattern 3: Cart, owner of code

1. Give Bevy org members write permissions to the repo.
2. Require code-owner sign-off to merge PRs.
3. Make Cart / Francois / myself owners of all the code.

Pros

• This gives us the ability to hand out the useful permissions without actually enabling org members to write to main

Cons

• Increases risk of low-level vandalism (deleting issue tags, closing issues)
• Horribly janky
• Doesn't improve scaling much
• Infinite irrelevant pings: code owners are pinged every time a PR is made that touches code that they own

[alice-i-cecile]

Oh god plz no. The number of notifications are bad enough already.

I also don't think this actually improves the scaling bottleneck much either.

[alice-i-cecile]

Pattern 4: Bors, owner of code

1. Give Bevy org members write permissions to the repo.
2. Require code-owner sign-off to merge PRs.
3. Make bors the owner of all the code.
4. Modify bors so it approves code when someone with permissions calls bors r+

Pros

• This gives us the ability to hand out the useful permissions without actually enabling org members to write to main
• No irrelevant pings!

Cons

• Increases risk of low-level vandalism (deleting issue tags, closing issues)
• Extremely horribly janky
• Doesn't improve scaling much

[alice-i-cecile]

Less bad than 3, but wow, what a mess.

Again, I don't think this actually improves the scaling bottleneck much.

[bjorn3]

Always suggest updating pull request branches
Whenever there are new changes available in the base branch, present an “update branch” option in the pull request.

This should be enabled.

With bors squashing all commits already this shouldn't be needed. If anything it would confuse contributors into thinking that they need to update their branch before changes can be accepted.

Require a pull request before merging: Yes, this is standard practice.

This will probably break bors as it works on a branch without opening a pull request itself.

Require status checks to pass before merging: Yes, we're doing this with bors already. It's unclear if we can get the "always green" behavior we need with this alone. Enable for everything other than maybe dead-links.

I agree. If bors is one of the status checks that will also force usage of bors, which may allow restricting merge permission even for those with write permission on github.

Require branches to be up to date before merging: it seems like we could enable this, and remove bors!

Bors helps a lot with it's merge queue. Without bors you would have to wait for each PR to pass CI before you can merge it and update the next PR.

[alice-i-cecile]

Bors helps a lot with it's merge queue. Without bors you would have to wait for each PR to pass CI before you can merge it and update the next PR.

Most of the suggestions you mentioned were implicitly exploring "can we remove bors?". Couldn't we use a merge queue to get the same effect now?

[bjorn3]

Note: During the beta, there are some limitations when using the merge queue:

• The merge queue cannot be enabled on branch protection rules using wildcards (*) in the name.
• There is no support for squash merge commits. (Only merge commits and "rebase and merge" commits are supported.)

Looks like not. Squashing is not yet supported.

[cart]

We also still haven't been given merge queue access (we've been on the waitlist since they were announced).

[alice-i-cecile]

Pattern 5: Scoped team leads

1. Assign crates to each team.
2. Setup code owners for the crates, assigned personally to team leads.
3. Require 2 approvals (including a code owner) to allow merging PRs.
4. Keep permission levels for team leads at Triage, but ensure they have bors r+ power.
5. Encourage the use of bors delegate to hand off permissions

Pros

• natural extension of existing model
• some level of enforcement of scope via code-owners
• no increased risk of vandalism

Cons

• more pressure on individual volunteers
• less robust
• no one has access to useful project-management permissions granted by Write role

[cart]

On Removing Bors (and why I think we can't)

We already explored the "no bors world", pretty much to its full extent (i really wanted to stay "github native" and put off using bors for as long as I could). "Github native" has a number of deal breaking problems:

• Without requiring linear history, this would allow us to merge "old prs without conflicts", even if they break the build. This is unacceptable.
• If we do require linear history, this forces us to merge PRs one-by-one and manually rebase each pr. We added a "rebase bot" that automated rebases on command, but github can't always successfully rebase, even if there are no conflicts. Even with automation, this UX was bad.

Bors solves a number of problems for us:

• No need to rebase every pr prior to merging: bors only runs on the final "squashed and rebased" branch. This ensures we are "always green" without forcing the UX nightmare of linear history on each pull request (unless their changes really aren't "green" on top of main).
• Can merge multiple PRs at once (and no need to wait for any specific prs' validation). We can just fire off a bunch of bors r+ commands and things will work out (sometimes faster-than-serially because bors can check more than one branch at once).
• Hyper-scoped merge rights: only grant people access to merging code via bors. No other repo permissions granted. Bors does also support the CODEOWNERS pattern, if we decide this is the right move.

Github's merge queues might eventually solve these problems for us, but right now they are still in beta (and we haven't been granted access yet ... and we requested access right when they were announced). They also don't support squash merges yet, which would be close to deal breaking.

[alice-i-cecile]

Github's merge queues might eventually solve these problems for us, but right now they are still in beta (and we haven't been granted access yet ... and we requested access right when they were announced). They also don't support squash merges yet, which would be close to deal breaking.

Yep, unfortunately this is the conclusion that @bjorn3 and I have come to as well. We can revisit this again later, once the feature is out of beta.