Discuss pull request requirements / branch protection

[ptmerz]

We should come up with a policy on who is allowed to push to which branches, what needs to be fulfilled to allow PRs to be merged, who is allowed to approve, etc.

[ptmerz]

First thoughts would include

• Merging PR requires approval by at least one (two?) developers. Approving a PR means taking responsibility to review the code and taking responsibility for the decision to merge it!
• Merging PR requires CI to pass (non-negotiable, which allows to keep a clean commit history, which in turn allows to efficiently bisect when bugs are found)
• Only group members can create branches, outsider collaborators use Fork model (or should we all be doing that?)
• Nobody can push to master

Happy to discuss all of this though!

[SimonBoothroyd]

Agree with all of your suggestions.

[mattwthompson]

Generally agree, however some follow-ups

approval by at least one (two?) developers

I find myself sometimes requesting a second review as the first reviewer if a patch looks good but my confidence doesn't rise to a certain threshold (usually some specific technical detail I'm not familiar with but something else is). One should be fine but I think it should be ok to request a second

Approving a PR means taking responsibility to review the code and taking responsibility for the decision to merge it!

Related, should the reviewer be the one merging, or should the author be responsible for merging after approval (and all other checks passing)? I don't have a preference, I just want to make sure the responsibility is clear so a good patch isn't accidentally neglected.

Merging PR requires CI to pass

I agree that all tests must pass before merging, but does this apply to other status checks like linters/code coverage? And are we ok overriding this rule if i.e. we know everything is working but a single test in the array is failing for some unrelated reason?

should we all be doing forks?

I personally prefer everybody use forks but there usually isn't a practical difference. I think the main benefit of developing on feature branches of the main repo is that multiple developers can contribute to the same branch without adding each others' forks as remotes, which is a minor hassle.

Nobody can push to master

Agree; sometimes it's annoying to have tiny patches in PRs but it's worth the safety of not borking things

[SimonBoothroyd]

Related, should the reviewer be the one merging, or should the author be responsible for merging after approval (and all other checks passing)?

It's typically good GitHub manners to let the PR author merge 🙂

And are we ok overriding this rule if i.e. we know everything is working but a single test in the array is failing for some unrelated reason?

I think the only case where it might be permissible to allow failing tests is if they are also failing on master. However, my preference would be to require master be fixed, the fix merged to the feature branch, and then make sure the tests pass correctly before merge.

I agree that all tests must pass before merging, but does this apply to other status checks like linters/code coverage?

Definitely linting should be required to pass (except for the case of #41 were linting is a follow up PR to keep the PR clean). Codecov should probably be left to the discretion of the PR reviewer and author.

I personally prefer everybody use forks but there usually isn't a practical difference.

My preference would be internal developers branch rather than fork as there is a slightly less cognitive overhead 🙂

[ptmerz]

Agreeing with @SimonBoothroyd on all points here - I would definitely require tests & linting to pass (if test failures are unrelated, but don't happen on master, they must be transient - then I'd rather suggest rerunning to make sure that it's really unrelated).

[mattwthompson]

I was more thinking about cases like a single test failing to due HTTP or something, although those can in principle be fixed by re-running. Agree that failing tests on master should be addressed first!

[ptmerz]

I was more thinking about cases like a single test failing to due HTTP or something, although those can in principle be fixed by re-running. Agree that failing tests on master should be addressed first!

Yeah, for these cases I'd suggest requiring a re-run rather than overruling. We can revisit this if it turns out to be a major hassle.

[ptmerz]

This brings up the question of how we'd want to handle PRs that are at first failing tests, then get updated by additional commits, and then pass CI. If we want to keep a clean merge history, we would need to either squash-merge (i.e. make one commit out of the PR), or require PR owners to amend their commits locally and push --force back to the branch. I'd rather vote for the first option, but that might be unpractical for large PRs. Then again we should anyway discourage large PRs, so that might not be a problem.

[mattwthompson]

If a clean history is non-negotiable, squash-and-merging all PRs is the way to go imo

[ptmerz]

I guess it depends how we define "clean". I don't mind too much about linting errors. Tests not passing, on the other hand, is bad - that will make bisecting harder than necessary. So we might decide on a per-case basis.