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.
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?
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.
- 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...
- 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...
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.
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.
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.)
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!
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/ . 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. :)
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.
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.
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?
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...
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".
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.
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?
Sorry if you got an email that felt unsolicited. The only email that was sent was to some users of Sibbell (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.
18 comments
[ 2.9 ms ] story [ 49.7 ms ] threadIs there anything you learned by building your tool that would be worth us building into PullApprove?
- "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.
- 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...
- 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!
Merge tomorrow 8am if tests passing etc.
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.
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.
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!
I wrote a bot of my own, which I have at 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. :)
Does no one else use this methodology on their teams?
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...
If you can't trust your own developers, you have bigger problems then "merge access".