[Pulp-dev] PR merging

Fri Oct 23 21:03:22 UTC 2020

I wasn't able to get to this this week. Will send out an email next week
with another date.

David

On Tue, Oct 13, 2020 at 12:52 PM David Davis <daviddavis at redhat.com> wrote:

> 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/20201023/f56ae410/attachment.htm>