Open Stack

On Tue, May 7, 2019 at 1:08 PM Jay Bryant <jungleboyj at gmail.com> wrote:
>
> All,
>
> Cinder has been working with the same unwritten rules for quite some time as well with minimal issues.
>
> I think the concerns about not having it documented are warranted.  We have had question about it in the past with no documentation to point to.  It is more or less lore that has been passed down over the releases.  :-)
>
> At a minimum, having this e-mail thread is helpful.  If, however, we decide to document it I think we should have it consistent across the teams that use the rule.  I would be happy to help draft/review any such documentation.

Chiming in to say the manila community adopted a review policy during
Stein release - most of the review policy was what we followed prior,
without explicitly writing them down:
https://docs.openstack.org/manila/latest/contributor/manila-review-policy.html.
Here's a snip/snap from that policy that is relevant to this
discussion:

<quote>
Previously, the manila core team informally enforced a code review
convention that each code change be reviewed and merged by reviewers
of different affiliations. This was followed because the OpenStack
Technical Committee used the diversity of affiliation of the core
reviewer team as a metric for maturity of the project. However, since
the Rocky release cycle, the TC has changed its view on the subject 3
4. We believe this is a step in the right direction.

While there is no strict requirement that two core reviewers accepting
a code change have different affiliations. Other things being equal,
we will continue to informally encourage organizational diversity by
having core reviewers from different organizations. Core reviewers
have the professional responsibility of avoiding conflicts of
interest.
</unquote>

>
> Jay
>
> On 5/4/2019 8:19 PM, Morgan Fainberg wrote:
>
>
>
> On Sat, May 4, 2019, 16:48 Eric Fried <openstack at fried.cc> wrote:
>>
>> (NB: I tagged [all] because it would be interesting to know where other
>> teams stand on this issue.)
>>
>> Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
>>
>> Summary:
>> - There is a (currently unwritten? at least for Nova) rule that a patch
>> should not be approved exclusively by cores from the same company. This
>> is rife with nuance, including but not limited to:
>>   - Usually (but not always) relevant when the patch was proposed by
>> member of same company
>>   - N/A for trivial things like typo fixes
>> - The issue is:
>>   - Should the rule be abolished? and/or
>>   - Should the rule be written down?
>>
>> Consensus (not unanimous):
>> - The rule should not be abolished. There are cases where both the
>> impetus and the subject matter expertise for a patch all reside within
>> one company. In such cases, at least one core from another company
>> should still be engaged and provide a "procedural +2" - much like cores
>> proxy SME +1s when there's no core with deep expertise.
>> - If there is reasonable justification for bending the rules (e.g. typo
>> fixes as noted above, some piece of work clearly not related to the
>> company's interest, unwedging the gate, etc.) said justification should
>> be clearly documented in review commentary.
>> - The rule should not be documented (this email notwithstanding). This
>> would either encourage loopholing or turn into a huge detailed legal
>> tome that nobody will read. It would also *require* enforcement, which
>> is difficult and awkward. Overall, we should be able to trust cores to
>> act in good faith and in the appropriate spirit.
>>
>> efried
>> .
>
>
> Keystone used to have the same policy outlined in this email (with much of the same nuance and exceptions). Without going into crazy details (as the contributor and core numbers went down), we opted to really lean on "Overall, we should be able to trust cores to act in good faith". We abolished the rule and the cores always ask for outside input when the familiarity lies outside of the team. We often also pull in cores more familiar with the code sometimes ending up with 3x+2s before we workflow the patch.
>
> Personally <non-core/non-leadership hat> I don't like the "this is an unwritten rule and it shouldn't be documented"; if documenting and enforcement of the rule elicits worry of gaming the system or being a dense some not read, in my mind (and experience) the rule may not be worth having. I voice my opinion with the caveat that every team is different. If the rule works, and helps the team (Nova in this case) feel more confident in the management of code, the rule has a place to live on. What works for one team doesn't always work for another.