search

projecthamster / hamster-shell-extension

Shell extension for hamster
http://projecthamster.org
GNU General Public License v3.0
217 stars 92 forks source link

Discuss and decided on branch protection policies #322

Open elbenfreund opened 4 years ago

elbenfreund commented 4 years ago

Github provides an array of additional features to enforce quality assurance policies. While this can help increasing code quality and normalizing workflow, it can also slow things down. This issue is intended as an entry point about what policies appeal to us as developers and finding a consensus.

DirkHoffmann commented 4 years ago

The problem is for me that I do not know all the bells and whistles of github, and I did not find an executive summary of options and their consequences/implementation. As a matter of principle, I would apply KISS and be pragmatic: Rather count on quick reactions in rare case of errors than lengthy reviews and therefore delayed improvements. If there were a risk of abuse, this would be different; but I believe there isn't. So I attribute the solution/answer to @elbenfreund or @mwilck, who I believe are more experienced with these control mechanisms. But this should not prevent others from giving their input!

elbenfreund commented 4 years ago

Here are my suggestions (for masterand develop) for right now:

Require pull request reviews before merging

1 Review required. Right now, for this repo, there seem to be multiple people engaged with the actual code. so I have some hope things would go relatively fast. If not, we could always disable it.

Require status checks to pass before merging

I absolutly would turn this on. even if we do not have any status hooks set up :) So the discussion would be more about which checks to enforce.

signed commits

I think this is sensible, but can not judge whether or not ppl right now are signing their commits. In order to clear our PR backlog, I would opt to keep it disabled for the next few days/weeks and revisit.

Include administrators

:+1: Otherwise temptation is just too big to "just this one commit, because I really really know what I am doing and want to be done with it." Also less of an impression of double standards.

Restrict who can push to matching branches

disabled

Allow force pushes

disabled

Allow deletions

disabled

matthijskooijman commented 4 years ago

LGTM. Especially disallowing force-pushes is good, not even so much to guard against ill intentions, but just against honest mistakes (with potential big consequences).

As for signed commits - I personally not doing signing of commits (and none of the projects I have contributed to so far have asked for it), but it probably is a good idea.

matthijskooijman commented 4 years ago

(OTOH, I guess that commit signing might be mostly useful if there's also a whitelist of people that are allowed to sign commits so you can actually check integrity? Anyone can generate a keypair and sign commits, of course.)

rhertzog commented 4 years ago

I agree with most of the things proposed by @elbenfreund here but I would not require signed commits. It makes it cumbersome to have to explain to external contributors how to setup a GPG key, etc.

As for pull requests and review required, we can try it but I believe some middle ground is fine too, i.e. require pull request but let the PR submitter merge its own PR if nobody reviewed it within 14 days. Or the opposite, trust people to only push tiny/sane changes (e.g doc, typo, fixup) and let them open PR that require a review for bigger more invasive changes.