I see your point Koushik, but as a matter of spreading the knowledge I think it is best to have non-expert give a second ok. In those cases extra explanation will be needed. That doesn't hurt either. If we all have the discipline to work on reviews once a week, we are spreading knowledge and keeping the list short.
If we demand this of ourselves we are also demanding of ourselves to keep specialised code as simple as possible. We are preventing the creation of niches. What Wilder suggests id to drop the PR after a specified time instead of letting it pass. This forces whoever thinks it is important enough to share knowledge. thoughts? On Fri, Jul 17, 2015 at 1:50 PM, Koushik Das <koushik....@citrix.com> wrote: > For any reviews there are 2 aspects - general coding guidelines and domain > knowledge. Based on the current backlog of PRs, I feel that whenever some > domain knowledge is required to review a PR and there are not enough > reviewers for that, the PR waits for a longer time. That's why I had > suggested that if a bug fix PR is green (as per travis) but there isn't > enough LGTMs beyond a specified time then there should be some exception to > the rule. > > -----Original Message----- > From: Daan Hoogland [mailto:daan.hoogl...@gmail.com] > Sent: Friday, 17 July 2015 16:50 > To: dev > Subject: Re: [DISCUSS] PR list length > > I agree as well and since I started this thread a lot of work is being done > to keep the list as short as possible. I am still worried that the list is > growing, it has over a few weeks and though the growth stopped during the > lifetime of this thread I hope we can keep up the discipline to keep it > short. At the moment it depends on us few which is not a future proof > situation. > > On Fri, Jul 17, 2015 at 1:08 PM, Rajani Karuturi <raj...@apache.org> wrote: >> I agree with wilder. >> >> ~Rajani >> >> On Thu, Jul 16, 2015 at 12:08 PM, Wilder Rodrigues < >> wrodrig...@schubergphilis.com> wrote: >> >>> We should stick to the 2 LGTM. No matter if that a bug fix or a new >>> feature. >>> >>> Currently we have PRs where 1 LGTM was given, but them the second >>> reviewer asked questions and raised concerns which were not answered >>> accordingly. If the 1 LGTM would have been applied, all concernes would >>> have been ignored. >>> >>> IMO, a PR siting for 7 or more days without a reply to the reviewer’s >>> questions/concerns should be closed without merge. In case it’s >>> really needed, the author will give it some attention and reopen it. >>> >>> Cheers, >>> Wilder >>> >>> >>> > On 14 Jul 2015, at 14:23, Koushik Das <koushik....@citrix.com> wrote: >>> > >>> > For bug fixes I feel 1 LGTM should be fine provided there is a Jira >>> ticket with all details and the request is pending for more than a >>> specified time (may be 7 days). For new features the existing process >>> should be fine. >>> > >>> > -----Original Message----- >>> > From: Wido den Hollander [mailto:w...@widodh.nl] >>> > Sent: Tuesday, 14 July 2015 17:42 >>> > To: dev@cloudstack.apache.org >>> > Subject: Re: [DISCUSS] PR list length >>> > >>> > -----BEGIN PGP SIGNED MESSAGE----- >>> > Hash: SHA1 >>> > >>> > >>> > >>> > On 14-07-15 13:56, Daan Hoogland wrote: >>> >> H, >>> >> >>> >> It is a concern to me that the list of PRs on our github page is >>> >> beyond a single page (maybe configurable but now a t a very >>> >> reasonable 25). I think we should adhere to a discipline of not >>> >> having any PRs open after the weekend. This is putting a very >>> >> strong statement outthere, I realize. A PR might be under heavy >>> >> construction and very big (which should result in a discussion >>> >> about splitting it!) I Discussed this with Wilder and the idea >>> >> popped up to have a seven day limit on (undicussed) PRs. This is >>> >> however more sensible from an automation point of view then from a >>> >> development discipline point of view. A regular cycle of >>> >> closing-or-discarding PRs makes more sense. >>> >> The list of PRs remaining open is slowly but very steadily growing >>> >> over time. >>> >> >>> >> thoughts? >>> >> >>> > >>> > I agree. I took the time to look at most of the PRs this morning, >>> > but a >>> lot of stuff is about code I don't know, so it's hard to vote LGTM on >>> such a PR. >>> > >>> > But I agree, 25+ PRs open is not good. >>> > >>> > Wido >>> > -----BEGIN PGP SIGNATURE----- >>> > Version: GnuPG v1 >>> > >>> > iQIcBAEBAgAGBQJVpPx1AAoJEAGbWC3bPspC5hsQAK1GxhgnHleFvexMOWOxZA3v >>> > 8XfR3Sh78DJZvG9hjY9eP4TauWJ6mBoVR5Mxe9M0eWqJ2Uy28PacIaUq4LXfrAY9 >>> > z5c+iq4Whi+FUz5mMtmL6x/3MqBlN8Ag9TDnZVE/pwDB1g8m27l23NhK6c5tKpXt >>> > CWh7xTqtCDVGnAO8eA1kk7aLicj3Wd8XaQzR4W7xDxf4XSN6lXEMfnFenD6ShqcA >>> > ktHOwI8r7hFt/M6+eZ7YmBF3dosw0mMH1lgBKaq+jEMSjHJWyVUu4UHxsx1Z9Fup >>> > nyYEEx8U5nYCgl72Zvmtvzth3Es2LoKy1ly19r6YlycMPtqO1T51qcwq3zcdKrpG >>> > 0pRPnEuQhMhUhJvuKOd05pEvISOf8Eilm+3k9W9ZxxYtgCe2cqgrn+0/60Uw0fzG >>> > 2U2lWlO4p4tYOKUbTSZTqsjYeeA0FvLV1Ib0wq8rwyZTHpxVpdWaz2lR2X3SltZH >>> > JJJsOdtMUxV3lzIBSL7fKjVi9TqbSgrd6QKio3jBl34cw8PStWIRZilIq+fglRnx >>> > BC0epH1YJAB3BzIeChe1kHKzrqADgo0arJt8N4n/Lza6+kW68k7hDx195XUgo3c0 >>> > OsKm2jkoo7JtURaOo6/lF+tFBngYdgTyWCFSer/UReycx/xnZcSI0DIz+QbYMOrj >>> > +Lg/AwfNPbXnNAFVQlrF >>> > =rzba >>> > -----END PGP SIGNATURE----- >>> >>> > > > > -- > Daan -- Daan
