Improvement proposal for jenkinsci/jenkins repo review process

[Oleg Nenashev]

Hello,

There are many open PRs in Jenkins core repo. Currently it's very hard to perform reviews, because for every PR you need to go into and to see the current status by going through the conversation. It would be great so somehow show current statuses in the PR list.

I propose to add the following labels:

• needs-review (dark yellow)
• needs-fix (orange)
  
• needs-testcase (light orange)
• needs-docs (ligh orange)
• ready-for-merge (violet)
• Indicates that the PR has passed the review (e.g. "+1s and no responses during several days")
  
• A merger can merge the PR & squash it

A similar approach is being used in Jenkins CERT team for a while. It seems to be effective.

The approach could be also used in plugins. In the long-term it may be possible to add overall PR status dashboards, which may be helpful for the project.

If there is no concerns, I'll go forward and add labels.

Best regards, Oleg

[Tom Fennelly]

I'm not sure about all of the labels, but +1 for the PR owner to be allowed (if they wish) perform a PR squash once the review has been completed.

I'm not even sure that was ever an agreed upon "rule" (thou shalt not squash), but I do know there are people for and against it and nobody is right (imo), so it should be the choice of the PR owner to do it or not (without getting badgered either way).

[Daniel Beck]

On 05.10.2015, at 13:51, Tom Fennelly <tom.fe...@gmail.com> wrote:

> I'm not even sure that was ever an agreed upon "rule" (thou shalt not squash), but I do know there are people for and against it and nobody is right (imo), so it should be the choice of the PR owner to do it or not (without getting badgered either way).

The PR owner probably should not rewrite the PR history as this will lose review comments AFAICT. And they generally cannot merge themselves.

[Tom Fennelly]

That was why I said "once the review is completed". 

[Jesse Glick]

On Mon, Oct 5, 2015 at 8:17 AM, Tom Fennelly <tom.fe...@gmail.com> wrote:
>> The PR owner probably should not rewrite the PR history as this will lose
>> review comments AFAICT. And they generally cannot merge themselves.
>
> That was why I said "once the review is completed".

IMO the commits in the PR itself should not be squashed in any case.
If the merger wishes to squash, they can commit an aggregate change to
`master` with a commit message including `(closes #1234)`, leaving the
PR branch with historical commits. At least that way we retain the
history of development so long as we continue to host on GitHub.
(AFAIK you cannot archive PR information as `git-notes` or whatever.)

Anyway I am afraid this flame-war-in-the-making is distracting from
the topic of the thread, which is PR labels. That seems a good idea to
me.

[nicolas de loof]

Agree. I don't like all those "merge PR #" commits in project history when they aren't required by some long term parallel work

--
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/CANfRfr1F6YRSt9O35JKFLqnnvcay9-Teg3zqPpziwTKHMW5Fdg%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Mark Waite]

I like the labels idea.  KostyaSha has been encouraging me to consider labels on the git plugin and git client plugin pull requests as a classification technique.  This seems like a good generalization (in addition to the plugin topic specific labels that Kostya and I have discussed).

Mark Waite

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJznFjjcG43CaUXpT3Rfz%3D-d2S1Nc0UTgtKj1052CSa%2Bzsg%40mail.gmail.com.

[Oleg Nenashev]

Anyway I am afraid this flame-war-in-the-making is distracting from the topic of the thread, which is PR labels. That seems a good idea to me.

Yes, it's for another discussion. In this thread I just propose the "ready-for-merge" status, which indicates that "there were reviewers from Jenkins contributors/users and they have +1s on the request". Even if the labels can be changed by jenkinsci/core team members right now, hopefully they will help others with reviews.

I like the labels idea.  KostyaSha has been encouraging me to consider labels on the git plugin and git client plugin pull requests as a classification technique.  This seems like a good generalization (in addition to the plugin topic specific labels that Kostya and I have discussed).

Yes, we could use the same approach in the plugins. I can teach Jenkins IRC Bot to setup the default labels when the list gets finalized.
If nobody puts -1, I'm going to create labels tomorrow to start the test drive

понедельник, 5 октября 2015 г., 18:56:21 UTC+3 пользователь Mark Waite написал:

[nicolas de loof]

I like the labels but dislike the +1 / -1 approach

imho a reviewer can make comments on the proposed changes, some can be pure enthusiasm but most of them will be to discuss some constructs or API use that looks incorrect, and just putting such a comment with explanation why this should be reviewed is enough to make it clear some action is required, no need to add a childish :-1: 

When the PR looks good, "LGTM" is a clear message for a completed review by a reviewer. 

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/fc7c7aac-32dd-44b2-a558-4f25b98c3d90%40googlegroups.com.

[Baptiste Mathus]

+1 for the labels too.

Well, Nicolas, IMO -1 can just be a summary of the comment (but not the only thing, granted), not necessarily offensive (though I can see where you're coming...).

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANMVJzmpGife1s%3DWSTWPOGkE_n02bkSP%2BmA3QKaLLX6LDcr9Gw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Baptiste <Batmat> MATHUS - http://batmat.net
Sauvez un arbre,
Mangez un castor !

[nicolas de loof]

If the comment is detailed enough then it doesn't add any value

See for sample answer I got for a PR on docker, which was rejected. Kind message and reference to project guidelines, but no :-1: or anything comparable -> https://github.com/docker/docker/pull/16794#issuecomment-145849500

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/CANWgJS6cx1w1rWPHUrnJTgHbMmzrW3NiXDkfRDFeYD50xdswxw%40mail.gmail.com.

[Oleg Nenashev]

I agree with Nicolas that :-1: is too strong. It may be used to provide a feedback regarding actions of jenkinsci/core (improper merge, direct feature commit, etc.), but it may discourage external contributors. "needs-fix" label plus a detailed explanation of the issue and expected actions should be enough. BTW, it's a topic for MAINTAINING.md or whatever.

The labels have been created. Hopefully I'll find some time to process PRs and to apply labels there.

вторник, 6 октября 2015 г., 18:53:42 UTC+3 пользователь nicolas de loof написал: