Enable "Require a pull request before merging"

[rusty-snake]

What do you think about enabling "Require a pull request before merging" for master?

Require a pull request before merging
When enabled, all commits must be made to a non-protected branch and submitted via a pull request before they can be merged into a branch that matches this rule.

Going through a review process (and CI run) for all changes to catch mistakes, typos, missing things, ... will reduce further fixup commits and improve the quality of firejail.

[netblue30]

I agree, but let's wait for this release to go out. How do we set it up?

[rusty-snake]

Add a branch protection rule for master and enable "Require a pull request before merging".

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

[netblue30]

will do, thanks!

[netblue30]

I have set it up for mainline, and requires an approval. GitHub will run the checks on the branch, however the guy checking in should be able to approve it himself, so we don't always need two people to be involved in checking in.

[Fred-Barclay]

Looks great! Would definitely be nice if the committer could approve/merge as well. If GitHub allows, perhaps not until after the checks are complete?

[netblue30]

Would definitely be nice if the committer could approve/merge as well.

Try it on this pull rq: #4920

If it doesn't work, I'm afraid we'll have to scrap "Require a pull request before merging". Every one of you should be able to fully check in without a second person approving it. If something breaks, we can always fix it later.

[Fred-Barclay]

It doesn't work - the "Merge pull request" button is greyed-out for the author of a PR. For me (for example) this means I can merge PRs by anyone except myself. Ditto for approving merges: I can approve others' PRs but not my own.

[rusty-snake]

And if you follow and self approve?

[Fred-Barclay]

I tried... you can't self-approve your own PR 😞
It's a shame really - would be great to be able to enforce CI etc.

[kmk3]

@netblue30 on Feb 11:

Would definitely be nice if the committer could approve/merge as well.

Try it on this pull rq: #4920

If it doesn't work, I'm afraid we'll have to scrap "Require a pull request
before merging". Every one of you should be able to fully check in without a
second person approving it.

This is caused by the following sub-rule:

• Require approvals

When enabled, pull requests targeting a matching branch require a number of
approvals and no changes requested before they can be merged.

On my own PRs, the "Approve" and "Request changes" inputs are disabled and have
these messages on hover, respectively:

Pull request authors can’t approve their own pull request

Pull request authors can’t request changes on their own pull request

I think the logic of not allowing self-approvals is that if everyone could
approve their own PRs, then the rule wouldn't make much of a difference. It
would work the same way as the default, which is to allow merging without any
approval.

So in order for that to be possible, the rule has to be disabled.

[kmk3]

@Fred-Barclay on Feb 11:

I tried... you can't self-approve your own PR disappointed It's a shame
really - would be great to be able to enforce CI etc.

That's a separate setting:

• Require status checks to pass before merging

How about enabling it for all CI checks?

[rusty-snake]

Skipped checks (e.g. "Fix typo in README.md") are not considered passed checks by GH IIRC. This would mean we need to run the full CI for every change. (Well if GH does not likes unnecessary computation in actions they would ...)

[kmk3]

@rusty-snake on Feb 11:

Skipped checks (e.g. "Fix typo in README.md") are not considered passed
checks by GH IIRC. This would mean we need to run the full CI for every
change. (Well if GH does not likes unnecessary computation in actions they
would ...)

Indeed: kmk3#2

It hangs on all checks:

Some checks haven’t completed yet

5 expected checks

build-clang Expected — Waiting for status to be reported

[...]

Required statuses must pass before merging

All required statuses and check runs on this pull request must run
successfully to enable automatic merging.

That makes it rather useless...

[netblue30]

Removed the branch protection for now. So, full check in for everybody!