I would also suggest avoiding +1/LGTM comments. It's enough to have the author and the integrator responsible in case something goes wrong, we don't need to "spread the blame" by having multiple +1 comments.
"LGTM, but" is fine if there's a specific area that you have doubts about, and you're assigning the PR to someone else. Otherwise, if it looks good, then we should integrate it without superfluous comments :) Cheers Dan On Wed, Oct 5, 2016 at 12:37 PM, Sebastian Laskawiec <slask...@redhat.com> wrote: > I agree with both Tristan and Sanne - they key point here is to make > reviewing PRs a habit. Doing it once a day (or even more often as Tristan > suggested) is a good place to start in my opinion. > > The "rebase and merge" can speed the things up but it doesn't solve the main > problem. We simply need to be more courageous when integrating PRs and avoid > blocking project's progress. > > Thanks > Sebastian > > > > On Wed, Oct 5, 2016 at 11:21 AM, Sanne Grinovero <sa...@infinispan.org> > wrote: >> >> Nice suggestions. I would be careful in actually *not* creating >> dedicated "pr merge sprints" as during such a sprint one would have >> more frequent merges, and so making the keep-up-rebasing and conflict >> resolving even more painful ;) >> >> I think the only solution is - like Tristan suggests - to make it a >> discipline to regularly check for PRs to be merged. >> >> Slightly unrelated, but it might help: did you notice that Github >> recently evolved their PR merge button? >> For trivial changes, i.e. if you're confident to not need to re-run >> the testsuite after a rebase, you can now just press the green button >> to have it rebase & merge, without creating a merge point. >> >> I just enabled the feature on the Infinispan repo ;) >> >> Sanne >> >> >> On 5 October 2016 at 09:18, Tristan Tarrant <ttarr...@redhat.com> wrote: >> > [Moving to infinispan-dev] >> > >> > I agree with all of you. >> > >> > It is not respectful towards the developer who has opened the PR to let >> > it linger for longer than necessary, and also for the developer who has >> > opened it not to react on the comments (that happens too!) >> > >> > Let's set some ground rules for Pull Requests (this should go in the >> > Contribution guide): >> > >> > Rules for everybody >> > - Each and everyone of the core engineers looks at the PR queue at least >> > once per day. More frequently if possible, especially if engaging in an >> > active collaborative review. >> > - We have more than one project: cachestores, console, integrations, >> > quickstarts, clients, website... >> > >> > Rules for PR owners: >> > - PRs *must* be kept "applicable" (no conflicts) >> > - non-trivial PRs *must* have a corresponding Jira and the link to it >> > *must* be present in the PR description. Viceversa, the Jira's workflow >> > must be advanced to "Pull Request Sent" and include the link to the PR >> > - if the PR is for multiple branches and it cherry-picks cleanly to all >> > of them, no need to open multiple PRs. Use the "Backport" label to >> > indicate this >> > - if there is a need for separate PRs for different branches, clearly >> > indicate the non-master branch it needs to be applied to in the PR title >> > using the [branch] notation >> > - Ensure the correct labels are applied when opening a PR or when the >> > status changes >> > - The owner *must* react to comments / CI failures / requests for rebase >> > within 24 hours. Reaction means acknowledgement, not necessarily being >> > able to fix the issues >> > - If the owner cannot fix the issues related to comments / CI within a >> > week, the PR should be closed and reopened when they can be addressed >> > - While we still have some intermittent CI failures, we're in a >> > situation where it is quite reliable. Exceptions obviously happen. >> > >> > Rules for PR reviewers >> > - A PR should be acted upon (commented, merged) within 24 hours of its >> > opening >> > - A PR can be commented upon even if CI hasn't finished its job >> > - PRs opened by external developers won't have the appropriate labels >> > for permission reasons. Add them. >> > - If the owner has addressed all comments / CI failures, the PR should >> > be merged within a week >> > - Some effects of a PR cannot be detected by CI. This for example >> > includes verifying that docs / PDFs / etc render correctly, that >> > distribution packages contain the appropriate files, etc. Do your best >> > to evaluate these. >> > >> > >> > With that being said, the solution for the current situation is to >> > divide the queue among ourselves and work through it using the sprint >> > approach (but we need to be careful with stability: "commit storms" can >> > be detrimental). >> > >> > Tristan >> > >> > On 05/10/16 09:39, Radim Vansa wrote: >> >> I think that can be combined: sprint just explicitly prioritizes PRs >> >> instead of normal development, so you won't be reviewing PRs only when >> >> "you have a free spot" but "until you can't review anything else". So >> >> once the PR count hits a threshold, Tristan schedules sprint. >> >> >> >> R. >> >> >> >> On 10/05/2016 09:17 AM, Sebastian Laskawiec wrote: >> >>> That's an interesting idea... but after a month or two, the PR queue >> >>> will pile up again and we will need another sprint... >> >>> >> >>> IMO we should have a day-to-day strategy for doing this. >> >>> >> >>> On Wed, Oct 5, 2016 at 9:07 AM, Gustavo Fernandes <gfern...@redhat.com >> >>> <mailto:gfern...@redhat.com>> wrote: >> >>> >> >>> I propose doing with PRs the same we've been doing to areas that >> >>> require some care, the so called 'sprints'. >> >>> >> >>> We are in more than 10, a couple of PRs integrated each and we can >> >>> get >> >>> rid of most of the queue. >> >>> >> >>> Gustavo >> >>> >> >>> On Wed, Oct 5, 2016 at 7:36 AM, Sebastian Laskawiec >> >>> <slask...@redhat.com <mailto:slask...@redhat.com>> wrote: >> >>> > Hey guys! >> >>> > >> >>> > I'm not sure what do you think about our PR queue but for me it >> >>> simply >> >>> > sucks... >> >>> > >> >>> > Some of our PRs are sitting there since I remember (like this >> >>> one [1] - Nov >> >>> > 2015!!! I guess we should prepare an anniversary cake, November >> >>> will is >> >>> > really soon :D) and some of them have more than 150 comments [2] >> >>> (maybe this >> >>> > sounds weird since it's my PR, but if something is not good >> >>> enough after 150 >> >>> > comments, maybe we should throw it away and try to implement it >> >>> again - >> >>> > differently?). >> >>> > >> >>> > After thinking about this for a little while, I would like to >> >>> propose one >> >>> > rule for reviewing PRs - when you have a free spot and you'd >> >>> like to >> >>> > integrate a couple of PRs - always start with the oldest and go >> >>> through all >> >>> > "Ready for review" PRs. >> >>> > >> >>> > I know that integrating some new stuff will be tempting but >> >>> please don't do >> >>> > that. Even if the new PR modifies only one small line.. noooo. >> >>> Go through >> >>> > some old stuff instead. This way we should revisit old PRs much >> >>> more >> >>> > frequently than new ones and this will force us to make some >> >>> tough decisions >> >>> > that we avoided for quite some time (e.g. whether or not we >> >>> should integrate >> >>> > [1] or [2] - but those are only examples). >> >>> > >> >>> > And finally, I think we should have more courage to integrate >> >>> PRs especially >> >>> > into master branch... After all, we have all machinery in place >> >>> - nightly CI >> >>> > jobs, stress tests, performance tests - what bad can happen? We >> >>> break the >> >>> > build? So what? We can always fix it by git revert. >> >>> > >> >>> > What do you think? Maybe you have better ideas? >> >>> > >> >>> > Thanks >> >>> > Seb >> >>> > >> >>> > [1] https://github.com/infinispan/infinispan/pull/3860 >> >>> <https://github.com/infinispan/infinispan/pull/3860> >> >>> > [2] https://github.com/infinispan/infinispan/pull/4348 >> >>> <https://github.com/infinispan/infinispan/pull/4348> >> >>> >> >>> >> >> >> >> >> > >> > -- >> > Tristan Tarrant >> > Infinispan Lead >> > JBoss, a division of Red Hat >> > _______________________________________________ >> > infinispan-dev mailing list >> > infinispan-dev@lists.jboss.org >> > https://lists.jboss.org/mailman/listinfo/infinispan-dev >> _______________________________________________ >> infinispan-dev mailing list >> infinispan-dev@lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
