+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 <mmarsh...@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
>

Reply via email to