Who merges a PR v2.0

[fespinoza]

given that github now has protected branches, it's possible to block the merge of a PR, until CI passes and there is at least an approval of the changes for protected branches (develop and master are good candidates for this).

this behavior can be enabled or not in github, per repo.

So the question is:

• should we take that as a standardized practice with our repos?
• if so.. who merge the PR?
• if not.. do we have any changes for what we have already?

https://github.com/hyperoslo/playbook/blob/master/GIT_AND_GITHUB.md#who-merges-the-pull-request

@hyperoslo/everyone

[timkurvers]

I'd very much like to have this as a standard practice on all our repos. Not entirely sure whether we can set the amount of approvals required?

With regards to who merges: either creator or the approver would be fine I think. Given that approval by a 'person who knows stuff' means 'this is good to go', who clicks the actual button is relatively insignificant.

[gilstad]

@fespinoza As long as you developers like it I am all for it. I guess this forces code review on everything, and that is a good thing :)

Can the block be bypassed if needed? If only one developer is available and we need to deploy a critical fix (Like this week, when Vasiliy is on vacation).

In another company I worked in we introduced blocks similar to this, and in many cases it was just annoying and slow to wait for someone to approve. As far as I can remember there was a requirement of 2 devs to approve stuff, and that it too much. A lot of the developers really hated it, and several projects removed this blocks again.

[fespinoza]

you can choose those settings, so you can skip the checks if you need to.

the whole thing is more for a "someone else must see this PR before it gets merged"

[jgorset]

I like that this makes it easier to do the right thing, but I think it's important that we're still able to be pragmatic about it.

I remember how we didn't care who the reviewer was when we first started doing code reviews, and how quickly it devolved into sending code to random people because we had a rule that someone else had to see it. We forgot why we were doing code reviews in the first place, and how only someone intimately familiar with the whole that the code fits into and the problem it's trying to solve can do a good job of it. Now that we're clear on that, I think it's a good thing that it's easy to break that rule if it wouldn't serve its purpose anyway. If we're going to make that harder, I think we ought to have a pretty good reason for it.

[timkurvers]

@gilstad:

Can the block be bypassed if needed? If only one developer is available and we need to deploy a critical fix (Like this week, when Vasiliy is on vacation).

Every administrator can override and merge regardless of checks/requirements.

[jgorset]

So that would be all of us, then. 😛

[timkurvers]

It's an escape hatch, the button being red and dangerous. Which is good.

[sindrenm]

As with who actually merges the PR, I prefer – now that we have the approval system – that the submitter also merges. That way he/she is also responsible for potential bugs that occur from merging, i.e., having his/her code work together with the already existing and hopefully working code.

[fespinoza]

yeah i think so to @sindrenm .. also sometimes you just forget some detail, like re-generating the API documentation, etc

[sindrenm]

Always with the API documentation.

[sindrenm]

So, this just happened: https://github.com/hyperoslo/cellular/pull/27.

What would the correct flow be here? First, @jgorset requests come changes due to codestyle, which is great that we can do. Then @toothfairy fixes that. Great! But then what? I would at least assume that, if there are no other issues with codestyle or anything, @jgorset changes his review to “approved” before this gets merged? Here is looks as if a rejected PR gets merged, which feels weird to me.

And also there's still the discussion about whether @toothfairy himself should merge after it gets approval (I'm :+1: on that) or if someone else with access does it.

[jgorset]

I just want to make stuff happen without pressing too many buttons, yo.

[fespinoza]

i think the safest option is that given the presence of approvals, that the author of the PR merges it.

then if you requested changes in a review, should check again and approve if you think it's convenient. That repo is not yet configured to require a review to be able to merge, so this kind of conflicts can happen

[jgorset]

Why does it matter who merges it, though? Let's not get bogged down in rules that we don't need.

[sindrenm]

Just realized that having the submitter also merge removes the need for those pesky “WIP” PRs that everyone (should) hate so much.

[fespinoza]

this is my position: as mentioned in slack

the whole PR approval is about communication, to not to merge carelessly stuff the natural flow is to wait for a peer approval you should be able to override this in need, but it must feel as not the straightforward way

to me it doesn’t matter that much who merges the PR but have a clear sense of review and approval from my peers otherwise we become too easy on the merge button and things that should not be merged are merged because of that that can be avoided with communication, so having a workflow that enforces that is something i think is useful

it may not be feasible for a 100% of our repos, but in the more critical ones always more than 1 dev participates then it should be mandatory to have another’s view on the PR and as author highlight the intention and the why you need those changes and make the reviewer’s job easier and for your future self

the same thing with CI, you shouldn’t be able to merge unless CI passes and even i am agreeing with this rule, i am all against big corporation kind of bullshit

it’s the same of the usage of a linter like rubocop or es-lint if you configure it in your project and use editor tools like syntastic or the equivalents in atom/sublime/gatito you don’t need to argue about small style violations and all devs maintain a similar style (per repo at least)

[fespinoza]

close for inactivity