Protect `master` and contribute through pull requests
fredrik-bakke opened this issue · 8 comments
(The following is paraphrased and expanded from a message I sent Jonathan.)
Since we are now 4 people contributing to the yoneda project, if everyone is comfortable with it, I think it would be a good idea to change the workflow to protecting the master branch and only commit changes to it through pull requests. "Protecting" the master branch means that it is not possible to commit to it other than by pull requests. This change would make it easier to avoid merging conflicts, would open room for quality assurance protocols (e.g. one can block pull requests that do not type check), and gives a chance to review code before commits are merged (this would for instance make it easier for me to give feedback related to style guidelines).
Of course, this does add some overhead to your current process as you will have to work in a different branch than the master branch and will have to keep this branch in sync with the master branch when you want your changes merged, in addition to having to create pull requests. However, when you get used to the new workflow, the overhead isn't too large, and I think the benefits outweigh this overhead.
I support this suggestion :)
Thanks for the suggestion @fredrik-bakke. This is all new to me but sounds very sensible. I'm aware that I have bad hygiene with my own commits, so I imagine this will help ;)
Should all four of us be administrators, or is it easier if just one person handles it? Aside from protecting the master branch and requiring pull requests are there any settings I need to pay attention to?
I think it is okay to have several administrators so that things are not blocked if someone is away. I would recommend also requiring the GitHub Action checks to pass before merging. You could also make a review required, but that's up to you if you want that enforced by GitHub.
Oh, another good thing is to forbid force-push to master.
I second what Nikolai has written. I don't think it is necessary to require reviews with the current state of the project, though. But I would definitively encourage it.
Under status checks I've found "Check formalizations" but not "Check with latest Rzk." Does that check happen automatically if I require "Check formalizations"?
There's also an option "Require branches to be up to date before merging" which seems reasonable to me, but I'm happy to take your advice.
I've forbidden force pushing but not required reviews.
"Check formalisations" is the name of the only job within the "Check with latest Rzk" workflow.
You can see (and possibly rename) it in .github/workflows/rzk.yml file.
"Require branches to be up to date" — yes, it is good to have this on, since if someone updates a dependent file (and merges in master), you want to check that submitted PR still works, so you want people to update the PR for that.
Okay. I'll do this now and close this issue.