[Pulp-dev] RFC: Replace LGTM with github reviews
Michael Hrivnak
mhrivnak at redhat.com
Tue Oct 4 15:51:24 UTC 2016
Some times a conflict resolution is required when merging forward, so we
would need to make sure that non-fast-forward case is handled.
Michael
On Tue, Oct 4, 2016 at 11:48 AM, Brian Bouterse <bbouters at redhat.com> wrote:
> It would still allow for merge forwards I think because:
>
> - The merge forward would happen locally (github can't stop that)
> - The pushes would be fast-forward which our protected branch policy
> allows.
>
> That's what I expect at least.
>
> -Brian
>
>
> On 10/04/2016 11:45 AM, Michael Hrivnak wrote:
>
>> Big +1 to using the review feature.
>>
>> Although like Austin, I really like seeing in the PR list which have
>> been LGTM'd. Maybe it's worth not having that anymore as the price for
>> using the review feature, but it would be a shame to lose.
>>
>> On mixing this with the protected branch feature, that sounds good.
>> Would that still allow merging forward? For example, if I have a PR to
>> 2.10-dev that's been reviewed and approved, once I merge it to 2.10-dev,
>> can I merge that change to master and push the master branch?
>>
>> Michael
>>
>> On Tue, Oct 4, 2016 at 11:06 AM, Austin Macdonald <amacdona at redhat.com
>> <mailto:amacdona at redhat.com>> wrote:
>>
>>
>>
>> On 10/03/2016 05:43 PM, Sean Myers wrote:
>> > Github's new review feature is supported by another new github
>> feature,
>> > protected branches. If wanted to, we could combine the two ideas,
>> and
>> > only push to protected branches if there is at least one approved
>> review
>> > and no review with required changes outstanding.
>> >
>> > I think this could be used instead of the "LGTM" label
>> I don't disagree, but I do like being able to see whether a PR has
>> been
>> approved or not at a glance in the PR list.
>> > , and can also help
>> > with the notion of "drive-by" reviews, since the UI gives you a
>> clear
>> > distinction between comments that require changes to be made and
>> comments
>> > that do not require changes to be made.
>> >
>> > For reference:
>> > https://help.github.com/articles/about-pull-request-reviews/
>> <https://help.github.com/articles/about-pull-request-reviews/>
>> >
>> > Since we've already got support for protected branches, I propose we
>> > enable required reviews, and stop using the LGTM label.
>> >
>> >
>> >
>> > _______________________________________________
>> > Pulp-dev mailing list
>> > Pulp-dev at redhat.com <mailto:Pulp-dev at redhat.com>
>> > https://www.redhat.com/mailman/listinfo/pulp-dev
>> <https://www.redhat.com/mailman/listinfo/pulp-dev>
>>
>>
>>
>> _______________________________________________
>> Pulp-dev mailing list
>> Pulp-dev at redhat.com <mailto:Pulp-dev at redhat.com>
>> https://www.redhat.com/mailman/listinfo/pulp-dev
>> <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/20161004/d5770c23/attachment.htm>
More information about the Pulp-dev
mailing list