Re: [jaxrs-dev] Committer Conventions

[Santiago Pericas-Geertsen <santiago.pericasgeertsen@xxxxxxxxxx>]

 

(1) Minimum Length of review period before merge / close of PR

 

We should define an aboslute minimum review period so all interested committers had a fair chance to review / vote on each PR before it gets finally merged / closed. Current proposal is one week. A shorter period makes it unnecessarily hard for some committers to review (e. g. when on business travel). A longer period reduces overall agility of the project a lot, particularly when subsequent PRs are interrelated / ontop of each other, hence slows down authors.

 The problem with 1 week is that someone could easily be out on vacation etc. for that period. How would you feel is something you care about is proposed and merged before you get back? I think in needs to be more than a week.

 

(2) May a comitter merge his own PRs (hence be also the assignee)?

 

If I merge my own PR, I might do not be as critical wrt to quality, IP checks, project rules, and so on, compared to the case where I merge other PRs. So it might be a good thing that "the red (green) button" is not pressed by myself -- just as if I would not be a committer. On the other hand, if I distrust myself, why do I have commit rights?

 Fine by me.

 

 

(3) May a committer review his own PRs?

 

Technically Githubs counts reviews only if the review tool was used. So if we need three committer reviews, this means three people pressing that particular button. But Github does not allow the contributing committer to review his own code. Not even another committer can add the originally authoring committer as a reviewer. I assume by good reason: The paranoia I outlined in (2). So shall we enforce that even a committer needs three other committers as reviewers, or do we want to allow committers an "implied +1", i. e. enforcing only TWO reviews in that case? Proposal: Allow to reduce reviewers from 3 to 2 for commiter-opened PRs as we trust committers do not start a PR overhastily.

 I think an “implied +1” makes sense.

 

(4) Shall we enforce Github code reviews technically?

 

Github can prevent us from merging if we have too few reviews. Do we want that? The proposal is: Yes.

 Sure.

— Santiago

From: Christian Kaltepoth [mailto:notifications@xxxxxxxxxx] 

Sent: Samstag, 31. März 2018 10:39
To: eclipse-ee4j/jaxrs-api
Cc: Markus KARG; Mention
Subject: Re: [eclipse-ee4j/jaxrs-api] First version of .travis.yml (#611)

 

Hi @mkarg,

thanks for your comment. Let me respond on a few things:

This PR was merged after just two (not three) positive reviews

Correct, there are only two reviews. But merging requires three committers agreeing on a change. In this case the PR author is also a committer and therefore I think that creating a pull request for a change can be defined as an implicit +1 vote by the author. Therefore, if a PR is created by a committer, only two additional reviews are required. Does this make sense?

after just two days of review period.

I agree that merging was performed too early. Sorry about that. I didn't check the date and actually thought the PR was open for review longer than just two days. Seems like my mind tricked me. Maybe because I started the first discussion about this about a week ago.

However, we talked about voting times back then on the mailing list and didn't agree on any specific period. Some people wrote that one week is fine, others preferred two weeks. I guess that was also the reason for @spericas to not include it in the wiki page. But we should definitely define something for that.

Is it ok to assign my own PRs to myself?

Good question. IMO this is fine. We defined the assignee as a committer who is "in charge". So my impression was that in case of a PR which was created by a committer, it would be fine if the author assigns the PR to himself. However, I agree that there are also good arguments for separating author and assignee. Maybe we should really discuss this on the list.

Is it ok to review my own PRs?

You cannot review your own pull request using GitHub. But if a committer creates a PR, we could count that as a review. At least in my opinion.

To sum up. Perhaps we should really discuss some of these things on the list.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

_______________________________________________
jaxrs-dev mailing list
jaxrs-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/jaxrs-dev