Is 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.
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.
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".
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. :)
Merge tomorrow 8am if tests passing etc.
