Proposal: Require Pull Requests for commits to Jenkins core

[Christopher Orr]

Hi all,

As raised previously on this mailing list
(https://groups.google.com/d/topic/jenkinsci-dev/d64UIypbzh0/discussion), and
discussed in the community meeting today
(http://meetings.jenkins-ci.org/jenkins-meeting/2016/jenkins-meeting.2016-01-06-19.01.html),
there is a proposal saying that Git commits should not be made to
Jenkins core without adhering to the common and useful practice of going
through peer code review, via GitHub Pull Requests.

That discussion concluded that there wasn't a concrete proposal to be
agreed (or disagreed) with, so it was decided to try and form a set of
guidelines that can be voted on in the next meeting.

Therefore, I present for your discussion:

1. All code and translation commits to Jenkins core
(https://github.com/jenkinsci/jenkins) must be proposed via a Pull
Request (PR).

2. While other non-code commits (e.g. changelog.html and Javadoc
addition/fixes) may be made directly, using a PR is still encouraged.
Similarly, release-related commits (which are scripted by Kohsuke,
AFAIK) are also not required to go through PR.

3. PRs can be merged by any core committer after at least one other
person has reviewed the code. i.e. it's against the spirit of the
proposal if committers immediately merge their own PRs; a reasonable
amount of time should be given for people to take a look at the code.

(I'm leaving this point intentionally vague; it's clear that this can be
gamed in 100 different ways, but we're proposing that code review should
be done *more often*, not exactly *how* those reviews should be done;
that process (if likely unwritten or unclear) exists already)

4. As this will result in an increase in the number of PRs, and there
already over 120 outstanding, PRs opened in the future should be
reviewed and merged within a certain time period (1 month?), otherwise
they should get the "proposed-for-close" label
(https://github.com/jenkinsci/jenkins/labels/proposed-for-close).

That last point isn't so clear to me. Should such labelled PRs then be
closed after a further month? We should also try and encourage more
people to volunteer to review PRs?

Now that we have the starting points for discussion, it's over to you :)

Regards,
Chris

[Jesse Glick]

On Wed, Jan 6, 2016 at 4:40 PM, Christopher Orr <ch...@orr.me.uk> wrote:
> PRs opened in the future should be
> reviewed and merged within a certain time period (1 month?), otherwise
> they should get the "proposed-for-close" label

I do not like the idea of closing a PR merely because no reviewers
have gotten to it yet.

[Manuel Jesús Recena Soto]

I agree with Jesse.

The rest of the rules seem reasonable.

Regards,

>
> --
> You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANfRfr2zUbEQRyrTFjsHG9VOQCHcn6Q%2BgVOTB-bbARh%2BFrTDRg%40mail.gmail.com.
> For more options, visit https://groups.google.com/d/optout.

[Christopher Orr]

Yeah, I can understand that.

Alternative proposals would be gratefully accepted, even if it is "PRs
may end up lying around for years, unmerged".

Regards,
Chris