You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Michael Marshall <mm...@apache.org> on 2022/02/23 06:24:05 UTC
[DISCUSS] Dismiss Stale Code Reviews
Hi All,
In my recent PR to update the `.asf.yaml` to protect release branches,
I set the `dismiss_stale_reviews` to `true` for PRs targeting master
branch [0]. I mistakenly thought this setting would only dismiss PRs
updated by force. Instead, all approvals are dismissed when additional
commits are added to the PR. The GitHub feature is documented here
[1].
Since the PR changed the old setting, I want to bring awareness to the
change and determine our preferred behavior before changing the
setting again.
I think we should return to our old setting [2]. The GitHub PR history
clearly shows when a contributor/committer approved a PR. I feel that
it is up to the "merging" committer to give the final review of the
PR's approval history before merging. Further, when dismiss stale code
reviews is true, GitHub modifies previous approval "history" in the PR
making it look like a reviewer never approved the PR, which I find a
bit confusing.
Here is a sample PR where approvals were dismissed: [3].
Let me know how you think we should proceed.
Thanks,
Michael
[0] https://github.com/apache/pulsar/blob/master/.asf.yaml#L76
[1] https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule#creating-a-branch-protection-rule
[2] https://github.com/apache/pulsar/pull/14425
[3] https://github.com/apache/pulsar/pull/14409
Re: [DISCUSS] Dismiss Stale Code Reviews
Posted by Michael Marshall <mm...@apache.org>.
Closing the loop, we merged the PR to set `dismiss_stale_reviews` to
`false` two days ago.
Thanks,
Michael
On Wed, Feb 23, 2022 at 2:52 AM Li Li <ur...@gmail.com> wrote:
>
> +1
>
> > On Feb 23, 2022, at 4:23 PM, Guangning E <eg...@gmail.com> wrote:
> >
> > +1
> >
> >
> > Thanks,
> > Guangning
> >
> > Enrico Olivelli <eo...@gmail.com> 于2022年2月23日周三 16:01写道：
> >
> >> +1
> >>
> >> Enrico
> >>
> >> Il Mer 23 Feb 2022, 07:31 PengHui Li <pe...@apache.org> ha scritto:
> >>
> >>> +1
> >>>
> >>> Before I always thought it was Github added this new feature :)
> >>> Thanks for sharing the great knowledge.
> >>>
> >>> Penghui
> >>>
> >>> On Wed, Feb 23, 2022 at 2:24 PM Michael Marshall <mm...@apache.org>
> >>> wrote:
> >>>
> >>>> Hi All,
> >>>>
> >>>> In my recent PR to update the `.asf.yaml` to protect release branches,
> >>>> I set the `dismiss_stale_reviews` to `true` for PRs targeting master
> >>>> branch [0]. I mistakenly thought this setting would only dismiss PRs
> >>>> updated by force. Instead, all approvals are dismissed when additional
> >>>> commits are added to the PR. The GitHub feature is documented here
> >>>> [1].
> >>>>
> >>>> Since the PR changed the old setting, I want to bring awareness to the
> >>>> change and determine our preferred behavior before changing the
> >>>> setting again.
> >>>>
> >>>> I think we should return to our old setting [2]. The GitHub PR history
> >>>> clearly shows when a contributor/committer approved a PR. I feel that
> >>>> it is up to the "merging" committer to give the final review of the
> >>>> PR's approval history before merging. Further, when dismiss stale code
> >>>> reviews is true, GitHub modifies previous approval "history" in the PR
> >>>> making it look like a reviewer never approved the PR, which I find a
> >>>> bit confusing.
> >>>>
> >>>> Here is a sample PR where approvals were dismissed: [3].
> >>>>
> >>>> Let me know how you think we should proceed.
> >>>>
> >>>> Thanks,
> >>>> Michael
> >>>>
> >>>> [0] https://github.com/apache/pulsar/blob/master/.asf.yaml#L76
> >>>> [1]
> >>>>
> >>>
> >> https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule#creating-a-branch-protection-rule
> >>>> [2] https://github.com/apache/pulsar/pull/14425
> >>>> [3] https://github.com/apache/pulsar/pull/14409
> >>>>
> >>>
> >>
>
Re: [DISCUSS] Dismiss Stale Code Reviews
Posted by Li Li <ur...@gmail.com>.
+1
> On Feb 23, 2022, at 4:23 PM, Guangning E <eg...@gmail.com> wrote:
>
> +1
>
>
> Thanks,
> Guangning
>
> Enrico Olivelli <eo...@gmail.com> 于2022年2月23日周三 16:01写道：
>
>> +1
>>
>> Enrico
>>
>> Il Mer 23 Feb 2022, 07:31 PengHui Li <pe...@apache.org> ha scritto:
>>
>>> +1
>>>
>>> Before I always thought it was Github added this new feature :)
>>> Thanks for sharing the great knowledge.
>>>
>>> Penghui
>>>
>>> On Wed, Feb 23, 2022 at 2:24 PM Michael Marshall <mm...@apache.org>
>>> wrote:
>>>
>>>> Hi All,
>>>>
>>>> In my recent PR to update the `.asf.yaml` to protect release branches,
>>>> I set the `dismiss_stale_reviews` to `true` for PRs targeting master
>>>> branch [0]. I mistakenly thought this setting would only dismiss PRs
>>>> updated by force. Instead, all approvals are dismissed when additional
>>>> commits are added to the PR. The GitHub feature is documented here
>>>> [1].
>>>>
>>>> Since the PR changed the old setting, I want to bring awareness to the
>>>> change and determine our preferred behavior before changing the
>>>> setting again.
>>>>
>>>> I think we should return to our old setting [2]. The GitHub PR history
>>>> clearly shows when a contributor/committer approved a PR. I feel that
>>>> it is up to the "merging" committer to give the final review of the
>>>> PR's approval history before merging. Further, when dismiss stale code
>>>> reviews is true, GitHub modifies previous approval "history" in the PR
>>>> making it look like a reviewer never approved the PR, which I find a
>>>> bit confusing.
>>>>
>>>> Here is a sample PR where approvals were dismissed: [3].
>>>>
>>>> Let me know how you think we should proceed.
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> [0] https://github.com/apache/pulsar/blob/master/.asf.yaml#L76
>>>> [1]
>>>>
>>>
>> https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule#creating-a-branch-protection-rule
>>>> [2] https://github.com/apache/pulsar/pull/14425
>>>> [3] https://github.com/apache/pulsar/pull/14409
>>>>
>>>
>>
Re: [DISCUSS] Dismiss Stale Code Reviews
Posted by Guangning E <eg...@gmail.com>.
+1
Thanks,
Guangning
Enrico Olivelli <eo...@gmail.com> 于2022年2月23日周三 16:01写道：
> +1
>
> Enrico
>
> Il Mer 23 Feb 2022, 07:31 PengHui Li <pe...@apache.org> ha scritto:
>
> > +1
> >
> > Before I always thought it was Github added this new feature :)
> > Thanks for sharing the great knowledge.
> >
> > Penghui
> >
> > On Wed, Feb 23, 2022 at 2:24 PM Michael Marshall <mm...@apache.org>
> > wrote:
> >
> > > Hi All,
> > >
> > > In my recent PR to update the `.asf.yaml` to protect release branches,
> > > I set the `dismiss_stale_reviews` to `true` for PRs targeting master
> > > branch [0]. I mistakenly thought this setting would only dismiss PRs
> > > updated by force. Instead, all approvals are dismissed when additional
> > > commits are added to the PR. The GitHub feature is documented here
> > > [1].
> > >
> > > Since the PR changed the old setting, I want to bring awareness to the
> > > change and determine our preferred behavior before changing the
> > > setting again.
> > >
> > > I think we should return to our old setting [2]. The GitHub PR history
> > > clearly shows when a contributor/committer approved a PR. I feel that
> > > it is up to the "merging" committer to give the final review of the
> > > PR's approval history before merging. Further, when dismiss stale code
> > > reviews is true, GitHub modifies previous approval "history" in the PR
> > > making it look like a reviewer never approved the PR, which I find a
> > > bit confusing.
> > >
> > > Here is a sample PR where approvals were dismissed: [3].
> > >
> > > Let me know how you think we should proceed.
> > >
> > > Thanks,
> > > Michael
> > >
> > > [0] https://github.com/apache/pulsar/blob/master/.asf.yaml#L76
> > > [1]
> > >
> >
> https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule#creating-a-branch-protection-rule
> > > [2] https://github.com/apache/pulsar/pull/14425
> > > [3] https://github.com/apache/pulsar/pull/14409
> > >
> >
>
Re: [DISCUSS] Dismiss Stale Code Reviews
Posted by Enrico Olivelli <eo...@gmail.com>.
+1
Enrico
Il Mer 23 Feb 2022, 07:31 PengHui Li <pe...@apache.org> ha scritto:
> +1
>
> Before I always thought it was Github added this new feature :)
> Thanks for sharing the great knowledge.
>
> Penghui
>
> On Wed, Feb 23, 2022 at 2:24 PM Michael Marshall <mm...@apache.org>
> wrote:
>
> > Hi All,
> >
> > In my recent PR to update the `.asf.yaml` to protect release branches,
> > I set the `dismiss_stale_reviews` to `true` for PRs targeting master
> > branch [0]. I mistakenly thought this setting would only dismiss PRs
> > updated by force. Instead, all approvals are dismissed when additional
> > commits are added to the PR. The GitHub feature is documented here
> > [1].
> >
> > Since the PR changed the old setting, I want to bring awareness to the
> > change and determine our preferred behavior before changing the
> > setting again.
> >
> > I think we should return to our old setting [2]. The GitHub PR history
> > clearly shows when a contributor/committer approved a PR. I feel that
> > it is up to the "merging" committer to give the final review of the
> > PR's approval history before merging. Further, when dismiss stale code
> > reviews is true, GitHub modifies previous approval "history" in the PR
> > making it look like a reviewer never approved the PR, which I find a
> > bit confusing.
> >
> > Here is a sample PR where approvals were dismissed: [3].
> >
> > Let me know how you think we should proceed.
> >
> > Thanks,
> > Michael
> >
> > [0] https://github.com/apache/pulsar/blob/master/.asf.yaml#L76
> > [1]
> >
> https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule#creating-a-branch-protection-rule
> > [2] https://github.com/apache/pulsar/pull/14425
> > [3] https://github.com/apache/pulsar/pull/14409
> >
>
Re: [DISCUSS] Dismiss Stale Code Reviews
Posted by PengHui Li <pe...@apache.org>.
+1
Before I always thought it was Github added this new feature :)
Thanks for sharing the great knowledge.
Penghui
On Wed, Feb 23, 2022 at 2:24 PM Michael Marshall <mm...@apache.org>
wrote:
> Hi All,
>
> In my recent PR to update the `.asf.yaml` to protect release branches,
> I set the `dismiss_stale_reviews` to `true` for PRs targeting master
> branch [0]. I mistakenly thought this setting would only dismiss PRs
> updated by force. Instead, all approvals are dismissed when additional
> commits are added to the PR. The GitHub feature is documented here
> [1].
>
> Since the PR changed the old setting, I want to bring awareness to the
> change and determine our preferred behavior before changing the
> setting again.
>
> I think we should return to our old setting [2]. The GitHub PR history
> clearly shows when a contributor/committer approved a PR. I feel that
> it is up to the "merging" committer to give the final review of the
> PR's approval history before merging. Further, when dismiss stale code
> reviews is true, GitHub modifies previous approval "history" in the PR
> making it look like a reviewer never approved the PR, which I find a
> bit confusing.
>
> Here is a sample PR where approvals were dismissed: [3].
>
> Let me know how you think we should proceed.
>
> Thanks,
> Michael
>
> [0] https://github.com/apache/pulsar/blob/master/.asf.yaml#L76
> [1]
> https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule#creating-a-branch-protection-rule
> [2] https://github.com/apache/pulsar/pull/14425
> [3] https://github.com/apache/pulsar/pull/14409
>