Re: Code review requirements

I'd like to cite our review-poilcy [1]

The Maven project has no strict review policy, but the following 
practices established:

  * /Commit Then Review (CTR)/for trivial/simple changes
  * /Review Then Commit (RTC)/for complex changes and changes against
    release branches of past releases (e.g.|maven-3.8.x|branch)

Committers are invited to also request reviews for trivial changes, 
because every review can increase code quality.

[1] https://maven.apache.org/developers/conventions/git.html#review-policy


Am 15.05.2026 um 18:00 schrieb Romain Manni-Bucau:
> The question is really review then commit or the old style commit then
> review.
> Last years prove RTC has value due to auto motions, even for releases (some
> projects adoptée it more than t'en years ago).
>
> While for releases I dont care much, for code acceptable not encouraging
> automerge without previous sibling review sounds better and saner for asf
> (security, community, quality, discussion wide).
>
>
> Romain Manni-Bucau
> @rmannibucau<https://x.com/rmannibucau>  | .NET Blog
> <https://dotnetbirdie.github.io/>  | Blog<https://rmannibucau.github.io/>  | Old
> Blog<http://rmannibucau.wordpress.com>  | Github
> <https://github.com/rmannibucau>  | LinkedIn
> <https://www.linkedin.com/in/rmannibucau>  | Book
> <https://www.packtpub.com/en-us/product/java-ee-8-high-performance-9781788473064>
> Javaccino founder (Java/.NET service - contact via linkedin)
>
> Le ven. 15 mai 2026, 16:41, Slawomir Jaranowski<[email protected]>  a
> écrit :
>
>> Hi,
>>
>>  From me also -1
>>
>> Agree with Tamás
>>
>> and more:
>>   - release process and tool are not ready - if I'm misses about it,
>> please try do release first in new way without write to default
>> branch, next we can talk
>>   - I can create a fake account on on GitHub and switching between it -
>> one for create PR and one for approve
>>   - we have a vote process where artifact and commits are checked
>> before publishing
>>   - you can check reproducible build during vote
>>   - we have protected branches so force push with history override is
>> disabled,
>>   - all commits are logged on public ML
>>
>>
>>
>> On Fri, 15 May 2026 at 16:08, Tamás Cservenák<[email protected]>  wrote:
>>> -1 to REQUIRE this
>>> +1 to ASK for this (on case-by-case basis)
>>>
>>> Our velocity is already affected by slow responses, just look at votes
>>> for example, and there have been PRs sitting since years.
>>>
>>> I find this more like a "let's shoot ourselves in our foot" thing,
>>> given past experience, to bring ourselves to a grinding halt.
>>>
>>> See here for example
>>> https://github.com/apache/maven-site/pull/321
>>>
>>> Thanks
>>> T
>>>
>>> On Fri, 15 May 2026 at 14:25, Romain Manni-Bucau<[email protected]>
>> wrote:
>>>> Makes a lot of sense to me, +1
>>>>
>>>>
>>>> Romain Manni-Bucau
>>>> @rmannibucau<https://x.com/rmannibucau>  | .NET Blog
>>>> <https://dotnetbirdie.github.io/>  | Blog <
>> https://rmannibucau.github.io/> | Old
>>>> Blog<http://rmannibucau.wordpress.com>  | Github
>>>> <https://github.com/rmannibucau>  | LinkedIn
>>>> <https://www.linkedin.com/in/rmannibucau>  | Book
>>>> <
>> https://www.packtpub.com/en-us/product/java-ee-8-high-performance-9781788473064
>>>> Javaccino founder (Java/.NET service - contact via linkedin)
>>>>
>>>> Le ven. 15 mai 2026, 12:47, Elliotte Rusty Harold<[email protected]>
>> a
>>>> écrit :
>>>>
>>>>> I'vd noticed that on at least some (maybe all?) of our repos PRs can
>>>>> be and are being merged without any approving code reviews. We have
>>>>> enough active developers that this shouldn't be necessary. This feels
>>>>> increasingly important given the active state sponsored supply chain
>>>>> attacks on many open source projects.
>>>>>
>>>>> We could add something like this to .asf.yaml to guarantee code
>> reviews:
>>>>>    protected_branches:
>>>>>      main:
>>>>>        required_status_checks:
>>>>>          # strict means "Require branches to be up to date before
>> merging".
>>>>>          strict: true
>>>>>        required_pull_request_reviews:
>>>>>          require_last_push_approval: true
>>>>>          required_approving_review_count: 1
>>>>>
>>>>> Contrary to what has been asserted in the past, this is not a veto.
>> It
>>>>> does not require authors to get approval from all reviewers, or
>>>>> prevent merging PRs where one or more reviewers have requested
>>>>> changes. It simply requires one other person to approve the PR.
>> That's
>>>>> what we do anyway 90%+ of the time and should be a low enough bar to
>>>>> clear for anything important.
>>>>>
>>>>> --
>>>>> Elliotte Rusty Harold
>>>>> [email protected]
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail:[email protected]
>>>>> For additional commands, e-mail:[email protected]
>>>>>
>>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail:[email protected]
>>> For additional commands, e-mail:[email protected]
>>>
>>
>> --
>> Sławomir Jaranowski
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail:[email protected]
>> For additional commands, e-mail:[email protected]
>>
>>