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

[Povilas Kanapickas]

On 1/2/22 6:30 PM, Ralph Little wrote> On 2022-01-02 3:14 a.m., Povilas
Kanapickas wrote:
>> On 1/2/22 5:07 AM, Ralph Little wrote:
>>>>> 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?
>>
> I guess my point is that I don't really care much about requiring Merge
> Requests for any changes to the repo as much as I care about not pushing
> changes directly to master without branching and merging (and benefiting
> from our pipeline checks).
> It is too easy to break master by a careless single push.

I finally understood. I was not thinking about the third option of not
using merge requests, but just using separate branches, checking the CI
results and merging via local git instance.

> Referring to your original proposition:
> 
>> What do you think about requiring merge requests for all incoming
>> changes on the sane-backends repository?
> 
> ... I am open to discussion certainly and I can see the advantages.

Making sure the changes work via pushing branches and checking CI solves
the issue of broken commits being pushed just like requiring merge requests.

In addition to the above, requiring merge requests solve this:
 - there's a place to conveniently comment about any change landing to
the master branch far into the future
 - the chance of accidental push to master is eliminated (happened to
even to me, though on a different project)

I don't know whether these are important points to convince you. Myself
I only have slight preference for requiring merge requests and Wolfram
indicated that he strongly supports it due to ability to do code reviews.

Cheers,
Povilas