[Pulp-dev] PR merging

[Matthias Dellweg]

On Wed, Sep 30, 2020 at 2:37 PM David Davis <daviddavis at 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 <mdellweg at 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 <dalley at 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 <daviddavis at 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 at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>>
>>> _______________________________________________
>>> Pulp-dev mailing list
>>> Pulp-dev at redhat.com
>>> https://www.redhat.com/mailman/listinfo/pulp-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pulp-dev/attachments/20200930/8ad23c31/attachment.htm>