I think there are at least two different things to discuss here. I don’t think it quite falls under governance. Here are the two things I see:
- What should we code review?
- What does GitHub allow us to do?
What should we code review?
The point of our process it to make sure our source code is in a good state on the
develop branch. We should be free to change our process if it’s getting in the way or isn’t enforceable.
I think we should:
A. Continuous integration changes: test that the CI works, but no review required. (Of course if it’s requested, we should get help!)
B. GitHub template updates: review, no testing.
C. Doc changes: code review and build doc.
D. Source code: code review and test. How much we test can be dependent on what the change is, what it might impact, and what we think we need to test to keep
develop in a good state. (In a distributed development team, it’s very, very disruptive when we can’t trust that the source code is in a good state.)
I do think we can move to different levels of testing depending on the change.
For continuous integration changes, we should be confident that the changes work before changing implementations.
What does GitHub allow us to do?
GitHub only gives us repo-level permissions on code review. GitHub has it so either all pull requests on the repo have to be approved or none. They also allow admins to merge without review, but by pressing a big red button.
In theory, I’m ok with no review here. But in practice, the difficulty with allowing admins to merge without review is that mistakes can happen. I’ve made them, I’ve seen almost everyone that’s contributed to the Stan and Math repo make them. It’s really easy to accidentally commit a change that’s unrelated and have things slip by the review process, which isn’t what is intended. (This is really annoying because this also applies to the GitHub templates for pull requests and issues too.)
There are at least a couple ways to handle this. Perhaps we could have the
stan-buildbot account automatically approve pull requests when it doesn’t touch source code. Or we can store the configuration in another repo outside of the Stan repo (that’s essentially what’s done now; it’s all visible through Jenkins directly, but harder to get to than the Jenkinsfiles).
I, personally, wouldn’t like some devs to have the authority to merge without review. I think from the outside, there will be some confusion as to who is allowed to do this and under what circumstances. I’d rather just avoid that confusion in the hopes that it prevents accidental breaking of Stan.