On Wed, Sep 30, 2020 at 2:37 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. > > 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. > I haven't used the required-something in a while, and with the new backwards compatibility policy it should not be needed anymore (definitely not on the pulpcore side). I'd say if there is such a thing on the PR it should never be merged automatically. Also we could say, that plugins should remove that tag, once the corresponding pulpcore change was merged. > > 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