Maybe we could implement a policy where, once the author of the PR is finished, they add a +1 review (or tag) to their own PR, and only then would it be merged (if the CI passes).
IMO having a reviewer enable the auto-merge functionality is basically equivalent to having them merge the PR manually, with all of the same drawbacks. As I mentioned earlier though, our PR feedback loop is not *that* long all things considered, so unless our CI times grow to >45 minutes, I'm not so sure it's worth it. On Thu, Oct 1, 2020 at 7:35 AM Ina Panova <ipan...@redhat.com> wrote: > > > -------- > Regards, > > Ina Panova > Senior Software Engineer| Pulp| Red Hat Inc. > > "Do not go where the path may lead, > go instead where there is no path and leave a trail." > > > On Wed, Sep 30, 2020 at 2:38 PM David Davis <davidda...@redhat.com> wrote: > >> I am also concerned about the lack of human involvement and the potential >> for things to be merged accidentally. I definitely could see that happening. >> >> I like the idea of having the PR processor only merge if a label (eg >> "Merge when Ready") is present. The question then is whether it should be >> applied automatically or not when a PR is opened. Maybe to start out with, >> it's not. >> >> On one hand, I like the idea of having this label as well and adding it > to the already mentioned conditions in your PR[0]. However, I would imagine > this would be a manual step: whether the reviewer or the author of the PR > would set this label. This will also cover the case Daniel > mentioned.."sometimes there's only minor changes requested and it doesn't > warrant a re-review. Currently what we do pretty often is just approve the > PR, leave a note to change the wording here and there, and the author can > just merge it whenever they're done with the minor changes." > > On the other hand I don't know what will be easier: pressing the merge > button or manually adding the label. Probably the only benefit would be - > you don't need to wait and hypnotize the CI so it gets green. > > Openstack upstream policies also uses auto-merge [0] scroll to the end. > > In my opinion, if we want to automate things this will require us to be > more diligent and responsible and leave +1 only when the PR is *really* > ready. Having the PR in a not a not draft state, +2 approvals and passing > CI I think is satisfactory enough to be merged. Adding another label feels > like killing the whole gist of automation. > > Another thing that can be done is clearing up all the approvals on each > push to the PR. But this will require people approving once again. > > [0] > https://github.com/pulp/pulp-ci/pull/737/files#diff-cbdf61d38083f599940c37eeb49cb0a9R116-R141 > [1] https://docs.openstack.org/zaqar/latest/contributor/first_patch.html > > One other thing I thought of is that the PR processor could also check PR >> dependencies (ie "Required PRs") and see if they are all mergeable before >> merging any PR. >> >> David >> >> >> On Wed, Sep 30, 2020 at 4:10 AM Matthias Dellweg <mdell...@redhat.com> >> wrote: >> >>> This just reminds me that Gitlab has a very nice "merge when CI passes >>> button" to decouple the merge question from the reviews. >>> The only way i could see this happen in Github is via setting a label >>> that instructs the PR processor to merge when (label is set) && (ci is >>> green) && (other conditions). >>> Does not sound too user friendly, but labels can only be set by people >>> with merge permissions anyway (so not a security concern). >>> >>> On Tue, Sep 29, 2020 at 8:04 PM Daniel Alley <dal...@redhat.com> wrote: >>> >>>> I have some doubts about the cost/benefit ratio of coding automation to >>>> merge PRs vs. simply changing the default option / being selective about >>>> which options are available. >>>> >>>> I like the idea in general though. A lot of projects do something like >>>> this. I occasionally contributed to the Servo project (RIP) and they used >>>> automation to manage this. The benefits were, their tests suites ran for >>>> nearly 2 hours on 3 different operating systems, so the feedback loop was >>>> quite slow, and automatically triggering the full test run every time a PR >>>> was changed would be a horrendous waste of resources at the scale they were >>>> running at. So basically they would run a minimal CI within the PR, then >>>> require a manual review, and once approved it would trigger the full CI, >>>> and if that passed then it would automatically merge the PR hours later >>>> whenever it finished. >>>> >>>> If our test suites keep getting longer and longer and take more than >>>> the 20-25 minutes they currently take, I can definitely see how the commit >>>> processor approach could add value. >>>> >>>> On the other hand, it has the downside that, sometimes even once a >>>> commit is approved you might want to change something minor like rebasing >>>> the commits manually, and automation might start getting in the way of >>>> things like that. Or, another example, sometimes there's only minor >>>> changes requested and it doesn't warrant a re-review. Currently what we do >>>> pretty often is just approve the PR, leave a note to change the wording >>>> here and there, and the author can just merge it whenever they're done with >>>> the minor changes. >>>> >>>> On Tue, Sep 29, 2020 at 12:49 PM David Davis <davidda...@redhat.com> >>>> wrote: >>>> >>>>> Hi all, >>>>> >>>>> Last week the discussion about how to merge PRs got me thinking about >>>>> how we could potentially programmatically merge PRs. The openshift project >>>>> on github does this[0] and I wondered if it would work for Pulp. >>>>> >>>>> I think the main benefits would be (1) we'd be able to code how to >>>>> best merge PRs and (2) we'd no longer see PRs sitting around that are >>>>> ready >>>>> to merge but not merged (granted this doesn't happen often). >>>>> >>>>> I created a PoC against the PR processor that's quite simple (here are >>>>> the relevant bits[1]). It's worth noting that we can still prevent PRs >>>>> from >>>>> being merged by setting them to drafts (I believe both the PR author or >>>>> any >>>>> repo owner do this) and the PR processor won't merge anything unless it's >>>>> ready to be merged. >>>>> >>>>> I myself am a bit conflicted about this so feedback would be >>>>> appreciated. >>>>> >>>>> [0] https://github.com/openshift >>>>> [1] >>>>> https://github.com/pulp/pulp-ci/pull/737/files#diff-cbdf61d38083f599940c37eeb49cb0a9R116-R141 >>>>> >>>>> David >>>>> _______________________________________________ >>>>> Pulp-dev mailing list >>>>> Pulp-dev@redhat.com >>>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>>> >>>> _______________________________________________ >>>> Pulp-dev mailing list >>>> Pulp-dev@redhat.com >>>> https://www.redhat.com/mailman/listinfo/pulp-dev >>>> >>> _______________________________________________ >> Pulp-dev mailing list >> Pulp-dev@redhat.com >> https://www.redhat.com/mailman/listinfo/pulp-dev >> > _______________________________________________ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev