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