+1 - for the last time :D Originally I used +1/LGTM to let others look into the PR before integrating it. But I agree - it was impractical. Let's integrate stuff instead...
On Wed, Oct 5, 2016 at 11:51 AM, Dan Berindei <dan.berin...@gmail.com> wrote: > 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 >
_______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
