You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Boris Schrijver <bo...@pcextreme.nl> on 2015/12/14 16:58:47 UTC
PR's and the 2 LGTM Story
Hi all,
Just to make this clear once more. A PR needs two LGTM's. One of them needs to
run the integration- and unit-tests. While the other does code review.
This is a the minimal requirement. More is always welcome, anything less will be
reverted.
I just enforced this policy [1]. Any active committer has at all times the right
to do so.
[1]
https://github.com/apache/cloudstack/commit/a00fef8c332ebaede32c46cbf3065f4acaa91f02
--
Met vriendelijke groet / Kind regards,
Boris Schrijver
PCextreme B.V.
http://www.pcextreme.nl/contact
Tel direct: +31 (0) 118 700 215
Re: PR's and the 2 LGTM Story
Posted by Sebastien Goasguen <ru...@gmail.com>.
Top posting on this one to reinforce a few things:
Master is now our release branch
Merge on master by the RMs *only*
PR still need 2 LGTM
Do note, that any committer is free to create any other branch and work there or work on their own fork.
The goal is not to slow down development but instead to:
- increase quality and keep it up
- increase velocity of releases
We are already seeing the benefits of this, so let’s keep it up.
-sebastien
> On Dec 15, 2015, at 8:35 AM, Wido den Hollander <wi...@widodh.nl> wrote:
>
>
>
> On 12/14/2015 11:15 PM, Remi Bergsma wrote:
>> Guys,
>>
>> I can see Daan fixed the build and am happy he cares so much about that. But I don't see why we had to do it like this, instead of reverting the PR that caused the build to fail? That's something one can do quickly without review.
>>
>> Now the mess is complete. A broken PR merged (can happen), another PR merged without proper review, a revert of this PR and finally a direct commit to master. Pffff. I think all of us agree we don't want any of this.
>>
>> Most people are often online, ping them for instant review. Works almost all of the time.
>>
>
> Indeed. Use Slack/IRC/Phones or what ever suites. Talk and resolve these
> things.
>
> Wido
>
>> Regard, Remi
>>
>> Sent from my iPhone
>>
>>> On 14 Dec 2015, at 17:41, Daan Hoogland <da...@gmail.com> wrote:
>>>
>>> You have been doing this blindly and I don't accept this
>>> I have applied the mentioned code directly to master. Next time first see
>>> if you can add your lgtm and whether the code is needed.
>>>
>>>> On Mon, Dec 14, 2015 at 4:58 PM, Boris Schrijver <bo...@pcextreme.nl> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> Just to make this clear once more. A PR needs two LGTM's. One of them
>>>> needs to
>>>> run the integration- and unit-tests. While the other does code review.
>>>>
>>>> This is a the minimal requirement. More is always welcome, anything less
>>>> will be
>>>> reverted.
>>>>
>>>> I just enforced this policy [1]. Any active committer has at all times the
>>>> right
>>>> to do so.
>>>>
>>>> [1]
>>>>
>>>> https://github.com/apache/cloudstack/commit/a00fef8c332ebaede32c46cbf3065f4acaa91f02
>>>>
>>>> --
>>>>
>>>> Met vriendelijke groet / Kind regards,
>>>>
>>>> Boris Schrijver
>>>>
>>>> PCextreme B.V.
>>>>
>>>> http://www.pcextreme.nl/contact
>>>> Tel direct: +31 (0) 118 700 215
>>>
>>>
>>>
>>> --
>>> Daan
Re: PR's and the 2 LGTM Story
Posted by Wido den Hollander <wi...@widodh.nl>.
On 12/14/2015 11:15 PM, Remi Bergsma wrote:
> Guys,
>
> I can see Daan fixed the build and am happy he cares so much about that. But I don't see why we had to do it like this, instead of reverting the PR that caused the build to fail? That's something one can do quickly without review.
>
> Now the mess is complete. A broken PR merged (can happen), another PR merged without proper review, a revert of this PR and finally a direct commit to master. Pffff. I think all of us agree we don't want any of this.
>
> Most people are often online, ping them for instant review. Works almost all of the time.
>
Indeed. Use Slack/IRC/Phones or what ever suites. Talk and resolve these
things.
Wido
> Regard, Remi
>
> Sent from my iPhone
>
>> On 14 Dec 2015, at 17:41, Daan Hoogland <da...@gmail.com> wrote:
>>
>> You have been doing this blindly and I don't accept this
>> I have applied the mentioned code directly to master. Next time first see
>> if you can add your lgtm and whether the code is needed.
>>
>>> On Mon, Dec 14, 2015 at 4:58 PM, Boris Schrijver <bo...@pcextreme.nl> wrote:
>>>
>>> Hi all,
>>>
>>> Just to make this clear once more. A PR needs two LGTM's. One of them
>>> needs to
>>> run the integration- and unit-tests. While the other does code review.
>>>
>>> This is a the minimal requirement. More is always welcome, anything less
>>> will be
>>> reverted.
>>>
>>> I just enforced this policy [1]. Any active committer has at all times the
>>> right
>>> to do so.
>>>
>>> [1]
>>>
>>> https://github.com/apache/cloudstack/commit/a00fef8c332ebaede32c46cbf3065f4acaa91f02
>>>
>>> --
>>>
>>> Met vriendelijke groet / Kind regards,
>>>
>>> Boris Schrijver
>>>
>>> PCextreme B.V.
>>>
>>> http://www.pcextreme.nl/contact
>>> Tel direct: +31 (0) 118 700 215
>>
>>
>>
>> --
>> Daan
Re: PR's and the 2 LGTM Story
Posted by Remi Bergsma <RB...@schubergphilis.com>.
Guys,
I can see Daan fixed the build and am happy he cares so much about that. But I don't see why we had to do it like this, instead of reverting the PR that caused the build to fail? That's something one can do quickly without review.
Now the mess is complete. A broken PR merged (can happen), another PR merged without proper review, a revert of this PR and finally a direct commit to master. Pffff. I think all of us agree we don't want any of this.
Most people are often online, ping them for instant review. Works almost all of the time.
Regard, Remi
Sent from my iPhone
> On 14 Dec 2015, at 17:41, Daan Hoogland <da...@gmail.com> wrote:
>
> You have been doing this blindly and I don't accept this
> I have applied the mentioned code directly to master. Next time first see
> if you can add your lgtm and whether the code is needed.
>
>> On Mon, Dec 14, 2015 at 4:58 PM, Boris Schrijver <bo...@pcextreme.nl> wrote:
>>
>> Hi all,
>>
>> Just to make this clear once more. A PR needs two LGTM's. One of them
>> needs to
>> run the integration- and unit-tests. While the other does code review.
>>
>> This is a the minimal requirement. More is always welcome, anything less
>> will be
>> reverted.
>>
>> I just enforced this policy [1]. Any active committer has at all times the
>> right
>> to do so.
>>
>> [1]
>>
>> https://github.com/apache/cloudstack/commit/a00fef8c332ebaede32c46cbf3065f4acaa91f02
>>
>> --
>>
>> Met vriendelijke groet / Kind regards,
>>
>> Boris Schrijver
>>
>> PCextreme B.V.
>>
>> http://www.pcextreme.nl/contact
>> Tel direct: +31 (0) 118 700 215
>
>
>
> --
> Daan
Re: PR's and the 2 LGTM Story
Posted by Remi Bergsma <RB...@schubergphilis.com>.
Guys,
I can see Daan fixed the build and am happy he cares so much about that. But I don't see why we had to do it like this, instead of reverting the PR that caused the build to fail? That's something one can do quickly without review.
Now the mess is complete. A broken PR merged (can happen), another PR merged without proper review, a revert of this PR and finally a direct commit to master. Pffff. I think all of us agree we don't want any of this.
Most people are often online, ping them for instant review. Works almost all of the time.
Regard, Remi
> On 14 Dec 2015, at 17:41, Daan Hoogland <da...@gmail.com> wrote:
>
> You have been doing this blindly and I don't accept this
> I have applied the mentioned code directly to master. Next time first see
> if you can add your lgtm and whether the code is needed.
>
>> On Mon, Dec 14, 2015 at 4:58 PM, Boris Schrijver <bo...@pcextreme.nl> wrote:
>>
>> Hi all,
>>
>> Just to make this clear once more. A PR needs two LGTM's. One of them
>> needs to
>> run the integration- and unit-tests. While the other does code review.
>>
>> This is a the minimal requirement. More is always welcome, anything less
>> will be
>> reverted.
>>
>> I just enforced this policy [1]. Any active committer has at all times the
>> right
>> to do so.
>>
>> [1]
>>
>> https://github.com/apache/cloudstack/commit/a00fef8c332ebaede32c46cbf3065f4acaa91f02
>>
>> --
>>
>> Met vriendelijke groet / Kind regards,
>>
>> Boris Schrijver
>>
>> PCextreme B.V.
>>
>> http://www.pcextreme.nl/contact
>> Tel direct: +31 (0) 118 700 215
>
>
>
> --
> Daan
Re: PR's and the 2 LGTM Story
Posted by Daan Hoogland <da...@gmail.com>.
You have been doing this blindly and I don't accept this
I have applied the mentioned code directly to master. Next time first see
if you can add your lgtm and whether the code is needed.
On Mon, Dec 14, 2015 at 4:58 PM, Boris Schrijver <bo...@pcextreme.nl> wrote:
> Hi all,
>
> Just to make this clear once more. A PR needs two LGTM's. One of them
> needs to
> run the integration- and unit-tests. While the other does code review.
>
> This is a the minimal requirement. More is always welcome, anything less
> will be
> reverted.
>
> I just enforced this policy [1]. Any active committer has at all times the
> right
> to do so.
>
> [1]
>
> https://github.com/apache/cloudstack/commit/a00fef8c332ebaede32c46cbf3065f4acaa91f02
>
> --
>
> Met vriendelijke groet / Kind regards,
>
> Boris Schrijver
>
> PCextreme B.V.
>
> http://www.pcextreme.nl/contact
> Tel direct: +31 (0) 118 700 215
>
--
Daan