Re: [sane-devel] Requiring GitLab merge requests for all changes
I thought I'd reanimate this discussion to see if I could get a little help. I've been working on SANE for many years, even before we used git. I've been committing and pushing to master from the beginning. I pretty much use the command line exclusively. I've never had a problem I could not recover from. But, I'm not well versed in gitlab. So, I need a cheat sheet for how you guys think changes should be committed now. Something simple that an old unix graybeard can handle :) thanks for your help- allan On Mon, Jan 3, 2022 at 4:29 PM Povilas Kanapickas wrote: > 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 > > -- "well, I stand up next to a mountain- and I chop it down with the edge of my hand"
Re: [sane-devel] Requiring GitLab merge requests for all changes
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
Re: [sane-devel] Requiring GitLab merge requests for all changes
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. 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. Cheers, Ralph
Re: [sane-devel] Requiring GitLab merge requests for all changes
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
Re: [sane-devel] Requiring GitLab merge requests for all changes
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). I'm not sure about requiring MRs for all branches though but I am open to persuasion. I think that was my point. Cheers, Ralph
Re: [sane-devel] Requiring GitLab merge requests for all changes
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? My though was that maybe it's just enough to have an agreement on the mailing list and then remind people about it when they break it. If this is still frequent, then we can add a hard rule in GitLab. Cheers, Povilas
Re: [sane-devel] Requiring GitLab merge requests for all changes
> What do you think about requiring merge requests for all incoming > changes on the sane-backends repository? I am all for it. I think there _always_ should be room for review, regardless how unlikely it is that someone will actually do it. > 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. Fine with me. signature.asc Description: PGP signature
Re: [sane-devel] Requiring GitLab merge requests for all changes
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. Cheers, Ralph
[sane-devel] Requiring GitLab merge requests for all changes
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
