Proposed changes to merge request workflow
Simon Peyton Jones
simonpj at microsoft.com
Fri Nov 8 10:52:58 UTC 2019
| 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
More information about the ghc-devs
mailing list