[QGIS-Developer] A plea: more volunteers needed for reviewing backports

Thu Apr 22 17:00:21 PDT 2021

On Fri, 9 Apr 2021 at 19:10, Matthias Kuhn <matthias at opengis.ch> wrote:
>
> Hi,
>
> Thanks for the continued effort in reviewing. It's one of the not-so-visible-highly-technically-and-socially-qualified-people-needed-but-very-important aspects of (open source) software development.
>
> I try to go over the PR queue every now and then and merge/comment/review what's possible, but these days I find less time than in the past. While at the same time it seems pull request activity increases it seems.
> I'll make sure we raise this topic at an upcoming OPENGIS.ch meeting and encourage people to participate.
>
> Additionally to what has been said already, backports are easy to forget / ignore by the original author because they have been opened by a bot and as an author you won't receive notifications by reviewers or the stale bot. I wonder if we could get the original author more involved if we'd mention them in the backport comment and increase their responsibility for this part. This might be worth some automation.

I definitely think this would help. The mentality at the moment is
predominantly that "as soon as the original PR is merged, the
responsibility is no longer mine" and that "someone else" will handle
the backports. In part this is due to how easy the bot has made
backporting.. we've now all got the mentality that the bot will handle
everything for us, but that's not the case in reality. I think
tweaking the backport message so that the author is mentioned will
help here, as at least they'll get notified if we close the backport
due to merge conflicts/etc. I'd also love to see the bot fixed so that
commits are cherry-picked and the ORIGINAL author name is attributed
to the commit. I think this is very important for accountability...
currently all the backport commits are anonymous and attributed to the
bot only, which makes it hard to tell who is responsible for the
change. I wish we could keep the original author here, as I think this
helps increase burden of responsiblity for that author... A poor
quality or buggy backport will directly reflect on their reputation,
so they are more likely to self-police backport PRs and ensure they
are suitable for merge. (At least, I hope so).

The other really painful thing with backport bot is that we have to
manually close and reopen ALL automated backports in order for the
tests to run. It's a minor thing, but definitely contributes to the
"chore" and drudgery of maintaining the PR list. Especially because
only a few have permission to do this, and unless the original
contributor has merge rights they can't even close/open their own
backports to help speed things along.

Don't get me wrong - backport bot *is* great, and has simplified work
a lot. But with a bit more investment and refinement it could be
incredible and save me substantially more time!

> Something else is that for example for me, the process to decide which backports get into pending backports was not obvious at first (only the LTR or also LR? At which stage of the LTR? How exactly do they get in there?), I'm sure for other people there are other parts of the process that are not immediately clear. I think documenting the review process could help here (the suggestions written by Nyall above for a lower entry barrier into the reviewer process would already be worth mentioning).

Right, we should definitely document this better. I'd suggest you and
I get together sometime to do this, as we've been appointed this
responsibility by PSC already. Can you ping me off list so we can
arrange this?

Nyall

>
> I hope I'll find more time again for reviews myself in the future.
> I actually enjoyed it a lot in the past; it has influenced my development a lot over the years as it allowed me to discuss concepts and approaches with experts from all over the globe, more than I had the chance before in the SMB offices I have been employed before.
>
> Matthias
>
> On Fri, Apr 9, 2021 at 9:21 AM Peter Petrik <peter.petrik at lutraconsulting.co.uk> wrote:
>>
>> Hi,
>>
>> First of all, thanks Nyall for your efforts. I try to review the code that we introduced to QGIS and I find kind-of familiar: for example QgsQuick, MeshLayer or MacOS stuff. But as mentioned in the thread, the less you know the particular code, the more time it takes to review it.  What I see that I can improve is to actively at least twice per week double check the PR queue to see if there is something in this area, since sometimes I need to be pinged to get a notification in email that the PR is waiting for review.
>>
>> What are my thoughts on other improvements not mentioned by Nyall and others:
>> 1. when someone introduces a new feature (and most notably coming from a paid contract), the review process should be taken into account in the quote and the reviewer should be notified. Proper review can take days...
>> 2.  we may have a (partially paid?) role for the review manager, to go through the PR queue, ping people for reviews, etc. (Alternatively) create some github-action bot to assign the maintainers to the PRs as reviewers?
>> 3. reintroduce the maintainers of particular QGIS code-base. For example here the motivation for companies could be to advertise their logo in the "sustainable member section"  or introducing some other category to advertise their efforts.
>>
>> Cheers,
>> Peter
>>
>>
>> On Fri, Apr 9, 2021 at 7:45 AM Nyall Dawson <nyall.dawson at gmail.com> wrote:
>>>
>>> On Thu, 8 Apr 2021 at 18:26, Julien Cabieces
>>> <julien.cabieces at oslandia.com> wrote:
>>> >
>>> >
>>> > Hi Nyall,
>>> >
>>> > We (at Oslandia) heard your call for more volunteers in reviewing the backports and PRs.
>>> >
>>> > We are very well aware that our contribution effort for this task is not enough. I see mainly two reasons:
>>> > - Lack of time and human ressources to fullfill this task. This is a bad reason, and a well known situation that we need to take care of internally, to free our schedule.
>>> > - A very partial knowledge of the code base in which we feel relevant to review. I try myself to review all Oracle related PRs, or take a look in some part of the code I know enough to make a good review,
>>>
>>> It would certainly be remiss of me not to acknowledge your hard work
>>> in maintaining the Oracle provider over recent months -- it's
>>> certainly very highly valued!
>>>
>>> > but it's very little compared to the QGIS code base.
>>>
>>> This is where things get really tricky. We do have a very small pool
>>> of people with the required expertise to handle the reviewing process,
>>> and it's a very fine balancing act of "opening this up to a wider
>>> community" vs "ensure that nothing nasty gets through". I don't have
>>> any easy answers here, but a few random thoughts:
>>>
>>> - ANY review is appreciated, even if it's just a "I had a look at this
>>> and the c++ code looks good to me, but I'm not familiar with the QGIS
>>> raster api so can someone else check this part out".
>>> - There's a lot of "chore" work in the pr review process which has a
>>> lower barrier of entry. For instance guiding new contributors through
>>> the QGIS processes such as the formatting scripts and CI setup. We
>>> regularly get PRs from first-time contributors and they almost always
>>> need some mentoring in the QGIS processes before they pass the CI
>>> setup and are ready for an in-depth review. This is the kind of thing
>>> which we should ideally do ASAP, as the faster we get the contributors
>>> through the initial maze of QGIS contributions the more likely they
>>> are to continue to contribute.
>>> - We've discussed this before, but a quick comment from a
>>> non-developer on new contributors PRs to say "great work, welcome
>>> abroad, this sounds like a very valuable contribution" is enough to
>>> give the project a positive vibe and give motivation to the submitter
>>> to get their PR in shape to merge
>>> - PR reviewing is a great way to learn new parts of the QGIS codebase ;)
>>>
>>> > I hesitated answering your first email, saying basically that we will try to improve the situation, but it would have been just a promise, and I'm not sure that we would have been able to keep it, regarding the points I described above.
>>>
>>> I really appreciate the reply, and the time you've taken to drop your
>>> thoughts into this conversation!
>>>
>>> Nyall
>>>
>>>
>>>
>>>
>>> >
>>> >
>>> > Kind regard,
>>> > Julien
>>> >
>>> > > On Tue, 23 Mar 2021 at 09:06, Nyall Dawson <nyall.dawson at gmail.com> wrote:
>>> > >>
>>> > >> Hi list,
>>> > >>
>>> > >> This is a public plea for more developers who are very familiar with
>>> > >> different parts of the QGIS codebase to become actively involved in
>>> > >> backport PR management.
>>> > >>
>>> > >> We NEED more people to be actively (i.e. daily) monitoring for
>>> > >> backport PRs and then reviewing them if the backport affects code
>>> > >> areas which they're knowledgeable about.
>>> > >>
>>> > >
>>> > > Well my earlier plea seems like it fell on deaf ears, and we've seen
>>> > > only minimal assistance coming to maintain the PR queue since I posted
>>> > > this.
>>> > >
>>> > > I'll ask nicely once more. If you're an organisation involved in QGIS
>>> > > development, you NEED to donate time to maintaining this list. It's
>>> > > not enough to just ensure your own PRs get through the queue, you HAVE
>>> > > to help with the shared burden of getting others PRs reviewed and
>>> > > merged.
>>> > >
>>> > > This callout applies to ALL organisations who are making money from
>>> > > core QGIS development. You know who you are, and you can ALL afford to
>>> > > donate 30 minutes of a staff member's time each week to helping with
>>> > > this shared burden. If your boss is blocking this then you NEED to let
>>> > > them know how big a risk they are running here. It's not fair to leave
>>> > > this responsibility on the already burdened shoulders of a few
>>> > > overworked individuals.
>>> > >
>>> > > Nyall
>>> > > _______________________________________________
>>> > > QGIS-Developer mailing list
>>> > > QGIS-Developer at lists.osgeo.org
>>> > > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>>> > > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>>> >
>>> > _______________________________________________
>>> > QGIS-Developer mailing list
>>> > QGIS-Developer at lists.osgeo.org
>>> > List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>>> > Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>>> _______________________________________________
>>> QGIS-Developer mailing list
>>> QGIS-Developer at lists.osgeo.org
>>> List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>>> Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>>
>> _______________________________________________
>> QGIS-Developer mailing list
>> QGIS-Developer at lists.osgeo.org
>> List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
>> Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer