You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@fineract.apache.org by Adam Monsen <am...@mifos.org> on 2026/07/21 16:44:14 UTC
dismiss stale reviews
When your PR is approved, do you then push additional commits? If so, why?
If a PR is approved, the next step is to merge it, not to make more
unapproved changes.
*I propose we enforce this by policy for the apache/fineract repo.*
When I'm reviewing a PR, I'd be surprised if it changes *after* I approve
it.
My proposal is here <https://github.com/apache/fineract/pull/6155>. With
that PR, additional post-approval commits require re-approval. This closes
a loophole for accidents or bad behavior.
+1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR or
discuss it here on-list.
Re: dismiss stale reviews
Posted by Adam Monsen <am...@mifos.org>.
Thanks everyone! I'll merge this
<https://github.com/apache/fineract/pull/6155> now.
Re: dismiss stale reviews
Posted by Abhishek Chaudhary <ab...@gmail.com>.
+1
On Wed, 22 Jul 2026 at 16:28, KRISHNA MEWARA <kr...@gmail.com>
wrote:
> +1
>
> On Wed, 22 Jul, 2026, 16:10 Linos T Darikai, <li...@gmail.com>
> wrote:
>
>> +1 I think this will help in review to avoid unnecessary confusion.
>>
>> On Wed, Jul 22, 2026 at 12:39 PM Tofunmi Oguntibeju <
>> paultofunmi01@gmail.com> wrote:
>>
>>> +1.
>>>
>>> I support this change. Dismissing stale approvals when new commits are
>>> pushed ensures reviews reflect the latest code, reduces the risk of
>>> outdated approvals being merged, and aligns with common GitHub branch
>>> protection best practices.
>>>
>>> On Tue, Jul 21, 2026 at 5:45 PM Adam Monsen <am...@mifos.org> wrote:
>>>
>>>> When your PR is approved, do you then push additional commits? If so,
>>>> why?
>>>>
>>>> If a PR is approved, the next step is to merge it, not to make more
>>>> unapproved changes.
>>>>
>>>> *I propose we enforce this by policy for the apache/fineract repo.*
>>>>
>>>> When I'm reviewing a PR, I'd be surprised if it changes *after* I
>>>> approve it.
>>>>
>>>> My proposal is here <https://github.com/apache/fineract/pull/6155>.
>>>> With that PR, additional post-approval commits require re-approval. This
>>>> closes a loophole for accidents or bad behavior.
>>>>
>>>> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR
>>>> or discuss it here on-list.
>>>>
>>>
Re: dismiss stale reviews
Posted by KRISHNA MEWARA <kr...@gmail.com>.
+1
On Wed, 22 Jul, 2026, 16:10 Linos T Darikai, <li...@gmail.com>
wrote:
> +1 I think this will help in review to avoid unnecessary confusion.
>
> On Wed, Jul 22, 2026 at 12:39 PM Tofunmi Oguntibeju <
> paultofunmi01@gmail.com> wrote:
>
>> +1.
>>
>> I support this change. Dismissing stale approvals when new commits are
>> pushed ensures reviews reflect the latest code, reduces the risk of
>> outdated approvals being merged, and aligns with common GitHub branch
>> protection best practices.
>>
>> On Tue, Jul 21, 2026 at 5:45 PM Adam Monsen <am...@mifos.org> wrote:
>>
>>> When your PR is approved, do you then push additional commits? If so,
>>> why?
>>>
>>> If a PR is approved, the next step is to merge it, not to make more
>>> unapproved changes.
>>>
>>> *I propose we enforce this by policy for the apache/fineract repo.*
>>>
>>> When I'm reviewing a PR, I'd be surprised if it changes *after* I
>>> approve it.
>>>
>>> My proposal is here <https://github.com/apache/fineract/pull/6155>.
>>> With that PR, additional post-approval commits require re-approval. This
>>> closes a loophole for accidents or bad behavior.
>>>
>>> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR
>>> or discuss it here on-list.
>>>
>>
Re: dismiss stale reviews
Posted by Linos T Darikai <li...@gmail.com>.
+1 I think this will help in review to avoid unnecessary confusion.
On Wed, Jul 22, 2026 at 12:39 PM Tofunmi Oguntibeju <pa...@gmail.com>
wrote:
> +1.
>
> I support this change. Dismissing stale approvals when new commits are
> pushed ensures reviews reflect the latest code, reduces the risk of
> outdated approvals being merged, and aligns with common GitHub branch
> protection best practices.
>
> On Tue, Jul 21, 2026 at 5:45 PM Adam Monsen <am...@mifos.org> wrote:
>
>> When your PR is approved, do you then push additional commits? If so, why?
>>
>> If a PR is approved, the next step is to merge it, not to make more
>> unapproved changes.
>>
>> *I propose we enforce this by policy for the apache/fineract repo.*
>>
>> When I'm reviewing a PR, I'd be surprised if it changes *after* I
>> approve it.
>>
>> My proposal is here <https://github.com/apache/fineract/pull/6155>. With
>> that PR, additional post-approval commits require re-approval. This closes
>> a loophole for accidents or bad behavior.
>>
>> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR
>> or discuss it here on-list.
>>
>
Re: dismiss stale reviews
Posted by Tofunmi Oguntibeju <pa...@gmail.com>.
+1.
I support this change. Dismissing stale approvals when new commits are
pushed ensures reviews reflect the latest code, reduces the risk of
outdated approvals being merged, and aligns with common GitHub branch
protection best practices.
On Tue, Jul 21, 2026 at 5:45 PM Adam Monsen <am...@mifos.org> wrote:
> When your PR is approved, do you then push additional commits? If so, why?
>
> If a PR is approved, the next step is to merge it, not to make more
> unapproved changes.
>
> *I propose we enforce this by policy for the apache/fineract repo.*
>
> When I'm reviewing a PR, I'd be surprised if it changes *after* I approve
> it.
>
> My proposal is here <https://github.com/apache/fineract/pull/6155>. With
> that PR, additional post-approval commits require re-approval. This closes
> a loophole for accidents or bad behavior.
>
> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR or
> discuss it here on-list.
>
Re: dismiss stale reviews
Posted by sujan kumar <su...@gmail.com>.
+1, this improves the review process and helps ensure only approved
changes are merged.
On Tue, 21 Jul 2026 at 22:43, Attila Budai <bu...@gmail.com>
wrote:
> +1
>
> On Tue, Jul 21, 2026, 7:00 PM Aman Mittal <am...@gmail.com>
> wrote:
>
>> +1 seems reasonable to me. Will this also affect PRs which has changed
>> requested by reviewer?
>>
>>
>> On Tue, Jul 21, 2026, 10:26 PM James Dailey <ja...@gmail.com>
>> wrote:
>>
>>> +1
>>>
>>>
>>> Sent from Gmail Mobile
>>>
>>> On Tue, Jul 21, 2026 at 9:45 AM Adam Monsen <am...@mifos.org> wrote:
>>>
>>>> When your PR is approved, do you then push additional commits? If so,
>>>> why?
>>>>
>>>> If a PR is approved, the next step is to merge it, not to make more
>>>> unapproved changes.
>>>>
>>>> *I propose we enforce this by policy for the apache/fineract repo.*
>>>>
>>>> When I'm reviewing a PR, I'd be surprised if it changes *after* I
>>>> approve it.
>>>>
>>>> My proposal is here <https://github.com/apache/fineract/pull/6155>.
>>>> With that PR, additional post-approval commits require re-approval. This
>>>> closes a loophole for accidents or bad behavior.
>>>>
>>>> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR
>>>> or discuss it here on-list.
>>>>
>>>
Re: dismiss stale reviews
Posted by Attila Budai <bu...@gmail.com>.
+1
On Tue, Jul 21, 2026, 7:00 PM Aman Mittal <am...@gmail.com>
wrote:
> +1 seems reasonable to me. Will this also affect PRs which has changed
> requested by reviewer?
>
>
> On Tue, Jul 21, 2026, 10:26 PM James Dailey <ja...@gmail.com>
> wrote:
>
>> +1
>>
>>
>> Sent from Gmail Mobile
>>
>> On Tue, Jul 21, 2026 at 9:45 AM Adam Monsen <am...@mifos.org> wrote:
>>
>>> When your PR is approved, do you then push additional commits? If so,
>>> why?
>>>
>>> If a PR is approved, the next step is to merge it, not to make more
>>> unapproved changes.
>>>
>>> *I propose we enforce this by policy for the apache/fineract repo.*
>>>
>>> When I'm reviewing a PR, I'd be surprised if it changes *after* I
>>> approve it.
>>>
>>> My proposal is here <https://github.com/apache/fineract/pull/6155>.
>>> With that PR, additional post-approval commits require re-approval. This
>>> closes a loophole for accidents or bad behavior.
>>>
>>> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR
>>> or discuss it here on-list.
>>>
>>
Re: dismiss stale reviews
Posted by Sandro Martínez <vi...@gmail.com>.
+1
On Tue, Jul 21, 2026, 11:58 Adam Monsen <am...@mifos.org> wrote:
> On Tue, Jul 21, 2026 at 9:59 AM Aman Mittal <am...@gmail.com>
> wrote:
>
>> +1 seems reasonable to me. Will this also affect PRs which has changed
>> requested by reviewer?
>>
>
> It would, yes, but a reviewer shouldn't approve a PR until all requested
> changes are complete. A reviewer can also revert an approval at any time
> (say if it was made in error).
>
> Note: We already have required_conversation_resolution: true
> <https://github.com/apache/fineract/blob/86edc6b19963e2e73ac5a2ad1926843f6832568f/.asf.yaml#L32>
> blocking the merge until conversations (questions, requested changes, etc.)
> are resolved. I see dismissing stale reviews as an additional enhancement
> along the same lines, codifying more of our community's definition of "when
> is a PR approved/ok/ready to merge?"
>
Re: dismiss stale reviews
Posted by Adam Monsen <am...@mifos.org>.
On Tue, Jul 21, 2026 at 9:59 AM Aman Mittal <am...@gmail.com>
wrote:
> +1 seems reasonable to me. Will this also affect PRs which has changed
> requested by reviewer?
>
It would, yes, but a reviewer shouldn't approve a PR until all requested
changes are complete. A reviewer can also revert an approval at any time
(say if it was made in error).
Note: We already have required_conversation_resolution: true
<https://github.com/apache/fineract/blob/86edc6b19963e2e73ac5a2ad1926843f6832568f/.asf.yaml#L32>
blocking the merge until conversations (questions, requested changes, etc.)
are resolved. I see dismissing stale reviews as an additional enhancement
along the same lines, codifying more of our community's definition of "when
is a PR approved/ok/ready to merge?"
Re: dismiss stale reviews
Posted by Aman Mittal <am...@gmail.com>.
+1 seems reasonable to me. Will this also affect PRs which has changed
requested by reviewer?
On Tue, Jul 21, 2026, 10:26 PM James Dailey <ja...@gmail.com> wrote:
> +1
>
>
> Sent from Gmail Mobile
>
> On Tue, Jul 21, 2026 at 9:45 AM Adam Monsen <am...@mifos.org> wrote:
>
>> When your PR is approved, do you then push additional commits? If so, why?
>>
>> If a PR is approved, the next step is to merge it, not to make more
>> unapproved changes.
>>
>> *I propose we enforce this by policy for the apache/fineract repo.*
>>
>> When I'm reviewing a PR, I'd be surprised if it changes *after* I
>> approve it.
>>
>> My proposal is here <https://github.com/apache/fineract/pull/6155>. With
>> that PR, additional post-approval commits require re-approval. This closes
>> a loophole for accidents or bad behavior.
>>
>> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR
>> or discuss it here on-list.
>>
>
Re: dismiss stale reviews
Posted by James Dailey <ja...@gmail.com>.
+1
Sent from Gmail Mobile
On Tue, Jul 21, 2026 at 9:45 AM Adam Monsen <am...@mifos.org> wrote:
> When your PR is approved, do you then push additional commits? If so, why?
>
> If a PR is approved, the next step is to merge it, not to make more
> unapproved changes.
>
> *I propose we enforce this by policy for the apache/fineract repo.*
>
> When I'm reviewing a PR, I'd be surprised if it changes *after* I approve
> it.
>
> My proposal is here <https://github.com/apache/fineract/pull/6155>. With
> that PR, additional post-approval commits require re-approval. This closes
> a loophole for accidents or bad behavior.
>
> +1 / 0 / -1 / thoughts? Please add a thumbs-up / thumbs-down to that PR or
> discuss it here on-list.
>