
Show HN: Require approval to merge GitHub pull requests - davegaeddert
https://pullapprove.com
======
justinlilly
This is neat! I'm excited to see things about the GitHub code review ecosystem
improving. I've long wanted to see "when tests pass and a reviewer :shipit:'s,
then go ahead and merge the code" bots.

I wrote a bot of my own, which I have at
[https://betterdiff.com/](https://betterdiff.com/) . The idea there is that
instead of having humans go through and give cursory reviews, you have a round
of automated review by robots (using normal, industry-standard tooling) and it
comments on the PR just like a reviewer would. I'd love to hear your thoughts.
:)

------
adrianmacneil
This is pretty cool. We built something similar internally at a previous
company - very handy to enforce good development practices, and ensure that no
unreviewed code is deployed to production for security purposes.

~~~
davegaeddert
Thanks! We started it for the same reason -- internal use. Felt like we might
as well turn it into a service so other people wouldn't have to build it too!

Is there anything you learned by building your tool that would be worth us
building into PullApprove?

~~~
adrianmacneil
Well, without looking past the landing page for your tool, some quick
thoughts:

\- "Read and write all public and private repository data" is a pretty serious
github auth request for people just wanting to kick the tires of your service.
Perhaps only ask for basic email address, then get additional permissions (or
allow manual setup) later?

\- Have you managed to prevent _all_ commit access to master (for example if
someone does a regular push to master without opening a pull request)? I'm not
super familiar with how github protected branches work, but it only mentions
preventing accidental force push. If there is any way to bypass this, it would
be nice to have an option which auto-reverts any unauthorized changes.

\- One pain point we found was where developers amended a pull request by
squashing all commits and force pushing to their PR branch. We found that this
was annoying having to go back and ask for another +1 from the team when the
actual diff hadn't changed. To fix this we stored the sha256 of the complete
diff along with the PR status, so that if a commit was amended but no code
changed, it was automatically approved.

\- On that note, there is quite a difference between people who want to use
this to encourage good development practices, and those who are using it for
security audit. For example, if I make a PR, and someone nitpicks my
indentation and gives a +1, then I amend my commit to fix the spacing, do I
need to bug that person for another code review, or just go ahead and merge?
In spirit my code has been reviewed already, but if you are trying to protect
against an owned laptop being used to deploy code into production, every
commit should be reviewed. I would recommend making this an option for each
team (whether to simply require approval for each PR, or every single commit).

\- Extra feature that would be cool: requiring multiple (or specific)
approvers if you touch a certain subset of files/folders in the repo.

~~~
davegaeddert
Awesome, thanks for the detailed feedback.

\- Great point on permissions. I'm certainly going to do some digging to see
how many people get turned away by that level of access. I could definitely
understand if people are cautious with that, as you were, because they should
be!

\- The prevention of commits to master all happens via GitHub's new protected
branches feature (they use Git hooks I believe). You're right, preventing
force push is part of it, but the other thing it can do is require status
checks (PullApprove is just another status check) on all commits to your base
branch -- meaning nothing can be directly committed to the base branch, but
should instead run through a pull request. Pretty interesting stuff:
[https://github.com/blog/2051-protected-branches-and-
required...](https://github.com/blog/2051-protected-branches-and-required-
status-checks)

\- Thanks for sharing this one, I definitely wouldn't have thought of that
without experiencing it myself.

\- I'm planning on building some settings towards this. The most basic one
giving an option that approval statuses go back to "pending" on new commits.
It could possibly get more specific, giving an option to ignore whitespace
changes etc...

\- Nice idea. I'll throw this in the hopper.

Let me know if you have any other thoughts!

------
josefdlange
Some cool stuff here, though I think a demonstration video or fancy animation
would be nice. Like someone else has mentioned, it's a little intimidating to
give full access just to see how it works -- you might convert more customers
if they can see how it works and some common use cases before they need to
commit the time to trying an integration.

~~~
davegaeddert
Thanks for the feedback, I totally agree. I'll see if we can work something up
that shows the whole flow -- from creating a pull request, to approving via
PullApprove, to merging.

------
huntleydavis
It would be cool too if you could set conditions on when to merge. I finish a
PR it's kind of late :

Merge tomorrow 8am if tests passing etc.

------
batoure
I used to think that this was a really weird missing feature in Github. Then
someone introduced me to true Gitflow using forked repos have used it with all
teams since. With a forked repo someone with read access can post a PR to the
originating repository without the ability to merge.

Does no one else use this methodology on their teams?

~~~
davegaeddert
Forked repos is absolutely one way to accomplish preventing merge privileges
(since it removes all push access). At least for us, our workflow and tooling
is much simpler if all developers work on the same repository.

Even in the case of forked repos, I think PullApprove can still have some
benefits. Ex. Someone forks and makes a pull request, but it needs to be
reviewed by two key developers before it should be merged. PullApprove could
help to formalize that process, requiring that both of those developers mark
it as "approved"...

Does that example apply to your workflow? Or is there anything else about your
process that a service like PullApprove could formalize? I'm looking forward
to hearing more about how other teams deal with some of these issues...

------
shadeless
Does it support a dynamic reviewer? In other words, instead of assigning a
person who approves pull requests, it would allow merging if at least one
other person with write access approved the code.

That would be the main use case for the company I work at, right now we're
doing that manually by tagging and waiting for the response.

~~~
davegaeddert
Currently there's two ways "approval" can be determined. 1) Approval by "any"
of the reviewers means it has passed. Ex. The 3 people with write-access get
chosen as the reviewers, and if any 1 of those 3 approve it, then it passes.
2) Approval by "all" of the reviewers means it has passed. Ex. Of the 3 people
chose as reviewers, all 3 would have to approve the PR for it to pass.

Does that make sense? I think #1 gets at the scenario that you're talking
about, but right now it doesn't automatically choose the reviewers based on
who has read/write/admin access. You would just choose those people manually
when you set up the repo and choose whether the rule is "anyone" can approve
it or "everyone" has to approve it.

~~~
istvanp
I think the scenario shadeless was describing is that the author of the PR
shouldn't be able to merge but anyone else with write access could. It would
be my ideal scenario as well. (You could of course bypass this by doing it in
Git if you really needed to.)

~~~
davegaeddert
Gotcha, so if three users have write access then when a PR is opened by one of
them, one of the other 2 users has to approve it?

If your scenario only has two users (you and one other person) then this flow
can already be accomplished (the two are both reviewers, and you set approval
required by "everyone" \-- so you approve your own PR and then the other user
has to also). If you've got more than two users, that's something we'll have
to add but it sounds like it's worth doing!

------
phlyingpenguin
I received an unsolicited email from these guys. The email does not get
subscribed to many things to keep volume low. Did they buy a bunch of
addresses to spam?

~~~
davegaeddert
Sorry if you got an email that felt unsolicited. The only email that was sent
was to some users of Sibbell ([https://sibbell.com](https://sibbell.com))
which is another GitHub-related service we built. We really weren't trying to
spam people, but thought they might have a genuine interest in another tool
based on GitHub.

------
blainesch
Oh look, another tool to get in the way. Of course "write" access means
"merge" access, they are the same thing. How about we try this thing called
trusting the developers.

If you can't trust your own developers, you have bigger problems then "merge
access".

~~~
akerl_
At risk of feeding the troll: the point, as made in their tag line, is that
you don't always want any individual with the ability to create a branch to be
able to merge to master, for a variety of compliance and stability reasons.
This is just a codification of the existing strategy used by all kinds of
teams, where a PR is submitted and approvals are solicited in the PR from
other team members before merge.

