[infinispan-dev] PR labels

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[infinispan-dev] PR labels

Tristan Tarrant-2
Hello people,

I'd like to rationalize the PR labels because I believe some of them are
useless:

[Ready for review] - Any PR without the [Preview] label must fall under
this category
[Backport] - The burden should be on the PR owner to create relevant
backport PRs, not on the reviewer
[Wait CI Results] - PRs should only be integrated after a successful CI
run (or when failures can be proven to be pre-existing)
[Check CI Failures!] - The CI runs already add failure/success to the PR
status. Checking CI failures should apply to ALL PRs.
[On Ice] PR should be closed and reopened when relevant again.

Comments/suggestions ?

Tristan
--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Radim Vansa
On 12/01/2017 09:26 AM, Tristan Tarrant wrote:
> Hello people,
>
> I'd like to rationalize the PR labels because I believe some of them are
> useless:
>
> [Ready for review] - Any PR without the [Preview] label must fall under
> this category
> [Backport] - The burden should be on the PR owner to create relevant
> backport PRs, not on the reviewer

I think that [Backport] means that this is already in upstream, and
therefore review should be mostly formal (not breaking APIs but not
"this could be done 1% better. Also it is a second warning for reviewer
that this shouldn't be cherry-picked on master (when merging from cmdline).

> [Wait CI Results] - PRs should only be integrated after a successful CI
> run (or when failures can be proven to be pre-existing)
> [Check CI Failures!] - The CI runs already add failure/success to the PR
> status. Checking CI failures should apply to ALL PRs.
> [On Ice] PR should be closed and reopened when relevant again.
>
> Comments/suggestions ?
>
> Tristan


--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Radim Vansa
On 12/01/2017 10:04 AM, Radim Vansa wrote:

> On 12/01/2017 09:26 AM, Tristan Tarrant wrote:
>> Hello people,
>>
>> I'd like to rationalize the PR labels because I believe some of them are
>> useless:
>>
>> [Ready for review] - Any PR without the [Preview] label must fall under
>> this category
>> [Backport] - The burden should be on the PR owner to create relevant
>> backport PRs, not on the reviewer
>
> I think that [Backport] means that this is already in upstream, and
> therefore review should be mostly formal (not breaking APIs but not
> "this could be done 1% better.

Hit send too fast... The complexity of a review indicates time spent
with the review; I'd expect a backport review to be a 15 minute job, not
2 hour one, so when looking for a appetizer before lunch these are
on-sight good candidates.

> Also it is a second warning for reviewer that this shouldn't be
> cherry-picked on master (when merging from cmdline).

>
>> [Wait CI Results] - PRs should only be integrated after a successful CI
>> run (or when failures can be proven to be pre-existing)
>> [Check CI Failures!] - The CI runs already add failure/success to the PR
>> status. Checking CI failures should apply to ALL PRs.
>> [On Ice] PR should be closed and reopened when relevant again.
>>
>> Comments/suggestions ?
>>
>> Tristan
>
>

--
Radim Vansa <[hidden email]>
JBoss Performance Team

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Pedro Ruivo-2
In reply to this post by Tristan Tarrant-2
Hi,

can we keep the "changes suggested/required"?
This would be exclusive with "ready for preview" and means someone
review the PR and requires a reply from the author.

Cheers,
Pedro

On 01-12-2017 08:26, Tristan Tarrant wrote:

> Hello people,
>
> I'd like to rationalize the PR labels because I believe some of them are
> useless:
>
> [Ready for review] - Any PR without the [Preview] label must fall under
> this category
> [Backport] - The burden should be on the PR owner to create relevant
> backport PRs, not on the reviewer
> [Wait CI Results] - PRs should only be integrated after a successful CI
> run (or when failures can be proven to be pre-existing)
> [Check CI Failures!] - The CI runs already add failure/success to the PR
> status. Checking CI failures should apply to ALL PRs.
> [On Ice] PR should be closed and reopened when relevant again.
>
> Comments/suggestions ?
>
> Tristan
>
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Tristan Tarrant-2
Sure: all the labels I haven't mentioned can stay.

Tristan

On 12/1/17 1:03 PM, Pedro Ruivo wrote:

> Hi,
>
> can we keep the "changes suggested/required"?
> This would be exclusive with "ready for preview" and means someone
> review the PR and requires a reply from the author.
>
> Cheers,
> Pedro
>
> On 01-12-2017 08:26, Tristan Tarrant wrote:
>> Hello people,
>>
>> I'd like to rationalize the PR labels because I believe some of them are
>> useless:
>>
>> [Ready for review] - Any PR without the [Preview] label must fall under
>> this category
>> [Backport] - The burden should be on the PR owner to create relevant
>> backport PRs, not on the reviewer
>> [Wait CI Results] - PRs should only be integrated after a successful CI
>> run (or when failures can be proven to be pre-existing)
>> [Check CI Failures!] - The CI runs already add failure/success to the PR
>> status. Checking CI failures should apply to ALL PRs.
>> [On Ice] PR should be closed and reopened when relevant again.
>>
>> Comments/suggestions ?
>>
>> Tristan
>>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Sebastian Laskawiec
In reply to this post by Tristan Tarrant-2
Hey Tristan,

Comments inlined.

Thanks,
Sebastian

On Fri, Dec 1, 2017 at 9:28 AM Tristan Tarrant <[hidden email]> wrote:
Hello people,

I'd like to rationalize the PR labels because I believe some of them are
useless:

[Ready for review] - Any PR without the [Preview] label must fall under
this category

If a PR doesn't fall into Preview category, it must be Ready for Review. In my opinion "Ready for Review" is redundant.
 
[Backport] - The burden should be on the PR owner to create relevant
backport PRs, not on the reviewer

+1
 
[Wait CI Results] - PRs should only be integrated after a successful CI
run (or when failures can be proven to be pre-existing)

All PRs should be evaluated by Jenkins. The CI check has 3 icons on Github Pull Request page - green tick, red cross and yellow dot. Yellow dot means that the PR is being built right now (or waiting in the queue). I believe "Wait CI Results" and that yellow dot are identical and "Wait CI Result" is redundant.
 
[Check CI Failures!] - The CI runs already add failure/success to the PR
status. Checking CI failures should apply to ALL PRs.
[On Ice] PR should be closed and reopened when relevant again.

Let just close such PRs! Redundant...
 

Comments/suggestions ?

Tristan
--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev

_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Tristan Tarrant-2
You basically just said +1 to all the ones I want to remove :)

Tristan

On 12/1/17 3:13 PM, Sebastian Laskawiec wrote:

> Hey Tristan,
>
> Comments inlined.
>
> Thanks,
> Sebastian
>
> On Fri, Dec 1, 2017 at 9:28 AM Tristan Tarrant <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Hello people,
>
>     I'd like to rationalize the PR labels because I believe some of them are
>     useless:
>
>     [Ready for review] - Any PR without the [Preview] label must fall under
>     this category
>
>
> If a PR doesn't fall into Preview category, it must be Ready for Review.
> In my opinion "Ready for Review" is redundant.
>
>     [Backport] - The burden should be on the PR owner to create relevant
>     backport PRs, not on the reviewer
>
>
> +1
>
>     [Wait CI Results] - PRs should only be integrated after a successful CI
>     run (or when failures can be proven to be pre-existing)
>
>
> All PRs should be evaluated by Jenkins. The CI check has 3 icons on
> Github Pull Request page - green tick, red cross and yellow dot. Yellow
> dot means that the PR is being built right now (or waiting in the
> queue). I believe "Wait CI Results" and that yellow dot are identical
> and "Wait CI Result" is redundant.
>
>     [Check CI Failures!] - The CI runs already add failure/success to the PR
>     status. Checking CI failures should apply to ALL PRs.
>     [On Ice] PR should be closed and reopened when relevant again.
>
>
> Let just close such PRs! Redundant...
>
>
>     Comments/suggestions ?
>
>     Tristan
>     --
>     Tristan Tarrant
>     Infinispan Lead
>     JBoss, a division of Red Hat
>     _______________________________________________
>     infinispan-dev mailing list
>     [hidden email] <mailto:[hidden email]>
>     https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> _______________________________________________
> infinispan-dev mailing list
> [hidden email]
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>

--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Tristan Tarrant-2
In reply to this post by Radim Vansa
Ok, good point.

Tristan

On 12/1/17 10:07 AM, Radim Vansa wrote:

> On 12/01/2017 10:04 AM, Radim Vansa wrote:
>> On 12/01/2017 09:26 AM, Tristan Tarrant wrote:
>>> Hello people,
>>>
>>> I'd like to rationalize the PR labels because I believe some of them are
>>> useless:
>>>
>>> [Ready for review] - Any PR without the [Preview] label must fall under
>>> this category
>>> [Backport] - The burden should be on the PR owner to create relevant
>>> backport PRs, not on the reviewer
>>
>> I think that [Backport] means that this is already in upstream, and
>> therefore review should be mostly formal (not breaking APIs but not
>> "this could be done 1% better.
>
> Hit send too fast... The complexity of a review indicates time spent
> with the review; I'd expect a backport review to be a 15 minute job, not
> 2 hour one, so when looking for a appetizer before lunch these are
> on-sight good candidates.
>
>> Also it is a second warning for reviewer that this shouldn't be
>> cherry-picked on master (when merging from cmdline).
>
>>
>>> [Wait CI Results] - PRs should only be integrated after a successful CI
>>> run (or when failures can be proven to be pre-existing)
>>> [Check CI Failures!] - The CI runs already add failure/success to the PR
>>> status. Checking CI failures should apply to ALL PRs.
>>> [On Ice] PR should be closed and reopened when relevant again.
>>>
>>> Comments/suggestions ?
>>>
>>> Tristan
>>
>>
>

--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev
Reply | Threaded
Open this post in threaded view
|

Re: [infinispan-dev] PR labels

Tristan Tarrant-2
Following the suggestions, I have removed:

[Ready for review] - Use [Preview] when it ain't ready
[Wait CI  Results] - Always wait for CI results
[Check CI Failures!] - See above
[On Ice] - Just close it and reopen it
[Next release] - Just close it and reopen it

I have left [Backport] in there to mean "already fuly reviewed for
another branch, only minimal effort required here".

Tristan

On 12/3/17 6:08 PM, Tristan Tarrant wrote:

> Ok, good point.
>
> Tristan
>
> On 12/1/17 10:07 AM, Radim Vansa wrote:
>> On 12/01/2017 10:04 AM, Radim Vansa wrote:
>>> On 12/01/2017 09:26 AM, Tristan Tarrant wrote:
>>>> Hello people,
>>>>
>>>> I'd like to rationalize the PR labels because I believe some of them
>>>> are
>>>> useless:
>>>>
>>>> [Ready for review] - Any PR without the [Preview] label must fall under
>>>> this category
>>>> [Backport] - The burden should be on the PR owner to create relevant
>>>> backport PRs, not on the reviewer
>>>
>>> I think that [Backport] means that this is already in upstream, and
>>> therefore review should be mostly formal (not breaking APIs but not
>>> "this could be done 1% better.
>>
>> Hit send too fast... The complexity of a review indicates time spent
>> with the review; I'd expect a backport review to be a 15 minute job, not
>> 2 hour one, so when looking for a appetizer before lunch these are
>> on-sight good candidates.
>>
>>> Also it is a second warning for reviewer that this shouldn't be
>>> cherry-picked on master (when merging from cmdline).
>>
>>>
>>>> [Wait CI Results] - PRs should only be integrated after a successful CI
>>>> run (or when failures can be proven to be pre-existing)
>>>> [Check CI Failures!] - The CI runs already add failure/success to
>>>> the PR
>>>> status. Checking CI failures should apply to ALL PRs.
>>>> [On Ice] PR should be closed and reopened when relevant again.
>>>>
>>>> Comments/suggestions ?
>>>>
>>>> Tristan
>>>
>>>
>>
>

--
Tristan Tarrant
Infinispan Lead
JBoss, a division of Red Hat
_______________________________________________
infinispan-dev mailing list
[hidden email]
https://lists.jboss.org/mailman/listinfo/infinispan-dev