Proposed changes to merge request workflow

Fri Nov 8 12:28:30 UTC 2019

Hi!

I have contributed a patch or two to GHC, so I guess I’m a reasonable example of an newbie.

The step of nominating reviewers just wouldn’t work for me. I have no idea of who in this project would be willing and able to give a review. Or who the eligible reviewers are. Maybe I’d select someone who haven’t been active for years. 

If you do this, can you please add an alternative “I’m a clueless newbie, help me select reviewers” to that step?

Regards,
Niklas

> 8 nov. 2019 kl. 11:53 skrev Simon Peyton Jones via ghc-devs <ghc-devs at haskell.org>:
> 
> |  If the maintainers are not willing to either review or find reviewers
> |  for a new contributors patch
> |  then it doesn't seem to me that a project wants or values new
> |  contributors.
> 
> Yes, that would be an unfortunate -- and indeed wrong -- impression to convey.  Thanks for highlighting it.
> 
> You'd like the maintainers to have an *obligation* to cause someone to produce a good review on every patch. Here's the worst-case scenario: a well-meaning but inexperienced person produces a stream of large, ill-thought-out, and mostly wrong patches.  To give a guarantee of high quality reviews of those patches amounts to a blank cheque on the time of volunteers working mostly in their spare time.
> 
> Now, of course, that's an extreme scenario.  But that's why I'm keen to avoid making it an unconditional obligation that the few maintainers must discharge.
> 
> I don’t think there is really a difference of opinion here.  Of course we welcome patches; of course everyone will try to help find reviewers if they are lacking!
> 
> So how about this
> - the author nominates reviewers
> - if he or she finds difficulty in doing so, or the reviewers s/he
>  nominates are unresponsive, then he or she should ask for help
> - maintainers should make efforts to help
> 
> In other words, as an author you remain in control. But help is available if you need it.
> 
> What do others think?
> 
> Simon
> 
> |  -----Original Message-----
> |  From: Matthew Pickering <matthewtpickering at gmail.com>
> |  Sent: 08 November 2019 10:25
> |  To: Simon Peyton Jones <simonpj at microsoft.com>
> |  Cc: Ben Gamari <ben at well-typed.com>; ghc-devs at haskell.org
> |  Subject: Re: Proposed changes to merge request workflow
> |  
> |  If the maintainers are not willing to either review or find reviewers
> |  for a new contributors patch
> |  then it doesn't seem to me that a project wants or values new
> |  contributors.
> |  
> |  A maintainer can make a value judgement about a patch that is isn't
> |  worth reviewing, but such
> |  situations are exceedingly rare. Everyone contributes patches in good
> |  faith in order to make the compiler better.
> |  
> |  Realistically it's impossible to be a good reviewer without having
> |  implemented patches on the code base. If you don't
> |  have a good handle for how things work then it's too big to get a feel
> |  for just by reading the code. You need to learn how things
> |  fit together by getting stuck writing patches.
> |  
> |  At least some of the maintainers are paid to maintain GHC and as such,
> |  should be expected to perform responsibilities that
> |  volunteers are not willing to perform. One of these tasks should be
> |  finding reviewers for all patches and making sure contributions
> |  do not languish indefinitely.
> |  
> |  Apart from this one point the suggested process sounds good but it
> |  seems to have stalled in the last month.
> |  
> |  Cheers,
> |  
> |  Matt
> |  
> |  On Wed, Oct 9, 2019 at 11:31 AM Simon Peyton Jones
> |  <simonpj at microsoft.com> wrote:
> |  >
> |  > |  >  Make it clear that it is the contributor's responsibility to
> |  identify
> |  > |  reviewers for their merge requests.
> |  > |
> |  > |  Asking for reviews is one of the most frustrating parts of
> |  > |  contributing patches, even if you know who to ask! So I think the
> |  > |  maintainer's should be responsible for finding suitable and willing
> |  > |  reviewers.
> |  >
> |  > It is true that it's hard to find reviewers. But if it's hard for the
> |  author it is also hard for the maintainers.  A patch is a service that an
> |  author is offering, which is great.   But every patch is owed, as a matter
> |  of right, suitable and willing reviewers, the patch is /also/ a blank
> |  cheque that any author can write, but it's up to someone else to pay.
> |  That's not good either.  No author has an unlimited call on the time of
> |  other volunteers, and I don't think any author truly expects that.
> |  >
> |  > It's an informal gift economy.  I review your patches (a) because I have
> |  learned that you have good judgement and write good code (b) because I
> |  want the bug that you are fixing to be fixed and (c) because you give me
> |  all sorts of helpful feedback about my patches, or otherwise contribute to
> |  the community in constructive ways.
> |  >
> |  > That may make it hard for /new/ authors to get started.  Being an
> |  assiduous reviewer is an excellent plan, because it gets you into GHC's
> |  code base, guided by someone else's work; and it earns you all those good-
> |  contributor points.  But even then it may be hard.  So I think it's
> |  absolutely reasonable for authors to ask for help in finding reviewers.
> |  >
> |  > But simply saying that it's "the maintainers" responsibility to find
> |  reviewers goes much too far in the other direction, IMHO.
> |  >
> |  > Perhaps we should articulate some of this thinking.
> |  >
> |  > Simon
> |  >
> |  > |  -----Original Message-----
> |  > |  From: ghc-devs <ghc-devs-bounces at haskell.org> On Behalf Of Matthew
> |  > |  Pickering
> |  > |  Sent: 09 October 2019 11:18
> |  > |  To: Ben Gamari <ben at well-typed.com>
> |  > |  Cc: ghc-devs at haskell.org
> |  > |  Subject: Re: Proposed changes to merge request workflow
> |  > |
> |  > |  Sounds good in principal but I object to
> |  > |
> |  > |  >  Make it clear that it is the contributor's responsibility to
> |  identify
> |  > |  reviewers for their merge requests.
> |  > |
> |  > |  Asking for reviews is one of the most frustrating parts of
> |  > |  contributing patches, even if you know who to ask! So I think the
> |  > |  maintainer's should be responsible for finding suitable and willing
> |  > |  reviewers.
> |  > |
> |  > |  Cheers,
> |  > |
> |  > |  Matt
> |  > |
> |  > |  On Tue, Oct 8, 2019 at 7:17 PM Ben Gamari <ben at well-typed.com> wrote:
> |  > |  >
> |  > |  > tl;dr. I would like feedback on a few proposed changes [1] to our
> |  merge
> |  > |  >        request workflow.
> |  > |  >
> |  > |  >
> |  > |  > Hello everyone,
> |  > |  >
> |  > |  > Over the past six months I have been monitoring the operation of
> |  our
> |  > |  > merge request workflow, which arose rather organically in the wake
> |  of
> |  > |  > the initial move to GitLab. While it works reasonably well, there
> |  is
> |  > |  > clearly room for improvement:
> |  > |  >
> |  > |  >   * we have no formal way to track the status of in-flight merge
> |  > |  >     requests (e.g. for authors to mark an MR as ready for review or
> |  > |  >     reviewers to mark work as ready for merge)
> |  > |  >
> |  > |  >   * merge requests still at times languish without review
> |  > |  >
> |  > |  >   * the backport protocol is somewhat error prone and requires a
> |  great
> |  > |  >     deal of attention to ensure that patches don't slip through the
> |  > |  >     cracks
> |  > |  >
> |  > |  >   * there is no technical mechanism to prevent that under-reviewed
> |  > |  >     patches from being merged (either intentionally or otherwise)
> |  to
> |  > |  >     `master`
> |  > |  >
> |  > |  > To address this I propose [1] a few changes to our workflow:
> |  > |  >
> |  > |  >   1. Define explicit phases of the merge request lifecycle,
> |  > |  >      systematically identified with labels. This will help to make
> |  it
> |  > |  >      clear who is responsible for a merge request at every stage of
> |  its
> |  > |  >      lifecycle.
> |  > |  >
> |  > |  >   2. Make it clear that it is the contributor's responsibility to
> |  > |  >      identify reviewers for their merge requests.
> |  > |  >
> |  > |  >   3. Institute a final pre-merge sanity check to ensure that
> |  > |  >      patches are adequately reviewed, documented, tested, and have
> |  had
> |  > |  >      their ticket and MR metadata updated.
> |  > |  >
> |  > |  > Note that this is merely a proposal; I am actively seeking input
> |  from
> |  > |  > the developer community. Do let me know what you think.
> |  > |  >
> |  > |  > Cheers,
> |  > |  >
> |  > |  > - Ben
> |  > |  >
> |  > |  >
> |  > |  > [1]
> |  > |
> |  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.h
> |  > |  askell.org%2Fghc%2Fghc%2Fwikis%2Fproposals%2Fmerge-request-
> |  > |
> |  workflow&data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
> |  > |
> |  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
> |  > |
> |  52&sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&reserv
> |  > |  ed=0
> |  > |  > _______________________________________________
> |  > |  > ghc-devs mailing list
> |  > |  > ghc-devs at haskell.org
> |  > |  >
> |  > |
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  > |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  > |
> |  devs&data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  > |
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
> |  > |
> |  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&reserved=0
> |  > |  _______________________________________________
> |  > |  ghc-devs mailing list
> |  > |  ghc-devs at haskell.org
> |  > |
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  > |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  > |
> |  devs&data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  > |
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
> |  > |
> |  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&reserved=0
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs