Branch protection rules should become a part of the automation
Benjamintf1 opened this issue · 8 comments
I'd suggest that we have standardized and automated branch protection rules. I suggest the following for those rules.
Which branches?:
I'd suggest that branch protection rules should be applied to: main and to v[0-9]* branches. This should give some protection to release line branches, while allowing for non approvers to still be able to create branches in cloudfoundry rather then having to keep and maintain forks. This should cover golang libraries well as well.
While certain repos have a develop or release-elect pattern, I suggest these patterns be considered deprecated as per discussion about better following open source patterns of development
Rules:
Require a pull request before merging: (this encourages good practice with regards to making sure changes that are submitted are good, but also fostering open discussion and tracking of meaningful changes).
Required Approvals: I set this to 1 before the permissions changes. I think this is a good idea. Arguably, it only prevents accepting your own pr if you are a approver, and then merging. You don't need two approvers, you just need 2 people of which one is an approver. For some areas we only have one approver or one very active approver. It doesn't do everything. I added it mostly to indicate ci requirements, but it's not technecially necessary for it.
Dismiss stale pull request approvals when new commits are pushed: I think this one is important and good for ci requirements. Prevents ddos and other such attacks against ci infrastructure, especially ci infrastructure that provisions iaas resources. We right now run against forks in logging-and-metric things maintained by our ci, and allowing running against forks is condusive to accepting contributions, but having this rule protects against attacks.
Require review from Code Owners: I don't think we need this. Would require keeping owners up to date, sounds like a lot of toil for not a lot of value.
Restrict who can dismiss pull request reviews: No opinions. Hasn't been a problem so far.
Allow specified actors to bypass required pull requests: I'd like this to include bots. Specifically, it'd be nice to continue to use ci to cut releases and bump submodules/golang(a not great situation, but still nice), and creating releases requires creating a commit and adding it to a branch for bosh.
Require status checks to pass before merging: Seems unnecessary to me. Obviously checks can be important, but I think it discourages adding new tests, and should be left to approver discretion.
Require conversation resolution before merging: Seems a bit like busy work to me? Could be left to discression?
Require signed commits: Seems a bit onerous to me for little benefit.
Require linear history: I don't see why this should be a requirement
Require deployments to succeed before merging: I htink this should be approver specific, that said I don't think any repos use deployments.
Do not allow bypassing the above settings: I think this should be false
Restrict who can push to matching branches: I think this should be true and limited to approvers and bots. This allows giving people push to branches, but not release branches for easier contribution.
Restrict pushes that create matching branches: This should also be true. It's a bummer to discover the branch you created, you're unable to update. It's not unheard of to have v3-do-thing as a branch and shouldn't be mistaken for a release branch.
Restrict who can push to matching branches: I think this should be true and limited to approvers and bots. This allows giving people push to branches, but not release branches for easier contribution.
@Benjamintf1 can you please clarify this recommendation? The first half seems to conflict with the second half.
❓ Are you recommending that working group leads, approvers (in the relevant area), and bots be able to bypass the branch protection rules and push to main and to v[0-9]* branches? Or just working group leads and bots?
@ctlong WG leads, approvers, and bots should be allowed to "interact with the main and v[0-9]* branches". Allowing those groups into this sub-permission does not "override branch protection rules" it only overrides this branch protection rule. Specifically.
If you have write, but are not overriding "Restrict who can push to matching branches", or "Allow specified actors to bypass required pull requests" you can create new branches, but cannot push or merge pull requests to main or v[0-9]*. (think, cf member, but not a approver for that area).
If you are an approver(have write and also overrided restrict who can push to matching branch, but not overrided bypass pull requests), then you can merge pull requests when the required reviews are reached, but NOT push directly to branches.
If you are bot(have write and overrided restrict who can push to branch AND overriding pull request), then you can push to branches and merge pull requests(I believe this is necessary permission, but bots shouldn't be merging pull requests).
These do not conflict because it allows for giving people who are not approvers to be given permission to create branches for creating prs.
The current automation is based on peribolos which doesn't support to configure branch protection rules.
Still, it makes sense to discuss a recommended setup for branch protection rules.
And as of today, only WG leads and TOC can configure branch protection rules because you need Github admin rights on the repo to do so. This is indeed a severe limitation because it hinders dev teams to take care of their CI without having to ask WG leads or TOC.
From TOC point of view, we don't want to block teams from configuring branch protection rules but we want to ensure that github repos can't be deleted but they are archived after going through the process described in rfc-0007-repository-ownership.
I see the following options to grant dev teams control over branch protection rules:
- Switch to Github fine grained roles which offers a 'maintainer' role. IIRC, this was discussed in the TOC and abandoned because fine grained roles are only available in a more expensive Github plan.
- Invent a new WG area admin role that can be maintained in the WG charters. WG area admins would get admin right on the WG area repositories and can then maintain branch protection roles similar to a WG lead. More power = more responsibility ... Requires a bit implementation in the org automation but no additional pericolos features.
- WG lead could promote some WG area approvers to WG technical leads. This grants admin access to all WG repositories. Works out of the box but is maybe too course-grained.
There is a tool called branchprotector from the same people who maintain peribolos: https://github.com/kubernetes/test-infra/blob/master/prow/cmd/branchprotector/README.md
The RFC for this is merged. We're now waiting on the updates to the branch protector tool upstream.
The changes got merged, and the ARD working group subsequently had the branch protection rules enabled as a guinea pig. Eventually, we'll open this up to other working groups.
Branch protection is implemented and meanwhile used by ARD, ARP and cf-on-k8s WGs.
Documentation:
- https://github.com/cloudfoundry/community/blob/main/toc/rfc/rfc-0015-branch-protection.md
- https://github.com/cloudfoundry/community/blob/main/org/readme.md
I think we can close this issue and create new feature requests for branch automation like opt-in per WG area, nicer ways to configure required status checks etc. Depends on feedback by the WGs.