Branch Protection Rules support for main branch

[richm]

This adds a playbook update_branch_protection_rules.yml for setting Branch Protection Rules for the main branch.

The default rules are quite strict - require approvals, require status checks, clear approvals if a new commit is pushed, do not allow admin override, and more.

The status checks are not currently configurable per group or per role, but you can have additional status checks. For example, the network role has additional status checks not used by the other roles. There are additional python status checks, and additional shell checks, for the roles that support and use those checks.

More default status checks will be added - we're in the process of revising the names of the baseos ci status checks, so more will be coming soon.

Signed-off-by: Rich Megginson rmeggins@redhat.com

[richm]

Branch protection rule.pdf This is an example of what the new rules look like in the kernel_settings role.

[martinpitt]

Thanks @richm ! This is quite a nice way to sync configuration across many related GH projects. Big :+1: TBH, I consider most of these restrictions as a "bare minimum", and they should really be enable by default. The only disputable one is "Require branches to be up to date before merging", which isn't suitable for projects which get more than ~ one PR per day and have too expensive CI to constantly rebase (like Cockpit), but for LSR we can afford to enforce it.

Cheers!

[tyll]

The only disputable one is "Require branches to be up to date before merging", which isn't suitable for projects which get more than ~ one PR per day and have too expensive CI to constantly rebase (like Cockpit), but for LSR we can afford to enforce it.

GitHub now has beta support for merge queues which should solve this problem for Cockpit as well: https://github.blog/changelog/2021-10-27-pull-request-merge-queue-limited-beta/ - it seems that I might have applied the network role for this already, since I got a notification recently, that we can use it.

The challenge that I see, is that the system roles CI does not seem to be reliable enough for this to properly work.

[richm]

The only disputable one is "Require branches to be up to date before merging", which isn't suitable for projects which get more than ~ one PR per day and have too expensive CI to constantly rebase (like Cockpit), but for LSR we can afford to enforce it.

GitHub now has beta support for merge queues which should solve this problem for Cockpit as well: https://github.blog/changelog/2021-10-27-pull-request-merge-queue-limited-beta/ - it seems that I might have applied the network role for this already, since I got a notification recently, that we can use it.

Ok. That's another thing I need to figure out how to apply to all roles . . . seems like overkill though with as few PRs as we get across all of the system roles, plus the CI problems mentioned below.

The challenge that I see, is that the system roles CI does not seem to be reliable enough for this to properly work.

[tyll]

GitHub now has beta support for merge queues which should solve this problem for Cockpit as well: github.blog/changelog/2021-10-27-pull-request-merge-queue-limited-beta - it seems that I might have applied the network role for this already, since I got a notification recently, that we can use it.

Ok. That's another thing I need to figure out how to apply to all roles . . . seems like overkill though with as few PRs as we get across all of the system roles, plus the CI problems mentioned below.

The unreliable CI and possible high latency is actually an argument for the merge queues because it will reduce the amount of necessary CI runs on a specific day when there is a deadline and changes pile up.

[richm]

GitHub now has beta support for merge queues which should solve this problem for Cockpit as well: github.blog/changelog/2021-10-27-pull-request-merge-queue-limited-beta - it seems that I might have applied the network role for this already, since I got a notification recently, that we can use it.

Ok. That's another thing I need to figure out how to apply to all roles . . . seems like overkill though with as few PRs as we get across all of the system roles, plus the CI problems mentioned below.

The unreliable CI and possible high latency is actually an argument for the merge queues because it will reduce the amount of necessary CI runs on a specific day when there is a deadline and changes pile up.

I still don't think it is necessary to use merge queues for system roles, despite the recently flurry of activity in the network role.

[tyll]

I still don't think it is necessary to use merge queues for system roles, despite the recently flurry of activity in the network role.

It seems it is also necessary to have merge queues for automated merging to properly work. Otherwise GitHub seems to wait for a human to click the "rebase" button. I enabled merge queues for the network role so we can take a look there.

Regarding disallowing admins to override the branch protection rules: I used this for the current network changelog PR since it was waiting on the integration tests and it is also the fallback in case the code owner is unavailable. What's the plan for that? I guess there should also be a more sophisticated codeowners file to add the system-roles core team as owners for LSR specific paths such as the changelog, maybe tox and .github?

[richm]

I still don't think it is necessary to use merge queues for system roles, despite the recently flurry of activity in the network role.

It seems it is also necessary to have merge queues for automated merging to properly work. Otherwise GitHub seems to wait for a human to click the "rebase" button. I enabled merge queues for the network role so we can take a look there.

Ok. Then the network role will be the PoC for merge queues, and we'll see if they will be generally useful for all of the system roles.

Regarding disallowing admins to override the branch protection rules: I used this for the current network changelog PR since it was waiting on the integration tests and it is also the fallback in case the code owner is unavailable.

Right.

What's the plan for that?

Typically what I do is go into Settings -> Branch Protection Rules and turn off this switch, merge the change, and then turn back on the switch.

I guess there should also be a more sophisticated codeowners file to add the system-roles core team as owners for LSR specific paths such as the changelog, maybe tox and .github?

You can do that with codeowners?

[richm]

I still don't think it is necessary to use merge queues for system roles, despite the recently flurry of activity in the network role.

It seems it is also necessary to have merge queues for automated merging to properly work. Otherwise GitHub seems to wait for a human to click the "rebase" button. I enabled merge queues for the network role so we can take a look there.

Ok. Then the network role will be the PoC for merge queues, and we'll see if they will be generally useful for all of the system roles.

Regarding disallowing admins to override the branch protection rules: I used this for the current network changelog PR since it was waiting on the integration tests and it is also the fallback in case the code owner is unavailable.

Right.

What's the plan for that?

Typically what I do is go into Settings -> Branch Protection Rules and turn off this switch, merge the change, and then turn back on the switch.

The only repo currently that has isAdminEnforced: true is the cockpit role - I guess @martinpitt prefers the extra layer of security and isn't too concerned with having to override status checks/approvals and force merge. Martin - are you ok if we set isAdminEnforced: false for now? @tyll are you willing to try isAdminEnforced: true?

It's too bad there isn't a switch to "allow admin to merge if CI/github action check failure" but "disallow admin to merge if there aren't enough reviews".

I guess there should also be a more sophisticated codeowners file to add the system-roles core team as owners for LSR specific paths such as the changelog, maybe tox and .github?

You can do that with codeowners?

Apparently yes, but I would rather do that in a subsequent PR.

[richm]

@tyll @martinpitt https://github.com/linux-system-roles/.github/pull/16/commits/80123f93606e4f7b1abafa436add32e70940c64a allows isAdminEnforced per role, and makes cockpit the only role currently that uses true