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

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 >> >> >

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 infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev

Hey Tristan, Comments inlined. Thanks, Sebastian On Fri, Dec 1, 2017 at 9:28 AM Tristan Tarrant <ttarrant(a)redhat.com <mailto:ttarrant@redhat.com>> 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 infinispan-dev(a)lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev(a)lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev