[Avocado-devel] Review and merge rules

[Lukáš Doktor]

Dne 04. 12. 19 v 18:34 Plamen Dimitrov napsal(a):
> Hi Cleber,
> 
> On 12/4/19 7:18 PM, Cleber Rosa wrote:
>> On Wed, Dec 04, 2019 at 03:05:55PM +0200, Plamen Dimitrov wrote:
>>> Hi all,
>>>
>>> I am fairly new to the avocado community (about a year or so) and I am not
>>> sure so far I was clear on the reviewing and merging authorization that is
>>> employed in all avocado repositories. I was left with the apparently false
>>> impression that new members of the community can only comment on newer PR-s
>>> and nothing else and merging a PR would require two reviews from authorized
>>> members. However, recently I noticed that we can also review open pull
>>> requests which I didn't know before. Is this true or just wrongly enabled
>>> github switch? If so are there formal rules who can review/merge/comment
>>> anywhere to be found in the official avocado documentation or is it a standard
>>> workflow with the only rules that:
>> Hi Plamen,
>>
>> In fact the general policies used vary between the various Avocado
>> repositories.  For instance, on Avocado-VT, the "two reviews" rule
>> apply, while in Avocado we don't have such a strict requirement on
>> the number of reviews.
> 
> I see, it makes sense now and definitely clears things up.
> 
>>> 1) two reviews are needed for merge for everyone
>> This is general rules for Avocado:
>>
>>   https://avocado-framework.readthedocs.io/en/73.0/guides/contributor/chapters/how.html#code-review
>>
>> It may be that GitHub prevents non-contributors from submitting formal
>> "Approved" reviews.  I myself consider a comment with a "Reviewed,
>> LGTM to me" just as good.
> 
> I had a bit of experience today from this and it seems GitHub allows
> official reviews/approvals from contributors for sure.
> 

IIRC github distinguishes between new-commers and people who contributed several commits. It makes sense since github is a world-wide platform so it makes it easy for the majority.

>> For Avocado-VT I'm mentally aware of the "two reviews" rule, but I
>> can't find that in the docs.
>>
>>> 2) everyone can review new pull requests
>> With regards to reviews, anyone *should* be able to review PRs.  I'd
>> consider the opposite to be a badly configured GitHub switch.  I don't
>> think we have that explicitly stated anywhere.  I have to say I took
>> that for granted.
>>
>> Most importantly IMO is: do you think there's room for improvements
>> here?  If so, what/how?
>>
> 
> If you are referring to the documentation state of this workflow, then I
> noticed that indeed the avocado situation on the matter has been well
> explained on the above link. Perhaps a single extra sentence explicitly
> encouraging reviews from contributors would make both points above clear.
> In the case of Avocado VT, I understand that differences in review process
> and overall contribution workflow differs mostly due to historical reasons.
> I am not sure if it is even possible to standardize these more across the
> avocado projects.
> 

The reason is that Avocado-vt (virt-test/kvm-autotest) is actually older than Avocado and has wider community attached. It is always better to have 2+ reviews, but in Avocado we simply can not afford it, which is why we encourage it, but don't enforce it.

As for the rules, it should be probably mentioned in the Avocado-vt Guidelines, contributions are welcome. On the other hand only a handful of people have write permissions and they should be thought beforehand. Also github is helping (a bit) as IIRC we have set the 2-ack rule there (also I am not certain of it since it only enforces such rules via web interface and I am merging via my scripts...).

Regards,
Lukáš

> Thanks a lot for asking this,
> Plamen
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/avocado-devel/attachments/20191205/b922a12f/attachment.sig>