Hi
Committers,
Christian
and I want do discuss the following questions with you! :-)
(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.
(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?
(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.
(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.
What
do you think?
-Markus
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.![]()
