[Avocado-devel] Review and merge rules

[Cleber Rosa]

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.

> 
> 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.

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.

> 
> Thanks,
> Plamen
> 

Most importantly IMO is: do you think there's room for improvements
here?  If so, what/how?

Thank you for raising these questions and concerns,
- Cleber.