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
