[VOTE] When in Doubt, Wait 24 Hours Before Merging

[Sheng Zha <zh...@apache.org>]

Hi,

In order to avoid having miscommunication and unaligned expectation, I'd
like to propose a lazy vote on a new rule for merging pull requests.
Specifically, for merging PRs, if there are open review comments and
changes afterwards didn’t address the comments, we should have a
grace-period of 24 hours for commenters to respond to the changes.

This rule should take effect on Feb. 6th if there's no objection. Thanks.

Bests,
Sheng

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Chris Olivier <cj...@apache.org>]

I assume you mean after a call of "Is everything ok to merge?"

There'd have to be some sort of trigger for ppl to know someone has started
the 24-hour timer, right?




On Thu, Feb 1, 2018 at 4:20 PM, Sheng Zha <zh...@apache.org> wrote:

> Hi,
>
> In order to avoid having miscommunication and unaligned expectation, I'd
> like to propose a lazy vote on a new rule for merging pull requests.
> Specifically, for merging PRs, if there are open review comments and
> changes afterwards didn’t address the comments, we should have a
> grace-period of 24 hours for commenters to respond to the changes.
>
> This rule should take effect on Feb. 6th if there's no objection. Thanks.
>
> Bests,
> Sheng
>

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Marco de Abreu <ma...@googlemail.com>]

I'm afraid that the approach starting the "timer" (I know, that's a bad
term) with the last commit does not really work out. There have been cases
in the past where a reviewer criticized something, but it was never
followed up by the contributor. As far as I understand your second
proposal, you would request the reviewer to check the pr every few days and
keep repeating themselves in order to remind the contributor until the
comment has been addressed?

I'd rather agree with Chris and his view that all comments have to be
addressed or (in case of no response from the reviewer after being
explicitly asked) a specific time has to pass in order to render a veto
invalid. With your proposal I'm afraid that, especially in big PRs, a veto
might get discarded without the reviewer actually being aware of it.

-Marco

Am 02.02.2018 2:07 nachm. schrieb "Sheng Zha" <sz...@gmail.com>:

> Thanks for the comments. I'm considering the vote failed given the veto,
> and will draft another proposal.
>
> Isabel, thanks for bringing it up. I will update the grace period to 72
> hours in a new proposal.
>
> Chris, regarding the trigger, I think it should just be based on the latest
> commit on the PR (I will explain more below), and then I can add a term to
> recommend not to merge PR if another request for changes is received during
> grace period.
>
> Nan, indeed I think we committers should avoid forcing a change through,
> and reasonable amount of explanation should be given. The difficulty (and
> blessing) is the lack of authority for performing such approval. The
> intended situation is when one committer approves and the other
> disapproves.
>
> Chris and Marco, given that everything is publicly accessible, I don't
> think committer or requester should be obliged to additional informing
> duty. On github, one would be automatically subscribed to the PR for any
> further changes if one makes a comment or review. I agree that it's the
> shared responsibility of the change requester and the merging committer's
> to drive consensus. I also agree that it would be courteous for a committer
> to ping commenter to reach consensus, and it's the right thing for me to
> do. Still, I think it should be the commenter's responsibility to follow up
> on the prior, potentially outdated comment when needed. Veto is preserved
> in that a committer can close a PR at any time, and can revert changes or
> vetoing releases that contain the change.
>
> Lastly, I'd like to use the remainder of this thread to drive towards
> consensus on the revision needed for the proposal. In that, I welcome both
> further discussion on related points and counter proposals. Thank you.
>
> -sz
>
> On Fri, Feb 2, 2018 at 7:43 AM, Chris Olivier <cj...@gmail.com>
> wrote:
>
> > -1 (binding) based upon:  It's not clear what triggers the timer or how a
> > veto is preserved.
> >
> > I'd like to argue that any comment on a PR by a committer should get some
> > sort of response, even if it is "bugger off", and the ability to veto
> must
> > be preserved (ie set review status as 'request changes')
> >
> > On Fri, Feb 2, 2018 at 6:18 AM, Isabel Drost-Fromm <is...@apache.org>
> > wrote:
> >
> >>
> >>
> >> Am 2. Februar 2018 01:20:31 MEZ schrieb Sheng Zha <zhasheng@apache.org
> >:
> >> >Specifically, for merging PRs, if there are open review comments and
> >> >changes afterwards didn’t address the comments, we should have a
> >> >grace-period of 24 hours for commenters to respond to the changes.
> >>
> >> Typical waiting period for lazy consensus would be 72 hours IIRC so ppl
> >> in other timezones/ on holiday get a chance to check as well.
> >>
> >> Isabel
> >>
> >> --
> >> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
> >>
> >
> >
>

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Sheng Zha <sz...@gmail.com>]

Thanks for the comments. I'm considering the vote failed given the veto,
and will draft another proposal.

Isabel, thanks for bringing it up. I will update the grace period to 72
hours in a new proposal.

Chris, regarding the trigger, I think it should just be based on the latest
commit on the PR (I will explain more below), and then I can add a term to
recommend not to merge PR if another request for changes is received during
grace period.

Nan, indeed I think we committers should avoid forcing a change through,
and reasonable amount of explanation should be given. The difficulty (and
blessing) is the lack of authority for performing such approval. The
intended situation is when one committer approves and the other disapproves.

Chris and Marco, given that everything is publicly accessible, I don't
think committer or requester should be obliged to additional informing
duty. On github, one would be automatically subscribed to the PR for any
further changes if one makes a comment or review. I agree that it's the
shared responsibility of the change requester and the merging committer's
to drive consensus. I also agree that it would be courteous for a committer
to ping commenter to reach consensus, and it's the right thing for me to
do. Still, I think it should be the commenter's responsibility to follow up
on the prior, potentially outdated comment when needed. Veto is preserved
in that a committer can close a PR at any time, and can revert changes or
vetoing releases that contain the change.

Lastly, I'd like to use the remainder of this thread to drive towards
consensus on the revision needed for the proposal. In that, I welcome both
further discussion on related points and counter proposals. Thank you.

-sz

On Fri, Feb 2, 2018 at 7:43 AM, Chris Olivier <cj...@gmail.com> wrote:

> -1 (binding) based upon:  It's not clear what triggers the timer or how a
> veto is preserved.
>
> I'd like to argue that any comment on a PR by a committer should get some
> sort of response, even if it is "bugger off", and the ability to veto must
> be preserved (ie set review status as 'request changes')
>
> On Fri, Feb 2, 2018 at 6:18 AM, Isabel Drost-Fromm <is...@apache.org>
> wrote:
>
>>
>>
>> Am 2. Februar 2018 01:20:31 MEZ schrieb Sheng Zha <zh...@apache.org>:
>> >Specifically, for merging PRs, if there are open review comments and
>> >changes afterwards didn’t address the comments, we should have a
>> >grace-period of 24 hours for commenters to respond to the changes.
>>
>> Typical waiting period for lazy consensus would be 72 hours IIRC so ppl
>> in other timezones/ on holiday get a chance to check as well.
>>
>> Isabel
>>
>> --
>> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
>>
>
>

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Chris Olivier <cj...@gmail.com>]

-1 (binding) based upon:  It's not clear what triggers the timer or how a
veto is preserved.

I'd like to argue that any comment on a PR by a committer should get some
sort of response, even if it is "bugger off", and the ability to veto must
be preserved (ie set review status as 'request changes')

On Fri, Feb 2, 2018 at 6:18 AM, Isabel Drost-Fromm <is...@apache.org>
wrote:

>
>
> Am 2. Februar 2018 01:20:31 MEZ schrieb Sheng Zha <zh...@apache.org>:
> >Specifically, for merging PRs, if there are open review comments and
> >changes afterwards didn’t address the comments, we should have a
> >grace-period of 24 hours for commenters to respond to the changes.
>
> Typical waiting period for lazy consensus would be 72 hours IIRC so ppl in
> other timezones/ on holiday get a chance to check as well.
>
> Isabel
>
> --
> Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.
>

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Isabel Drost-Fromm <is...@apache.org>]

Am 2. Februar 2018 01:20:31 MEZ schrieb Sheng Zha <zh...@apache.org>:
>Specifically, for merging PRs, if there are open review comments and
>changes afterwards didn’t address the comments, we should have a
>grace-period of 24 hours for commenters to respond to the changes.

Typical waiting period for lazy consensus would be 72 hours IIRC so ppl in other timezones/ on holiday get a chance to check as well.

Isabel

-- 
Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Chris Olivier <cj...@gmail.com>]

Also, if a committer clicked "request changes" as their review (which shows
near the CI build info), then they need to release that -- it can't be
merged -- a "request changes" to me means "-1".

On Thu, Feb 1, 2018 at 5:06 PM, Marco de Abreu <marco.g.abreu@googlemail.com
> wrote:

> Additionally, I'd like to propose to add that people with open requested
> changes actively have to be pinged and that the 24 hours only start after
> that ping. Was that part of your intention, Sheng?
>
> -Marco
>
> On Thu, Feb 1, 2018 at 4:29 PM, Nan Zhu <zh...@gmail.com> wrote:
>
> > +1, but do not understand why we merged PRs which was not completely
> > approved?
> >
> > On Thu, Feb 1, 2018 at 4:20 PM, Sheng Zha <zh...@apache.org> wrote:
> >
> > > Hi,
> > >
> > > In order to avoid having miscommunication and unaligned expectation,
> I'd
> > > like to propose a lazy vote on a new rule for merging pull requests.
> > > Specifically, for merging PRs, if there are open review comments and
> > > changes afterwards didn’t address the comments, we should have a
> > > grace-period of 24 hours for commenters to respond to the changes.
> > >
> > > This rule should take effect on Feb. 6th if there's no objection.
> Thanks.
> > >
> > > Bests,
> > > Sheng
> > >
> >
>

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Marco de Abreu <ma...@googlemail.com>]

Additionally, I'd like to propose to add that people with open requested
changes actively have to be pinged and that the 24 hours only start after
that ping. Was that part of your intention, Sheng?

-Marco

On Thu, Feb 1, 2018 at 4:29 PM, Nan Zhu <zh...@gmail.com> wrote:

> +1, but do not understand why we merged PRs which was not completely
> approved?
>
> On Thu, Feb 1, 2018 at 4:20 PM, Sheng Zha <zh...@apache.org> wrote:
>
> > Hi,
> >
> > In order to avoid having miscommunication and unaligned expectation, I'd
> > like to propose a lazy vote on a new rule for merging pull requests.
> > Specifically, for merging PRs, if there are open review comments and
> > changes afterwards didn’t address the comments, we should have a
> > grace-period of 24 hours for commenters to respond to the changes.
> >
> > This rule should take effect on Feb. 6th if there's no objection. Thanks.
> >
> > Bests,
> > Sheng
> >
>

Re: [VOTE] When in Doubt, Wait 24 Hours Before Merging

[Nan Zhu <zh...@gmail.com>]

+1, but do not understand why we merged PRs which was not completely
approved?

On Thu, Feb 1, 2018 at 4:20 PM, Sheng Zha <zh...@apache.org> wrote:

> Hi,
>
> In order to avoid having miscommunication and unaligned expectation, I'd
> like to propose a lazy vote on a new rule for merging pull requests.
> Specifically, for merging PRs, if there are open review comments and
> changes afterwards didn’t address the comments, we should have a
> grace-period of 24 hours for commenters to respond to the changes.
>
> This rule should take effect on Feb. 6th if there's no objection. Thanks.
>
> Bests,
> Sheng
>