| [jaxrs-dev] Committer Conventions |
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] Hi @mkarg, thanks for your comment. Let me respond on a few things:
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?
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.
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.
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. — |