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 <me...@apache.org> on 2026/05/13 21:38:02 UTC
discuss: requiring approved PRs (FINERACT-2605)
I was working on improving our branch protections
<https://issues.apache.org/jira/browse/FINERACT-2605> today and was
forced to change our workflow a bit.
*Before*: commits directly to develop were allowed. If a PR is used, all
conversations in that PR must be resolved.
*After*: commits directly to develop are not allowed. All changes must
come from approved PRs, and all conversations in PRs must be resolved.
What do people think of this change? Not an official vote but: +1? -1?
Must all changes come from approved PRs? Note another side effect:
/Someone else/ must review and approve your PR. For example, PR #5846
<https://github.com/apache/fineract/pull/5846>.
Personally I'm +1 on .asf.yaml as it is now
<https://github.com/apache/fineract/blob/e3d698bc1f2d297ad124e10c2fbf2596f91bb00b/.asf.yaml>,
enforcing the "After" rule as stated above.
I think it's a reasonable set of rules to enforce since AFAIK we already
follow this in practice, but there may be edge cases I'm not thinking
of. When I browsed git commit history the only exceptions I found were
my own commits directly to develop.
If we decide we don't want the new rules, we'll need to remove these
lines from .asf.yaml:
required_conversation_resolution: true
required_pull_request_reviews:
required_approving_review_count: 1
I wanted only the first line to keep things the way they are, but using
rulesets I can't have it without the other two lines. And we must now
use rulesets. So I thought through the new way (requiring at least one
approved PR), and I think that'll probably be a Good Thing. So I'm still +1.
(Not to complicate things too much but I believe we can omit the
conversation resolution requirement and keep the approved reviews
requirement. So yet another option. I prefer to keep the conversation
resolution requirement.)
Feedback welcome. If nobody has any strong feelings about this I'll just
use lazy consensus and leave it as-is.
--
Adam Monsen
Software Engineer » Mifos Initiative
Release Manager » Apache Fineract
Author » Steadfast Self-Hosting
PGP key » 0xA9A14F22F57DA182
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by Paul <pc...@gmail.com>.
+1
From a business process perspective, should provide a more consistent path
for issues. This allows most anyone to understand at a glance the issues
needs.
--
Paul
On Wed, May 13, 2026, 5:03 PM Adam Monsen <me...@apache.org> wrote:
> I was working on improving our branch protections
> <https://issues.apache.org/jira/browse/FINERACT-2605> today and was
> forced to change our workflow a bit.
>
> *Before*: commits directly to develop were allowed. If a PR is used, all
> conversations in that PR must be resolved.
>
> *After*: commits directly to develop are not allowed. All changes must
> come from approved PRs, and all conversations in PRs must be resolved.
>
> What do people think of this change? Not an official vote but: +1? -1?
> Must all changes come from approved PRs? Note another side effect: *Someone
> else* must review and approve your PR. For example, PR #5846
> <https://github.com/apache/fineract/pull/5846>.
>
> Personally I'm +1 on .asf.yaml as it is now
> <https://github.com/apache/fineract/blob/e3d698bc1f2d297ad124e10c2fbf2596f91bb00b/.asf.yaml>,
> enforcing the "After" rule as stated above.
>
> I think it's a reasonable set of rules to enforce since AFAIK we already
> follow this in practice, but there may be edge cases I'm not thinking of.
> When I browsed git commit history the only exceptions I found were my own
> commits directly to develop.
>
> If we decide we don't want the new rules, we'll need to remove these lines
> from .asf.yaml:
>
> required_conversation_resolution: true
> required_pull_request_reviews:
> required_approving_review_count: 1
>
> I wanted only the first line to keep things the way they are, but using
> rulesets I can't have it without the other two lines. And we must now use
> rulesets. So I thought through the new way (requiring at least one
> approved PR), and I think that'll probably be a Good Thing. So I'm still +1.
>
> (Not to complicate things too much but I believe we can omit the
> conversation resolution requirement and keep the approved reviews
> requirement. So yet another option. I prefer to keep the conversation
> resolution requirement.)
>
> Feedback welcome. If nobody has any strong feelings about this I'll just
> use lazy consensus and leave it as-is.
>
> --
> Adam Monsen
> Software Engineer » Mifos Initiative
> Release Manager » Apache Fineract
> Author » Steadfast Self-Hosting
> PGP key » 0xA9A14F22F57DA182
>
>
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by Aleksandar Vidakovic <ch...@monkeysintown.com>.
+1
On Thu, May 14, 2026 at 6:40 AM Sandro Martínez <vi...@gmail.com>
wrote:
> +1
>>
>
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by Sandro Martínez <vi...@gmail.com>.
>
> +1
>
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by Francisco Cuandon <fc...@gmail.com>.
+1
On Wed, May 13, 2026 at 9:37 PM Attila Budai <bu...@gmail.com>
wrote:
> +1
>
> On Thu, May 14, 2026, 3:02 AM Aman Mittal <am...@gmail.com>
> wrote:
>
>> +1
>>
>> On Thu, 14 May, 2026, 5:30 am James Dailey, <jd...@apache.org> wrote:
>>
>>> +1
>>>
>>>
>>> Sent from Gmail Mobile
>>>
>>> On Wed, May 13, 2026 at 3:03 PM Adam Monsen <me...@apache.org> wrote:
>>>
>>>> I was working on improving our branch protections
>>>> <https://issues.apache.org/jira/browse/FINERACT-2605> today and was
>>>> forced to change our workflow a bit.
>>>>
>>>> *Before*: commits directly to develop were allowed. If a PR is used,
>>>> all conversations in that PR must be resolved.
>>>>
>>>> *After*: commits directly to develop are not allowed. All changes must
>>>> come from approved PRs, and all conversations in PRs must be resolved.
>>>>
>>>> What do people think of this change? Not an official vote but: +1? -1?
>>>> Must all changes come from approved PRs? Note another side effect: *Someone
>>>> else* must review and approve your PR. For example, PR #5846
>>>> <https://github.com/apache/fineract/pull/5846>.
>>>>
>>>> Personally I'm +1 on .asf.yaml as it is now
>>>> <https://github.com/apache/fineract/blob/e3d698bc1f2d297ad124e10c2fbf2596f91bb00b/.asf.yaml>,
>>>> enforcing the "After" rule as stated above.
>>>>
>>>> I think it's a reasonable set of rules to enforce since AFAIK we
>>>> already follow this in practice, but there may be edge cases I'm not
>>>> thinking of. When I browsed git commit history the only exceptions I found
>>>> were my own commits directly to develop.
>>>>
>>>> If we decide we don't want the new rules, we'll need to remove these
>>>> lines from .asf.yaml:
>>>>
>>>> required_conversation_resolution: true
>>>> required_pull_request_reviews:
>>>> required_approving_review_count: 1
>>>>
>>>> I wanted only the first line to keep things the way they are, but using
>>>> rulesets I can't have it without the other two lines. And we must now
>>>> use rulesets. So I thought through the new way (requiring at least one
>>>> approved PR), and I think that'll probably be a Good Thing. So I'm still +1.
>>>>
>>>> (Not to complicate things too much but I believe we can omit the
>>>> conversation resolution requirement and keep the approved reviews
>>>> requirement. So yet another option. I prefer to keep the conversation
>>>> resolution requirement.)
>>>>
>>>> Feedback welcome. If nobody has any strong feelings about this I'll
>>>> just use lazy consensus and leave it as-is.
>>>>
>>>> --
>>>> Adam Monsen
>>>> Software Engineer » Mifos Initiative
>>>> Release Manager » Apache Fineract
>>>> Author » Steadfast Self-Hosting
>>>> PGP key » 0xA9A14F22F57DA182
>>>>
>>>>
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by Attila Budai <bu...@gmail.com>.
+1
On Thu, May 14, 2026, 3:02 AM Aman Mittal <am...@gmail.com>
wrote:
> +1
>
> On Thu, 14 May, 2026, 5:30 am James Dailey, <jd...@apache.org> wrote:
>
>> +1
>>
>>
>> Sent from Gmail Mobile
>>
>> On Wed, May 13, 2026 at 3:03 PM Adam Monsen <me...@apache.org> wrote:
>>
>>> I was working on improving our branch protections
>>> <https://issues.apache.org/jira/browse/FINERACT-2605> today and was
>>> forced to change our workflow a bit.
>>>
>>> *Before*: commits directly to develop were allowed. If a PR is used,
>>> all conversations in that PR must be resolved.
>>>
>>> *After*: commits directly to develop are not allowed. All changes must
>>> come from approved PRs, and all conversations in PRs must be resolved.
>>>
>>> What do people think of this change? Not an official vote but: +1? -1?
>>> Must all changes come from approved PRs? Note another side effect: *Someone
>>> else* must review and approve your PR. For example, PR #5846
>>> <https://github.com/apache/fineract/pull/5846>.
>>>
>>> Personally I'm +1 on .asf.yaml as it is now
>>> <https://github.com/apache/fineract/blob/e3d698bc1f2d297ad124e10c2fbf2596f91bb00b/.asf.yaml>,
>>> enforcing the "After" rule as stated above.
>>>
>>> I think it's a reasonable set of rules to enforce since AFAIK we already
>>> follow this in practice, but there may be edge cases I'm not thinking of.
>>> When I browsed git commit history the only exceptions I found were my own
>>> commits directly to develop.
>>>
>>> If we decide we don't want the new rules, we'll need to remove these
>>> lines from .asf.yaml:
>>>
>>> required_conversation_resolution: true
>>> required_pull_request_reviews:
>>> required_approving_review_count: 1
>>>
>>> I wanted only the first line to keep things the way they are, but using
>>> rulesets I can't have it without the other two lines. And we must now
>>> use rulesets. So I thought through the new way (requiring at least one
>>> approved PR), and I think that'll probably be a Good Thing. So I'm still +1.
>>>
>>> (Not to complicate things too much but I believe we can omit the
>>> conversation resolution requirement and keep the approved reviews
>>> requirement. So yet another option. I prefer to keep the conversation
>>> resolution requirement.)
>>>
>>> Feedback welcome. If nobody has any strong feelings about this I'll just
>>> use lazy consensus and leave it as-is.
>>>
>>> --
>>> Adam Monsen
>>> Software Engineer » Mifos Initiative
>>> Release Manager » Apache Fineract
>>> Author » Steadfast Self-Hosting
>>> PGP key » 0xA9A14F22F57DA182
>>>
>>>
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by Aman Mittal <am...@gmail.com>.
+1
On Thu, 14 May, 2026, 5:30 am James Dailey, <jd...@apache.org> wrote:
> +1
>
>
> Sent from Gmail Mobile
>
> On Wed, May 13, 2026 at 3:03 PM Adam Monsen <me...@apache.org> wrote:
>
>> I was working on improving our branch protections
>> <https://issues.apache.org/jira/browse/FINERACT-2605> today and was
>> forced to change our workflow a bit.
>>
>> *Before*: commits directly to develop were allowed. If a PR is used, all
>> conversations in that PR must be resolved.
>>
>> *After*: commits directly to develop are not allowed. All changes must
>> come from approved PRs, and all conversations in PRs must be resolved.
>>
>> What do people think of this change? Not an official vote but: +1? -1?
>> Must all changes come from approved PRs? Note another side effect: *Someone
>> else* must review and approve your PR. For example, PR #5846
>> <https://github.com/apache/fineract/pull/5846>.
>>
>> Personally I'm +1 on .asf.yaml as it is now
>> <https://github.com/apache/fineract/blob/e3d698bc1f2d297ad124e10c2fbf2596f91bb00b/.asf.yaml>,
>> enforcing the "After" rule as stated above.
>>
>> I think it's a reasonable set of rules to enforce since AFAIK we already
>> follow this in practice, but there may be edge cases I'm not thinking of.
>> When I browsed git commit history the only exceptions I found were my own
>> commits directly to develop.
>>
>> If we decide we don't want the new rules, we'll need to remove these
>> lines from .asf.yaml:
>>
>> required_conversation_resolution: true
>> required_pull_request_reviews:
>> required_approving_review_count: 1
>>
>> I wanted only the first line to keep things the way they are, but using
>> rulesets I can't have it without the other two lines. And we must now
>> use rulesets. So I thought through the new way (requiring at least one
>> approved PR), and I think that'll probably be a Good Thing. So I'm still +1.
>>
>> (Not to complicate things too much but I believe we can omit the
>> conversation resolution requirement and keep the approved reviews
>> requirement. So yet another option. I prefer to keep the conversation
>> resolution requirement.)
>>
>> Feedback welcome. If nobody has any strong feelings about this I'll just
>> use lazy consensus and leave it as-is.
>>
>> --
>> Adam Monsen
>> Software Engineer » Mifos Initiative
>> Release Manager » Apache Fineract
>> Author » Steadfast Self-Hosting
>> PGP key » 0xA9A14F22F57DA182
>>
>>
Re: discuss: requiring approved PRs (FINERACT-2605)
Posted by James Dailey <jd...@apache.org>.
+1
Sent from Gmail Mobile
On Wed, May 13, 2026 at 3:03 PM Adam Monsen <me...@apache.org> wrote:
> I was working on improving our branch protections
> <https://issues.apache.org/jira/browse/FINERACT-2605> today and was
> forced to change our workflow a bit.
>
> *Before*: commits directly to develop were allowed. If a PR is used, all
> conversations in that PR must be resolved.
>
> *After*: commits directly to develop are not allowed. All changes must
> come from approved PRs, and all conversations in PRs must be resolved.
>
> What do people think of this change? Not an official vote but: +1? -1?
> Must all changes come from approved PRs? Note another side effect: *Someone
> else* must review and approve your PR. For example, PR #5846
> <https://github.com/apache/fineract/pull/5846>.
>
> Personally I'm +1 on .asf.yaml as it is now
> <https://github.com/apache/fineract/blob/e3d698bc1f2d297ad124e10c2fbf2596f91bb00b/.asf.yaml>,
> enforcing the "After" rule as stated above.
>
> I think it's a reasonable set of rules to enforce since AFAIK we already
> follow this in practice, but there may be edge cases I'm not thinking of.
> When I browsed git commit history the only exceptions I found were my own
> commits directly to develop.
>
> If we decide we don't want the new rules, we'll need to remove these lines
> from .asf.yaml:
>
> required_conversation_resolution: true
> required_pull_request_reviews:
> required_approving_review_count: 1
>
> I wanted only the first line to keep things the way they are, but using
> rulesets I can't have it without the other two lines. And we must now use
> rulesets. So I thought through the new way (requiring at least one
> approved PR), and I think that'll probably be a Good Thing. So I'm still +1.
>
> (Not to complicate things too much but I believe we can omit the
> conversation resolution requirement and keep the approved reviews
> requirement. So yet another option. I prefer to keep the conversation
> resolution requirement.)
>
> Feedback welcome. If nobody has any strong feelings about this I'll just
> use lazy consensus and leave it as-is.
>
> --
> Adam Monsen
> Software Engineer » Mifos Initiative
> Release Manager » Apache Fineract
> Author » Steadfast Self-Hosting
> PGP key » 0xA9A14F22F57DA182
>
>