2 ms·

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

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 11y ago
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 11y ago
  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-status-checks 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!

adrianmacneil 11y ago

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 11y ago
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-status-checks 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!