Merging conventions

[diazona]

I feel like I'm using discussions like a chat system since this is so minor 😅 but I figure why not, people may be interested to read this at some point... anyway, I figured before this gets too far we should coordinate how we merge PRs. I'm used to a workflow where the assignee (or if not set, the PR author) is the one responsible for merging, but PRs from anyone other than a maintainer (if that ever comes up) would need to be approved by a maintainer first. @sjlongland did you want to do it differently? E.g. if I make a PR you need to be the one to merge it and vice versa, or either of us as maintainers can just merge any non-draft PR at our discretion? I'm not picky about what we pick, just wanted to make sure we're on the same page to avoid confusion.

[sjlongland]

There's certainly an advantage of having the reviewer (whom is different to the author) responsible for merging… at work often the task then gets handed back to the author to "review" the merge to ensure everything got caught.

While the team is very small, this can be a big bottleneck though. One person away (e.g. illness or vacation -- at Imbil when the satellite link went out, we had practically no Internet access other than very sketchy LTE), and suddenly everything deadlocks.

So I wouldn't want to strictly enforce this in case for whatever reason we needed to push on with something. Certainly if more people come on board, we should re-visit this. We've got some good geographic diversity at the moment, so that helps a bit, but let's not put ourselves in a straightjacket too quickly. :-)

[diazona]

Yeah, it definitely makes sense not to strictly enforce anything. But I'm thinking we should still be clear on what should normally happen - for instance, a PR author should generally leave it to someone else to actually merge, and only if there's no response for some amount of time (maybe roughly a week?) then the author can go ahead and merge their own PR.

[sjlongland]

Yep agreed, so the ideal being that the author of a PR SHOULD NOT review it, as it's very easy for someone to exhibit blind-spots when reviewing their own code and miss mistakes that others may spot more easily. (See RFC2119 for definitions of SHOULD / MAY etc.) :-)

The author of a PR MAY review the PR if:

• sufficient time (i.e. >1 month, but this will vary between people, my forgettery works overtime) has passed since authorship for the person to have "cleared their head" regarding the specifics of the PR's implementation
• AND no one else is available to review the PR (the longer the time scale, the less likely this is to happen unless the work is very specialised).

I have had the above situation arise at my workplace, a highly specialised piece of code no one else wanted to touch, it sat there in the review column for several weeks and so I had cleared my head enough to review it objectively -- however this is rarely practical and should be avoided.

The reviewer SHOULD merge the PR on acceptance. In the event the reviewer cannot merge a PR, it is any other person with commit access MAY merge the PR provided at least one person has reviewed it and indicated their acceptance.

In the event a PR is merged, the merge commit author's acceptance of the PR is implied.

[diazona]

Makes sense, I'm on board with that. 👍

The only thing I'd be a little worried about is that one-month time period possibly being too long; to my mind, for a simple and low-stakes project like this, if a project maintainer is completely uncommunicative for a month, it's beyond merge-a-PR-without-review territory and almost getting into are-you-still-involved-in-this-project territory. But I guess that's not something we actually expect to happen, so it's probably not worth worrying about it prematurely. If either of our availability changes such that we might actually be leaving PRs hanging for a month, then we can discuss it at that time and make other arrangements as needed.

[diazona]

I enabled some settings in the branch protection rule for main that I think should require approval of a PR by someone other than the author before it gets merged, namely "Require approvals" and "Require approval of the most recent reviewable push". Not that there's a lack of trust or anything - I just figured, if we can use Github's automatic controls to make it harder to accidentally merge something that shouldn't be merged, we might as well. We'll see whether it works as intended; if not, we can just roll that change back.