Proposed changes to merge request workflow

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Proposed changes to merge request workflow

Ben Gamari-3
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://gitlab.haskell.org/ghc/ghc/wikis/proposals/merge-request-workflow

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

GHC - devs mailing list
All sounds very sensible to me.

On Tue, Oct 8, 2019 at 2:17 PM Ben Gamari <[hidden email]> 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://gitlab.haskell.org/ghc/ghc/wikis/proposals/merge-request-workflow
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


--
Shayne Fletcher
Language Engineer / +1 917 699 7663
Digital Asset, creators of DAML

This message, and any attachments, is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at http://www.digitalasset.com/emaildisclaimer.html. If you are not the intended recipient, please delete this message.
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Richard Eisenberg-5
+1 from me.

On Oct 8, 2019, at 7:19 PM, Shayne Fletcher via ghc-devs <[hidden email]> wrote:

All sounds very sensible to me.

On Tue, Oct 8, 2019 at 2:17 PM Ben Gamari <[hidden email]> 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://gitlab.haskell.org/ghc/ghc/wikis/proposals/merge-request-workflow
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


--
Shayne Fletcher
Language Engineer / +1 917 699 7663
Digital Asset, creators of DAML

This message, and any attachments, is for the intended recipient(s) only, may contain information that is privileged, confidential and/or proprietary and subject to important terms and conditions available at http://www.digitalasset.com/emaildisclaimer.html. If you are not the intended recipient, please delete this message._______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Matthew Pickering
In reply to this post by Ben Gamari-3
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 <[hidden email]> 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://gitlab.haskell.org/ghc/ghc/wikis/proposals/merge-request-workflow
> _______________________________________________
> ghc-devs mailing list
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Oleg Grenrus
+1

On 9.10.2019 13.18, Matthew Pickering wrote:

> 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 <[hidden email]> 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://gitlab.haskell.org/ghc/ghc/wikis/proposals/merge-request-workflow
>> _______________________________________________
>> ghc-devs mailing list
>> [hidden email]
>> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
> _______________________________________________
> ghc-devs mailing list
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: Proposed changes to merge request workflow

GHC - devs mailing list
In reply to this post by Matthew Pickering
|  >  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 <[hidden email]> On Behalf Of Matthew
|  Pickering
|  Sent: 09 October 2019 11:18
|  To: Ben Gamari <[hidden email]>
|  Cc: [hidden email]
|  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 <[hidden email]> 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&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
|  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
|  52&amp;sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&amp;reserv
|  ed=0
|  > _______________________________________________
|  > ghc-devs mailing list
|  > [hidden email]
|  >
|  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
|  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
|  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
|  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
|  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&amp;reserved=0
|  _______________________________________________
|  ghc-devs mailing list
|  [hidden email]
|  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
|  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
|  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
|  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
|  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&amp;reserved=0
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Oleg Grenrus
Becoming a recognized reviewer before starting writing code feels
perverse for me.

Linux kernel writes in
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#select-the-recipients-for-your-patch 
as follows. (Note that patches are submitted by sending emails, read
accordingly)

 > You should always copy the appropriate subsystem maintainer(s) on any
patch to code that they maintain; look through the MAINTAINERS file and
the source code revision history to see who those maintainers are. The
script scripts/get_maintainer.pl can be very useful at this step. If you
cannot find a maintainer for the subsystem you are working on, Andrew
Morton ([hidden email]) serves as a maintainer of last resort.

GHC already has CODEOWNERS file, and there could be a person of last
resort. There should be more names there though, e.g. /libraries/base/
should have the whole CLC, not only HVR; template-haskell could have
Ryan Scott, etc. As the first commit message of CODEOWNERS says:

 > GitLab uses this file to suggest reviewers based upon the files that
a Merge Request touches.

Kernel guidelines also have a section about trivial patches. Thing to
learn, there is a light way to get trivial patches in, but what's a
trivial patch is objectively defined. An actual trivial patch monkey is
a real person, but to my understanding it's a circulating role.

 > For small patches you may want to CC the Trivial Patch Monkey
[hidden email] which collects “trivial” patches. Have a look into
the MAINTAINERS file for its current manager.
 >
 > Trivial patches must qualify for one of the following rules:
 >
 > - Spelling fixes in documentation
 > - Spelling fixes for errors which could break grep(1)
 > - Warning fixes (cluttering with useless warnings is bad)
 > - Compilation fixes (only if they are actually correct)
 > - Runtime fixes (only if they actually fix things)
 > - Removing use of deprecated functions/macros
 > - Contact detail and documentation fixes
 > - Non-portable code replaced by portable code (even in arch-specific,
since people copy, as long as it’s trivial)
 > - Any fix by the author/maintainer of the file (ie. patch monkey in
re-transmission mode)

- Oleg


On 9.10.2019 13.31, Simon Peyton Jones via ghc-devs 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 <[hidden email]> On Behalf Of Matthew
> |  Pickering
> |  Sent: 09 October 2019 11:18
> |  To: Ben Gamari <[hidden email]>
> |  Cc: [hidden email]
> |  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 <[hidden email]> 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&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
> |  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
> |  52&amp;sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&amp;reserv
> |  ed=0
> |  > _______________________________________________
> |  > ghc-devs mailing list
> |  > [hidden email]
> |  >
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
> |  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&amp;reserved=0
> |  _______________________________________________
> |  ghc-devs mailing list
> |  [hidden email]
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
> |  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&amp;reserved=0
> _______________________________________________
> ghc-devs mailing list
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Matthew Pickering
In reply to this post by GHC - devs mailing list
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
<[hidden email]> 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 <[hidden email]> On Behalf Of Matthew
> |  Pickering
> |  Sent: 09 October 2019 11:18
> |  To: Ben Gamari <[hidden email]>
> |  Cc: [hidden email]
> |  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 <[hidden email]> 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&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
> |  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
> |  52&amp;sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&amp;reserv
> |  ed=0
> |  > _______________________________________________
> |  > ghc-devs mailing list
> |  > [hidden email]
> |  >
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
> |  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&amp;reserved=0
> |  _______________________________________________
> |  ghc-devs mailing list
> |  [hidden email]
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
> |  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&amp;reserved=0
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

RE: Proposed changes to merge request workflow

GHC - devs mailing list
|  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 <[hidden email]>
|  Sent: 08 November 2019 10:25
|  To: Simon Peyton Jones <[hidden email]>
|  Cc: Ben Gamari <[hidden email]>; [hidden email]
|  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
|  <[hidden email]> 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 <[hidden email]> On Behalf Of Matthew
|  > |  Pickering
|  > |  Sent: 09 October 2019 11:18
|  > |  To: Ben Gamari <[hidden email]>
|  > |  Cc: [hidden email]
|  > |  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 <[hidden email]> 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&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
|  > |
|  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
|  > |
|  52&amp;sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&amp;reserv
|  > |  ed=0
|  > |  > _______________________________________________
|  > |  > ghc-devs mailing list
|  > |  > [hidden email]
|  > |  >
|  > |
|  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
|  > |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
|  > |
|  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
|  > |
|  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
|  > |
|  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&amp;reserved=0
|  > |  _______________________________________________
|  > |  ghc-devs mailing list
|  > |  [hidden email]
|  > |
|  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
|  > |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
|  > |
|  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
|  > |
|  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
|  > |
|  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&amp;reserved=0
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Niklas Larsson
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 <[hidden email]>:
>
> |  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 <[hidden email]>
> |  Sent: 08 November 2019 10:25
> |  To: Simon Peyton Jones <[hidden email]>
> |  Cc: Ben Gamari <[hidden email]>; [hidden email]
> |  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
> |  <[hidden email]> 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 <[hidden email]> On Behalf Of Matthew
> |  > |  Pickering
> |  > |  Sent: 09 October 2019 11:18
> |  > |  To: Ben Gamari <[hidden email]>
> |  > |  Cc: [hidden email]
> |  > |  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 <[hidden email]> 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&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
> |  > |
> |  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
> |  > |
> |  52&amp;sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&amp;reserv
> |  > |  ed=0
> |  > |  > _______________________________________________
> |  > |  > ghc-devs mailing list
> |  > |  > [hidden email]
> |  > |  >
> |  > |
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  > |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  > |
> |  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  > |
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
> |  > |
> |  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&amp;reserved=0
> |  > |  _______________________________________________
> |  > |  ghc-devs mailing list
> |  > |  [hidden email]
> |  > |
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  > |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  > |
> |  devs&amp;data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  > |
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
> |  > |
> |  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&amp;reserved=0
> _______________________________________________
> ghc-devs mailing list
> [hidden email]
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Ben Gamari-2
In reply to this post by Matthew Pickering
Matthew Pickering <[hidden email]> writes:

> 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.
>
For what it's worth, I am happy to try to find reviewers for a
newcomer's patch. However, on the whole it is better for everyone
involved if the contributor does it:

 * the contributor is more involved in the process and, consequently,
   more invested

 * the process moves more quickly since the contributor doesn't need to
   wait for someone else to find reviewers for their work

 * me and the rest of us at Well-Typed are less of a bottleneck and
   therefore have more time for improving GHC
 
Of course, even with this policy, if I see a patch languishing then I
will try to handle it. In my view all we are doing here is setting the
preferred default; .

> 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.
>
Indeed I've been stuck in an endless cycle of pre-release tasks.
Hopefully this will end today.

Cheers,

- Ben


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: Proposed changes to merge request workflow

Ben Gamari-2
In reply to this post by GHC - devs mailing list
Simon Peyton Jones via ghc-devs <[hidden email]> writes:

> |  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 my mind there has always been a (perhaps too implicit) promise that
maintainers are always present in the background and happy to help in
finding reviewers if asked (and perhaps even if not, if it seems a
contributor is lost).

Perhaps we should make this more explicit?

Cheers,

- Ben


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Richard Eisenberg-5
I wonder if it would alleviate the concerns to have a ghc-maintainers mailing list. This is distinct from ghc-devs, in that the maintainers have GHC as their day job. It would explicitly invite email from folks struggling to figure out how to contribute. I don't mean to create more mail for Ben et al, but having an explicit "seek help here" direction is nice. And (at least for me) mailing a list for help feels more comfortable than emailing an individual.

Richard

On Nov 8, 2019, at 6:30 PM, Ben Gamari <[hidden email]> wrote:

Simon Peyton Jones via ghc-devs <[hidden email]> writes:

|  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 my mind there has always been a (perhaps too implicit) promise that
maintainers are always present in the background and happy to help in
finding reviewers if asked (and perhaps even if not, if it seems a
contributor is lost).

Perhaps we should make this more explicit?

Cheers,

- Ben

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs


_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
Reply | Threaded
Open this post in threaded view
|

Re: Proposed changes to merge request workflow

Alan & Kim Zimmerman
What about some sort of script that detects MR older than x time without a reviewer, and asks a group of people to take a look.

On Fri, 8 Nov 2019 at 19:36, Richard Eisenberg <[hidden email]> wrote:
I wonder if it would alleviate the concerns to have a ghc-maintainers mailing list. This is distinct from ghc-devs, in that the maintainers have GHC as their day job. It would explicitly invite email from folks struggling to figure out how to contribute. I don't mean to create more mail for Ben et al, but having an explicit "seek help here" direction is nice. And (at least for me) mailing a list for help feels more comfortable than emailing an individual.

Richard

On Nov 8, 2019, at 6:30 PM, Ben Gamari <[hidden email]> wrote:

Simon Peyton Jones via ghc-devs <[hidden email]> writes:

|  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 my mind there has always been a (perhaps too implicit) promise that
maintainers are always present in the background and happy to help in
finding reviewers if asked (and perhaps even if not, if it seems a
contributor is lost).

Perhaps we should make this more explicit?

Cheers,

- Ben

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs

_______________________________________________
ghc-devs mailing list
[hidden email]
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs