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
