Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discuss and decided on branch protection policies #322

Open
elbenfreund opened this issue Mar 4, 2020 · 5 comments
Open

Discuss and decided on branch protection policies #322

elbenfreund opened this issue Mar 4, 2020 · 5 comments
Assignees
Labels
Packaging/Meta Issues and Bugs not related to the code itself but its packaging.

Comments

@elbenfreund
Copy link
Contributor

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
Copy link
Member

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!

@DirkHoffmann DirkHoffmann added the Packaging/Meta Issues and Bugs not related to the code itself but its packaging. label Mar 4, 2020
@elbenfreund
Copy link
Contributor Author

elbenfreund commented Mar 4, 2020

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

👍 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
Copy link
Member

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
Copy link
Member

(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
Copy link
Contributor

rhertzog commented Aug 28, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Packaging/Meta Issues and Bugs not related to the code itself but its packaging.
Projects
None yet
Development

No branches or pull requests

5 participants