[Spice-devel] SPICE: changing the merge rules - a proposal

[Francesco Giudici]

Hi,

On 15/05/20 23:58, Marc-André Lureau wrote:
> Hi
> 
> On Fri, May 15, 2020 at 7:06 PM Francesco Giudici <fgiudici at redhat.com 
> <mailto:fgiudici at redhat.com>> wrote:
> 
>     Hi,
>         the community around the SPICE project always tried to follow one
>     fundamental, implicit rule for accepting code contributions to the
>     project: every merge request (beside trivial patches) should be
>     reviewed
>     and acked at least by one before getting merged.
>     While everyone agrees with this fundamental rule, the actual status of
>     some SPICE projects makes the rule impractical to let the project move
>     forward.
> 
> 
> I wasn't aware of a maintenance problem. Perhaps we should first list 
> the projects that have maintenance issues & discuss our options, before 
> changing the common rule.

The idea of this e-mail is exactly this: let's discuss. But starting 
with a proposal and not only the issue seems to me the best way to try 
to move things forward (also if in the end the proposal my be completely 
changed it would have reached the goal).

> 
>     Let's consider the spice/spice project as an example: the number of
>     contributions is very low, both on the commit side (only 4 different
>     contributors with more than 1 commit from the beginning of the year,
>     and
>     a single contributor with 90% of commits) and on the review side (in
>     the
>     last 40 merge requests before the C++ switch one, 21 had no comments).
> 
> 
> You are omitting the passive reviewers. I consider myself as one of 
> them. If you need people to be more proactive, you could at least reach 
> me & probably others past contributors.

Great to know that you are looking at the contributions, also if not 
sure what a "passive" reviewer does. In this case anyway I think if you 
add a comment to the branch you looked at it will be much better. I'm 
pretty sure the submitter will be happy that someone looked at it. So, 
reaching out right now: if there are people looking at the branches and 
not commenting, please start doing. Also partial comments (not full 
reviews) may help I would say.

> 
>     The x11spice project is another example: we have only 4 contributors
>     from the beginning of the year (and a single contributor holding 70% of
>     the commits) and the reviews on the gitlab merge requests have been
>     provided by two people only, each one reviewing the merge requests of
>     the other.
> 
> 
> As projects become more specific/marginal, it's clear that the number of 
> maintainers is lower. Yet, we have those projects under the same 
> umbrella, and I don't think they should be treated differently.

Sorry, maybe I was not clear: projects under the same umbrella should be 
treated equally. So, the rules will be the same for every project, and 
can be used to address situations where contributions (reviews) are 
missing. This is something that the number above suggests.

  2 active
> developers/maintainers can be very healthy, I would say. So do we have a 
> maintenance issue with x11spice? Do we want to move the project out of 
> the "Spice-space" projects for ex? What is the problem exactly?

I think you are not getting the point, maybe I was not clear: as long as 
we have at least 2 contributors we get a review there. Perfect, nothing 
changes.
What if one of the two developers/maintainers gets sick? For months? Or 
stops for his own reasons to contribute to the project? Should the 
project stop?
Problem is: if there is low contribution we don't want that active 
contributors face even more obstacles to keep contributing. We want to 
keep the project healthy and alive as much as possible.


> 
>     For the sake of having the projects being able to move forward with a
>     reduced number of contributors/reviewers, the proposal is to *allow* a
>     maintainer to merge a Merge Request without an explicit ack if the
>     three
>     following conditions are met:
>     1) The Merge Request has been pending for at least 3 weeks without
>     getting new comments
>     2) The Merge Request submitter has kept asking a review on a weekly
>     basis
>     3) There are no pending nacks on the Merge Request
> 
> 
> It's hard to define a delay to bypass a reviewing & consensus rule. In 
> general, it should really be frowned upon.

I understand your feeling and I feel the same at least in part: but the 
rules we are talking about apply only when a project doesn't get reviews 
and gets paralyzed. Also notice that it *allows* a *maintainer* to merge 
the branch, it is not automatic. We give more freedom and trust to 
maintainers over strict rules when contributions are low.

> There is clearly more than 
> one person interested and using Spice. If the issue is important, it 
> should be fairly easy to get someone else to look at the issue in a 
> timely manner.

I like your optimism here, but honestly I don't think this is true. But 
anyway, checking if someone will give the review is already part of the 
proposal: the "asking a review on a weekly basis" meant this. Making 
this more explicit would help?


> If not, there are probably more important/interesting 
> things to do to get other interested.

That sounds great, but I don't have a clue here. If you have, please, 
make a proposal!

> 
> If Spice is good enough for our users and interested parties, then it 
> may be risky to change it in wild directions without their approval.

I don't understand what you mean here. If the alternative is the project 
not going forward anymore I would be in favor to let a maintainer push 
his own commits and branches introducing new features, reworks and so 
on. We are talking of an approved maintainer, not a casual committer. A 
maintainer asking for reviews for 3 weeks without getting any feedback.
The other option, a fork leaving the original project abandoned (or just 
the project abandoned) just because the maintainer doesn't want to 
struggle in a discussion like this, wouldn't be worse?

> 
> But 3 weeks is way too short. You could have more important work, family 
> issue, get sick and go on holiday, all happening each after the other. 
> If we need you to review some code, because you have the best 
> experience, we should wait. If really we want a delay, I would argue 
> waiting 3 months minimum (there might be exceptional circumstances, but 
> they are exception, not the rule). And during that time, the contributor 
> should have attempted multiple time to involve people, by different 
> means (at least ML, irc and gitlab issue+MR - eventually try a conf call 
> to explain the motivation, present the work differently etc, complain 
> about the Spice project laziness on public blog if need be etc).

IMHO, I don't think is that fair that one proposing a branch should 
fight to have someone even just looking at it. Likely, he/she will spend 
his/her time in another project next time.
But it is great that you have some ideas... can you come out with a 
draft amendment/proposal that looks good to you?

> 
> 
>     Note that having patches reviewed would still be the preferred way. If
>     at any time the number of contributors would raise again, we can switch
>     back to the mandatory review rule. Until then the priority is to allow
>     the project to move forward.
> 
>     What do you think? Please share your thoughts and/or contribute with
>     your own ideas.
> 
> 
> Thanks, but I think the trivial rule is already very subtle and 
> generally disapproved (fwiw, I am still in favor of a subjective trivial 
> rule), so I don't think this proposal would work.

This proposal is different from the trivial rule you mention. I would 
say that if you ask to review a trivial patch for 3 weeks and no one 
gives any feedback no one will be disappointed from someone merging it. 
If someone is against this please tell :-)
ASAICS this merging without reviews already happened in gitlab somewhere 
and went unnoticed...

> 
> We have to grow a community, by convincing people and doing interesting 
> work.

I would love this! But how? Living merge requests starving doesn't help 
this, would work the opposite. What is your proposal?

> Not by doing personal thing, and then leave the pieces behind, 
> because we did it alone and didn't gather others.

Sorry, this is just your thoughts (which I honestly think are plain 
wrong). We are discussing rules exactly because this is a project with a 
community. And if no one in the community is helping despite a 
maintainer asking for help and that maintainers brings on the 
development alone with all his best intentions... we'll, I have a lot of 
respect for that maintainer as he/she is keeping the project alive.
But here we don't want to discuss our personal opinions, let's focus on 
proposals.

> 
> Perhaps Spice is doing the job. Perhaps Spice needs to define new 
> objectives. These are interesting topics that I believe people would 
> want to discuss and participate. If not, we are doing it wrong.

Let's try to move forward then. Discussion and proposals on what to do 
may really help.

Thanks

Francesco

> 
> thanks
> 
> -- 
> Marc-André Lureau