search

cyberitsolutions / alloc-cli

A CLI that uses the alloc API
GNU Affero General Public License v3.0
1 stars 2 forks source link

Require 1 approved review before a merge can happen #14

Closed cjbayliss closed 5 years ago

cjbayliss commented 5 years ago

Github supposedly allows an admin to protect a branch and require X number of reviews before a merge can happen:

https://help.github.com/en/articles/merging-a-pull-request#required-reviews https://help.github.com/en/articles/about-protected-branches

We should consider setting this up to prevent accidental unreviewed merges.

cjbayliss commented 5 years ago

Better link: https://help.github.com/en/articles/enabling-required-reviews-for-pull-requests

alexlance commented 5 years ago

I don't think I have permission to make that change. The settings tab doesn't appear for me.

cjbayliss commented 5 years ago

Same here, I'll talk to @conz about it 🙂

cjbayliss commented 5 years ago

Right, I got @conz to make a second team called alloc-admin, and add us to it and then add the two repos as admin. We should now be able to edit settings.

Based on this guide I propose we set 'Protect this branch' for the master of both alloc and alloc-cli. Then 'Require pull request reviews before merging.' and set the number of reviews needed to 1. Then enable 'Dismiss stale pull request approvals when new commits are pushed.' and 'Include administrators.'

Does that sound sane to you @alexlance?

alexlance commented 5 years ago

I'm ... not ... sure. There's only two of us in here - if one or the other of us isn't available, then changes will be blocked. I think I prefer adopting a convention of requiring peer review, rather than adopting a mandatory requirement for it.

But I don't really mind - this repo is under the Cyber account, it should probably be Cyber's rules.

cjbayliss commented 5 years ago

Hm, ok. You have a good point.

if one or the other of us isn't available, then changes will be blocked.

I'm pretty sure if one of us is no longer involved or is inactive, the other should be able to go into the settings and uncheck 'Include administrators.' If admins can't do that, that defeats the purpose of having admins right? If admins actually can't do that then @conz should be able to uncheck it, and if @conz is no longer involved, we'd be out of luck anyway.

But I don't really mind - this repo is under the Cyber account, it should probably be Cyber's rules.

The only thing I can find is 'Project managers are expected to implement code review in their projects.' No details on how to implement the code review process, just that code review needs to happen. So I think whatever decision we make should be fine.

cjbayliss commented 5 years ago

@alexlance Do you mind if I close this issue (and it's sister #51)? you brought up a good point about there being too few of us for it to work. I figure if more people start contributing in the future we can make a new issue and re-discuss if that is what we feel like then. Sound ok?

alexlance commented 5 years ago

Yep go for it!

Maybe can revisit in the future if you think it will help things.

On Mon, Sep 02, 2019 at 07:41:49PM -0700, Christopher Bayliss wrote:

@alexlance Do you mind if I close this issue (and it's sister #51)? you brought up a good point about there being too few of us for it to work. I figure if more people start contributing in the future we can make a new issue and re-discuss if that is what we feel like then. Sound ok?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/cyberitsolutions/alloc-cli/issues/14#issuecomment-527282249