Discussion about Review then Commit (RTC)

[James Dailey <ja...@gmail.com>]

Devs -

We COULD have a more rigorous Review of code, prior to accepting a Commit
(RTC), but this may slow down development.

That said, there are some strong arguments for improving the quality of the
PRs and Commits.  This is, after all, a financial services software.

Some back and forth on this to share with the full community below, and
seeking more discussion, especially from those who are actively
committing.  Nothing has been decided.

So, Do we need to move to a more thorough RTC (review, then commit)?   Is
it sufficient for one person to review with a LGTM (looks good to me)?  How
do we ensure we follow our code review standards and suggested practices?
(https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide)


---- From Jdailey ---

At ApacheCON I met with a longstanding Apache member and discussed some of
the issues we've seen on the project.  I think some of the code quality
issues stem from JIRA issue tickets not being fully developed, and then PRs
being put through and allowed to be committed without proper and full
review before or after.

Our fineract wiki page - which I edited last in 2018 says:
"To support the committer's process, Fineract is adopting a lazy consensus
for changes. We trust Committers will use their powers for good and not
break the code."
https://cwiki.apache.org/confluence/display/FINERACT/Committer%27s+Zone

The process is essentially a "soft" Review then Commit (RTC), but there is
trust that the dev finds another *Fineract Committer* to review, and the
assumption that 72 hrs is enough time from someone else to review and
object if necessary.  It assumes good intent, which is nice.

On the other hand, I believe that I am actually seeing a pattern of mostly
Commit then Review, at least for some code changes, which in practice means
that the release manager is looking through all of the Commits when they
are trying to make a release.
@Aleksandar Vidakovic  - can you verify this is what you are doing?

So, how do we encourage more of the ethic of review? and better and better
quality code?   Do we need to go back to some formal voting of Commits?
 i.e. RTC whereby at least three Committers must agree on the change?   And
where a -1 is a veto?


--- from Greg Stein, Apache Member ----
* dev process "should" be discussed by the entire community on the dev@
list, rather than privately by a subset of the community

* I've found over the past couple decades that (1) review is sufficient
(ie. 3 reviews will grind the project to a halt). In the old (svn) days,
that would be a patch to the mailing list and a single "+1" or "lgtm"
(looks good to me) and the sender could then apply their patch. In GitHub
days, I'd suggest one person post a PR, and that a reviewer does the merge.
That gives you a record of two people approving the change. No self-merges
except for "Obvious Fixes" (eg. typos, changes to
comments/doc/readme/websites (not-code)).

--- from Aleks ----
.. I think what's mostly missing is a set of good practices that everyone
should have on the radar. We do have all those linters (Spotless) in place,
but there are some "patterns" (e. g. how to name classes and packages;
where to place them) that can slip through the cracks. Here's a list of
things that recently appeared on my radar (re-post from Slack):


   - how to edit Asciidoc files (don't bother adding your own line breaks)
   - sometimes it's not clear where to put certain classes (DTOs, entity
   classes, REST resource classes, Spring Java configuration, Spring
   configuration validation, exceptions, constants, de-/serializers, mappers,
   handlers, tasks, jobs)
   - nested folders: we already have quite some levels of folders/packages
   until we actually write code (e. g.
   "org.apache.fineract.portfolio.client.service" etc.); I would not recommend
   doing more folders beyond the "usual" ones; e. g. I would not create
   something like
   "org.apache.fineract.portfolio.client.service.service2.Service1Impl" and
   "org.apache.fineract.portfolio.client.service.service1.Service2Impl", but
   just make sure the classes are unique and put them on one level
   ("org.apache.fineract.portfolio.client.service.Service1Impl",
   "org.apache.fineract.portfolio.client.service.Service2Impl"
   - sometimes we have utility classes that are only used in one place (e.
   g. for Spring configuration); consider embedding them instead of creating
   separate files; this makes it easier for devs to differentiate important
   stuff from auxiliary structures
   - fix compile warnings as much as possible; we used to have actually
   only 1-2 warnings in fineract-client, because of the way how the code
   generator works; but recently we got 20-30, mostly related to generic
   types; we used to have a very strict compiler setting that treated those
   warnings like errors (would fail the build)
   - would be great if all contributors could configure their Github
   account with a proper GPG key to get verified tag associated to their
   commits


FYI: list above has no specific order... no claim that it's complete or
that all items are equally important to everyone... but having some of
those rules in place would make efforts to reduce boilerplate code and move
towards real modules certainly easier.

---

Re: Discussion about Review then Commit (RTC)

[Aleksandar Vidakovic <ch...@monkeysintown.com>]

... just wanted to add that not all our reviews in the past are soft...
some PRs have quite some lively discussions attached, depends on the
reviewer. But... not all reviews follow the same rules...

How about having a runbook for reviewers with maybe max 10
items/entries/rules (no science here... arbitrary number) and every time we
review we apply the same set of rules.

Note: some of this could be automated with ArchUnit, e. g. package and
class naming, package nesting, "size" of the PR (lines of code?) etc.

On Thu, 13 Oct 2022, 06:18 Robert Jakech, <ro...@fiter.io> wrote:

> I think we could aim for an hybrid approach:
> 1. At least one reviewer of a PR
> 2. At least one approver.
>
>
>
> On Thu, 13 Oct 2022 at 2:00 pm, James Dailey <ja...@gmail.com>
> wrote:
>
>> Devs -
>>
>> We COULD have a more rigorous Review of code, prior to accepting a Commit
>> (RTC), but this may slow down development.
>>
>> That said, there are some strong arguments for improving the quality of
>> the PRs and Commits.  This is, after all, a financial services software.
>>
>> Some back and forth on this to share with the full community below, and
>> seeking more discussion, especially from those who are actively
>> committing.  Nothing has been decided.
>>
>> So, Do we need to move to a more thorough RTC (review, then commit)?   Is
>> it sufficient for one person to review with a LGTM (looks good to me)?  How
>> do we ensure we follow our code review standards and suggested practices?
>> (https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide)
>>
>>
>> ---- From Jdailey ---
>>
>> At ApacheCON I met with a longstanding Apache member and discussed some
>> of the issues we've seen on the project.  I think some of the code quality
>> issues stem from JIRA issue tickets not being fully developed, and then PRs
>> being put through and allowed to be committed without proper and full
>> review before or after.
>>
>> Our fineract wiki page - which I edited last in 2018 says:
>> "To support the committer's process, Fineract is adopting a lazy
>> consensus for changes. We trust Committers will use their powers for good
>> and not break the code."
>> https://cwiki.apache.org/confluence/display/FINERACT/Committer%27s+Zone
>>
>> The process is essentially a "soft" Review then Commit (RTC), but there
>> is trust that the dev finds another *Fineract Committer* to review, and
>> the assumption that 72 hrs is enough time from someone else to review and
>> object if necessary.  It assumes good intent, which is nice.
>>
>> On the other hand, I believe that I am actually seeing a pattern of
>> mostly Commit then Review, at least for some code changes, which in
>> practice means that the release manager is looking through all of the
>> Commits when they are trying to make a release.
>> @Aleksandar Vidakovic  - can you verify this is what you are doing?
>>
>> So, how do we encourage more of the ethic of review? and better and
>> better quality code?   Do we need to go back to some formal voting of
>> Commits?  i.e. RTC whereby at least three Committers must agree on the
>> change?   And where a -1 is a veto?
>>
>>
>> --- from Greg Stein, Apache Member ----
>> * dev process "should" be discussed by the entire community on the dev@
>> list, rather than privately by a subset of the community
>>
>> * I've found over the past couple decades that (1) review is sufficient
>> (ie. 3 reviews will grind the project to a halt). In the old (svn) days,
>> that would be a patch to the mailing list and a single "+1" or "lgtm"
>> (looks good to me) and the sender could then apply their patch. In GitHub
>> days, I'd suggest one person post a PR, and that a reviewer does the merge.
>> That gives you a record of two people approving the change. No self-merges
>> except for "Obvious Fixes" (eg. typos, changes to
>> comments/doc/readme/websites (not-code)).
>>
>> --- from Aleks ----
>> .. I think what's mostly missing is a set of good practices that everyone
>> should have on the radar. We do have all those linters (Spotless) in place,
>> but there are some "patterns" (e. g. how to name classes and packages;
>> where to place them) that can slip through the cracks. Here's a list of
>> things that recently appeared on my radar (re-post from Slack):
>>
>>
>>    - how to edit Asciidoc files (don't bother adding your own line
>>    breaks)
>>    - sometimes it's not clear where to put certain classes (DTOs, entity
>>    classes, REST resource classes, Spring Java configuration, Spring
>>    configuration validation, exceptions, constants, de-/serializers, mappers,
>>    handlers, tasks, jobs)
>>    - nested folders: we already have quite some levels of
>>    folders/packages until we actually write code (e. g.
>>    "org.apache.fineract.portfolio.client.service" etc.); I would not recommend
>>    doing more folders beyond the "usual" ones; e. g. I would not create
>>    something like
>>    "org.apache.fineract.portfolio.client.service.service2.Service1Impl" and
>>    "org.apache.fineract.portfolio.client.service.service1.Service2Impl", but
>>    just make sure the classes are unique and put them on one level
>>    ("org.apache.fineract.portfolio.client.service.Service1Impl",
>>    "org.apache.fineract.portfolio.client.service.Service2Impl"
>>    - sometimes we have utility classes that are only used in one place
>>    (e. g. for Spring configuration); consider embedding them instead of
>>    creating separate files; this makes it easier for devs to differentiate
>>    important stuff from auxiliary structures
>>    - fix compile warnings as much as possible; we used to have actually
>>    only 1-2 warnings in fineract-client, because of the way how the code
>>    generator works; but recently we got 20-30, mostly related to generic
>>    types; we used to have a very strict compiler setting that treated those
>>    warnings like errors (would fail the build)
>>    - would be great if all contributors could configure their Github
>>    account with a proper GPG key to get verified tag associated to their
>>    commits
>>
>>
>> FYI: list above has no specific order... no claim that it's complete or
>> that all items are equally important to everyone... but having some of
>> those rules in place would make efforts to reduce boilerplate code and move
>> towards real modules certainly easier.
>>
>> ---
>>
>>
>>
>> --
> [image: Logo] <https://www.fiter.io>
> *Robert Jakech* *| CTO*
> *e:* robert@fiter.io
> *w:* fiter.io <http://www.fiter.io>
> *m:* +61458166091
> [image: twitter icon] <https://twitter.com/Fiter_io> [image: youtube icon]
> <https://www.youtube.com/channel/UCfWztdFtHI3PwaSLAEcwTNQ> [image:
> linkedin icon] <https://www.linkedin.com/company/fiter-io>
>

Re: Discussion about Review then Commit (RTC)

[Robert Jakech <ro...@fiter.io>]

I think we could aim for an hybrid approach:
1. At least one reviewer of a PR
2. At least one approver.



On Thu, 13 Oct 2022 at 2:00 pm, James Dailey <ja...@gmail.com> wrote:

> Devs -
>
> We COULD have a more rigorous Review of code, prior to accepting a Commit
> (RTC), but this may slow down development.
>
> That said, there are some strong arguments for improving the quality of
> the PRs and Commits.  This is, after all, a financial services software.
>
> Some back and forth on this to share with the full community below, and
> seeking more discussion, especially from those who are actively
> committing.  Nothing has been decided.
>
> So, Do we need to move to a more thorough RTC (review, then commit)?   Is
> it sufficient for one person to review with a LGTM (looks good to me)?  How
> do we ensure we follow our code review standards and suggested practices?
> (https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide)
>
>
> ---- From Jdailey ---
>
> At ApacheCON I met with a longstanding Apache member and discussed some of
> the issues we've seen on the project.  I think some of the code quality
> issues stem from JIRA issue tickets not being fully developed, and then PRs
> being put through and allowed to be committed without proper and full
> review before or after.
>
> Our fineract wiki page - which I edited last in 2018 says:
> "To support the committer's process, Fineract is adopting a lazy consensus
> for changes. We trust Committers will use their powers for good and not
> break the code."
> https://cwiki.apache.org/confluence/display/FINERACT/Committer%27s+Zone
>
> The process is essentially a "soft" Review then Commit (RTC), but there is
> trust that the dev finds another *Fineract Committer* to review, and the
> assumption that 72 hrs is enough time from someone else to review and
> object if necessary.  It assumes good intent, which is nice.
>
> On the other hand, I believe that I am actually seeing a pattern of mostly
> Commit then Review, at least for some code changes, which in practice means
> that the release manager is looking through all of the Commits when they
> are trying to make a release.
> @Aleksandar Vidakovic  - can you verify this is what you are doing?
>
> So, how do we encourage more of the ethic of review? and better and better
> quality code?   Do we need to go back to some formal voting of Commits?
>  i.e. RTC whereby at least three Committers must agree on the change?   And
> where a -1 is a veto?
>
>
> --- from Greg Stein, Apache Member ----
> * dev process "should" be discussed by the entire community on the dev@
> list, rather than privately by a subset of the community
>
> * I've found over the past couple decades that (1) review is sufficient
> (ie. 3 reviews will grind the project to a halt). In the old (svn) days,
> that would be a patch to the mailing list and a single "+1" or "lgtm"
> (looks good to me) and the sender could then apply their patch. In GitHub
> days, I'd suggest one person post a PR, and that a reviewer does the merge.
> That gives you a record of two people approving the change. No self-merges
> except for "Obvious Fixes" (eg. typos, changes to
> comments/doc/readme/websites (not-code)).
>
> --- from Aleks ----
> .. I think what's mostly missing is a set of good practices that everyone
> should have on the radar. We do have all those linters (Spotless) in place,
> but there are some "patterns" (e. g. how to name classes and packages;
> where to place them) that can slip through the cracks. Here's a list of
> things that recently appeared on my radar (re-post from Slack):
>
>
>    - how to edit Asciidoc files (don't bother adding your own line breaks)
>    - sometimes it's not clear where to put certain classes (DTOs, entity
>    classes, REST resource classes, Spring Java configuration, Spring
>    configuration validation, exceptions, constants, de-/serializers, mappers,
>    handlers, tasks, jobs)
>    - nested folders: we already have quite some levels of
>    folders/packages until we actually write code (e. g.
>    "org.apache.fineract.portfolio.client.service" etc.); I would not recommend
>    doing more folders beyond the "usual" ones; e. g. I would not create
>    something like
>    "org.apache.fineract.portfolio.client.service.service2.Service1Impl" and
>    "org.apache.fineract.portfolio.client.service.service1.Service2Impl", but
>    just make sure the classes are unique and put them on one level
>    ("org.apache.fineract.portfolio.client.service.Service1Impl",
>    "org.apache.fineract.portfolio.client.service.Service2Impl"
>    - sometimes we have utility classes that are only used in one place
>    (e. g. for Spring configuration); consider embedding them instead of
>    creating separate files; this makes it easier for devs to differentiate
>    important stuff from auxiliary structures
>    - fix compile warnings as much as possible; we used to have actually
>    only 1-2 warnings in fineract-client, because of the way how the code
>    generator works; but recently we got 20-30, mostly related to generic
>    types; we used to have a very strict compiler setting that treated those
>    warnings like errors (would fail the build)
>    - would be great if all contributors could configure their Github
>    account with a proper GPG key to get verified tag associated to their
>    commits
>
>
> FYI: list above has no specific order... no claim that it's complete or
> that all items are equally important to everyone... but having some of
> those rules in place would make efforts to reduce boilerplate code and move
> towards real modules certainly easier.
>
> ---
>
>
>
> --
[image: Logo] <https://www.fiter.io>
*Robert Jakech* *| CTO*
*e:* robert@fiter.io
*w:* fiter.io <http://www.fiter.io>
*m:* +61458166091
[image: twitter icon] <https://twitter.com/Fiter_io> [image: youtube icon]
<https://www.youtube.com/channel/UCfWztdFtHI3PwaSLAEcwTNQ> [image: linkedin
icon] <https://www.linkedin.com/company/fiter-io>