[Pulp-dev] PR merging

Thu Oct 1 13:39:24 UTC 2020

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 <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.
>
> 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/20201001/9691b381/attachment.htm>