[sane-devel] Requiring GitLab merge requests for all changes

[Povilas Kanapickas]

On 1/2/22 5:07 AM, Ralph Little wrote:
> Hi,
> 
> On 2022-01-01 6:56 p.m., Povilas Kanapickas wrote:
>> On 12/31/21 12:36 AM, Ralph Little wrote:
>>> Hi,
>>>
>>> On 2021-12-29 1:46 p.m., Povilas Kanapickas wrote:
>>>> Hello,
>>>>
>>>> What do you think about requiring merge requests for all incoming
>>>> changes on the sane-backends repository?
>>>>
>>>> This mostly doesn't change the current process because the majority of
>>>> changes land to master via merge requests already. If the developer
>>>> does
>>>> not think a true review is necessary then he can merge the new merge
>>>> request himself as soon as CI completes which currently introduces a
>>>> delay of around 15 minutes.
>>>>
>>>> Out of 20 direct pushes to master since 1.0.32 we still managed to
>>>> break
>>>> master once (twice, if we count a MR merge without waiting for CI).
>>>> This
>>>> is important because any build failure will cause bisecting harder for
>>>> unrelated backends.
>>>>
>>>> Lastly, merge requests provide a place for discussions even after a
>>>> merge request is merged (e.g. if issues have been caused). Filing issue
>>>> is not equivalent because there is no code review UI there.
>>>>
>>>> I can only think a single reasonable exception for the above policy:
>>>> the
>>>> push of the commit announcing a new release. With merge request we
>>>> would
>>>> need to create the release tag on a merge commit which is confusing.
>>>>
>>>> Please let me know what you think.
>>>>
>>>> Regards,
>>>> Povilas
>>> Personally, I always make changes to branches. I don't believe that
>>> there should be functional changes direct to master, even for small
>>> corrections.
>>> I don't know if I would necessarily go all the way to require MRs for
>>> every change but I am open to be convinced.
>> I'm not sure I understood correctly, so instead of guessing, let's
>> double check :) Are you not convinced that we should add a rule in
>> GitLab that forbids direct pushes or are you against general policy?
> I don't think we should permit direct commits to master as a rule,
> enforced either by policy or by some mechanism (if such a thing exists)
Gitlab allows adding a per-branch rule that has 3 settings:
 - Allowed to merge (Developers+Maintainers / Maintainers / No one / A
specific set of users)
 - Allowed to push (Developers+Maintainers / Maintainers / No one / A
specific set of users)
 - Allowed to force push (Yes/No)

So for example we could configure the following for master branch:

 - Allowed to merge = Developers+Maintainers
 - Allowed to push = No one (this would be relaxed to Maintainers for
the release commit)
 - Allowed to force push = No

Myself I slightly prefer the above strict rule because pushes of wrong
branch to wrong remote happen to everyone. An agreement would work fine too.

> I'm not sure about requiring MRs for all branches though but I am open
> to persuasion.
> I think that was my point.

I'm still not sure I understand :-) Are you talking about requiring a MR
from any arbitrary branch abc to another branch xyz? Or about requiring
a MR from any arbitrary branch abc to "master" branch? Could you please
explain like I'm five with a couple examples?

Cheers,
Povilas