[infinispan-dev] How to improve our PR queue
Sanne Grinovero
sanne at infinispan.org
Wed Oct 5 05:21:50 EDT 2016
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 <ttarrant at 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 <gfernand at redhat.com
>>> <mailto:gfernand at 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
>>> <slaskawi at redhat.com <mailto:slaskawi at 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 at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
More information about the infinispan-dev
mailing list