[Pulp-dev] PR merging

Tue Oct 13 16:52:24 UTC 2020

During the pulpcore team meeting today there was a desire to move forward
with auto-merging pulpcore PRs. This would mean automatically merging of
pulpcore PRs that have all the necessary approvals (2), tests passing, no
changes requested, and are not draft PRs.

Here is the pulp-ci PR again for anyone that wants to review it:
https://github.com/pulp/pulp-ci/pull/737

Assuming there are no objections, the tentative date for this to take
effect will be Oct 23. I'll send out an announcement closer to that date as
a reminder along with info about how plugins can enable this feature too.

David

On Thu, Oct 1, 2020 at 10:12 AM Brian Bouterse <bmbouter at redhat.com> wrote:

>
>
> On Thu, Oct 1, 2020 at 7:35 AM Ina Panova <ipanova at 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 <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.
>>>
>>> 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.
>>
> I agree. If we're adding automation but also with manual steps we're not
> really realizing the benefits the automation is supposed to give us.
>
> I do think automation with two acks is a good idea. To me, our acks
> indicate something is ready to merge, and if two committers are saying that
> is the case, I believe the machines can take it from there.
>
>>
>> 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 <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
>>>>>
>>>> _______________________________________________
>>> 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/20201013/a59f7c7a/attachment.htm>