Thanks Chris for the great reading suggestion <http://www.unterstein.net/su/docs/CathBaz.pdf>!
I'm suggesting that we adopt Mu's proposal to use github code owner mechanism to identify designated maintainer for each package. To address the concerns raised in this thread I proposed to add into the header of the CODEOWNERS file https://github.com/apache/incubator-mxnet/pull/9426 (changes below). Chris, Sebastian, Isabel - please suggest changes, but I hope I addressed your concerns. In the future we can also enable required reviews (see https://help.github.com/articles/about-pull-request-reviews/), but I would suggest to make one change at a time. I do suggest we should explore how we can best adopt existing github features before considering building additional CI tasks. Steffen # Please see documentation of use of CODEOWNERS file at # https://help.github.com/articles/about-codeowners/ and # https://github.com/blog/2392-introducing-code-owners # # The first owner listed for a package is considered the maintainer for a package. # Anybody can add themselves or a team (see https://help.github.com/articles/about-teams/) # as additional owners to get notified about changes in a specific package. # # By default the package maintainer should merge PR after appropriate review. # A PR which received 2 +1 (or LGTM) comments can be merged by any committer. # In the future we might consider adopting required reviews # (see https://help.github.com/articles/about-pull-request-reviews/) On Fri, Jan 12, 2018 at 7:22 PM Bhavin Thaker <[email protected]> wrote: > During the MXNet 1.0 release, there was feedback from the mentors and folks > in general@ to clarify at the top of the CODEOWNERs file on what the > contents of this file meant. > > Hi Mu, > > Please add the description of the file in the file header. I expect that > this will be a requirement for the next MXNet release 1.0.1. > > Thanks, > Bhavin Thaker. > > On Fri, Jan 12, 2018 at 5:43 PM Chris Olivier <[email protected]> > wrote: > > > i’d be +1 if CODEOWNERS file has a big note at the top saying basically > > it’s just for watching code changes that you’d like to know about (to > > review or just to follow) and that anyone can add themself. > > > > On Fri, Jan 12, 2018 at 1:58 PM Chris Olivier <[email protected]> > > wrote: > > > > > Does it have to be called "CODEOWNERS"? I would be more comfortable > with > > > it if it's a "watch list" where it just means you wish to watch code > here > > > or there in the source structure and anyone can add or remove their > name > > > from watching some part of the code at any time. > > > > > > On Fri, Jan 12, 2018 at 11:52 AM, Marco de Abreu < > > > [email protected]> wrote: > > > > > >> I agree. How about we find another way to allow people to subscribe > for > > >> changes in a specific file or directory? > > >> > > >> -Marco > > >> > > >> Am 12.01.2018 8:51 nachm. schrieb "Chris Olivier" < > > [email protected] > > >> >: > > >> > > >> > Have you read "The Cathedral and the Bazaar"? > > >> > > > >> > http://www.unterstein.net/su/docs/CathBaz.pdf > > >> > > > >> > One of the points I took from this is that once a project finds its > > >> stride, > > >> > it actually runs more efficiently without centralization than with. > > >> > > > >> > -Chris > > >> > > > >> > On Fri, Jan 12, 2018 at 11:10 AM, Marco de Abreu < > > >> > [email protected]> wrote: > > >> > > > >> > > Hi Chris, > > >> > > > > >> > > you have a good point about people being afraid of reviewing PRs > > which > > >> > they > > >> > > are not assigned to and I totally agree that we should encourage > > >> > everybody > > >> > > to review PRs. > > >> > > > > >> > > One important advantage I see in this is the notification: since > we > > >> are > > >> > not > > >> > > using the feature to required an approval, this step is entirely > for > > >> > > information purpose. I, for example, would like to get notified > if a > > >> PR > > >> > to > > >> > > change a CI file would be created. Just as an example: over > > >> Christmas, a > > >> > PR > > >> > > to update mkl has been pushed without me knowing about it. > Somehow, > > >> after > > >> > > my vacation, we started to get issues with mkl test - I only found > > out > > >> > > about this PR after quite a long investigation. If we would extend > > the > > >> > > usage of the code maintainers, we'll make sure that changes like > > these > > >> > will > > >> > > notify the people who have the best knowledge about that part. > > >> > > > > >> > > Marco > > >> > > > > >> > > Am 12.01.2018 8:03 nachm. schrieb "Chris Olivier" < > > >> [email protected] > > >> > >: > > >> > > > > >> > > > -1 (binding) > > >> > > > > > >> > > > I totally understand the motivation for this (I've definitely > > saved > > >> > > myself > > >> > > > some grief by getting called out automatically for > CMakeLists.txt > > >> > stuff, > > >> > > > for example), but I respectfully decline for the following > > >> reason(s): > > >> > > > > > >> > > > I feel that defining code-owners has some negative effects. > > >> > > > > > >> > > > Other committers may be reluctant to start reviewing and > approving > > >> PRs > > >> > > > since they aren't the one listed, so I feel this will in the > > >> long-run > > >> > > > reduce the number of people doing code reviews. > > >> > > > > > >> > > > If there aren't enough people doing PR's, then people can > complain > > >> on > > >> > > dev@ > > >> > > > asking for review. > > >> > > > > > >> > > > -Chris > > >> > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > On Fri, Jan 12, 2018 at 10:41 AM, Haibin Lin <[email protected] > > > > >> > wrote: > > >> > > > > > >> > > > > +1 (binding) > > >> > > > > > > >> > > > > On 2018-01-12 10:10, kellen sunderland < > > >> [email protected]> > > >> > > > > wrote: > > >> > > > > > +1 (non-binding) > > >> > > > > > > > >> > > > > > On Jan 12, 2018 6:32 PM, "Steffen Rochel" < > > >> [email protected] > > >> > > > > >> > > > > wrote: > > >> > > > > > > > >> > > > > > > I propose to adopt the proposal. > > >> > > > > > > +1 (non-binding) > > >> > > > > > > > > >> > > > > > > Steffen > > >> > > > > > > > > >> > > > > > > On Wed, Jan 10, 2018 at 8:39 PM Mu Li <[email protected] > > > > >> > wrote: > > >> > > > > > > > > >> > > > > > > > Hi Isabel, > > >> > > > > > > > > > >> > > > > > > > My apologies that not saying that clearly. > > >> > > > > > > > > > >> > > > > > > > The purpose of this proposal is encouraging more > > >> contributors > > >> > to > > >> > > > help > > >> > > > > > > > review and merge PRs. And also hope to shorten the time > > for > > >> a > > >> > PR > > >> > > to > > >> > > > > be > > >> > > > > > > > merged. After assigning maintainers to modules, then PR > > >> > > > contributors > > >> > > > > can > > >> > > > > > > > easily contact the reviewers. In other words, github > will > > >> > > > > automatically > > >> > > > > > > > assign the PR to the maintainer and send a notification > > >> email. > > >> > > > > > > > > > >> > > > > > > > I don't think I put the term "inbox" in my proposal. I > > never > > >> > > > > discussed > > >> > > > > > > PRs > > >> > > > > > > > with other contributors by sending email directly, which > > is > > >> > less > > >> > > > > > > effective > > >> > > > > > > > than just using github. I also don't aware any other > > >> > contributor > > >> > > > use > > >> > > > > the > > >> > > > > > > > direct email way. So I didn't clarify it on the > proposal. > > >> > > > > > > > > > >> > > > > > > > On Tue, Jan 9, 2018 at 11:47 AM, Isabel Drost-Fromm < > > >> > > > > [email protected]> > > >> > > > > > > > wrote: > > >> > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > Am 9. Januar 2018 18:25:50 MEZ schrieb Mu Li < > > >> > > [email protected] > > >> > > > >: > > >> > > > > > > > > >We should encourage to contract a specific > contributor > > >> for > > >> > > > issues > > >> > > > > and > > >> > > > > > > > > >PRs. > > >> > > > > > > > > > > >> > > > > > > > > My head translates "encourage to contact specific > > >> > contributor" > > >> > > > into > > >> > > > > > > > > "encourage to contact specific contributors inbox". > This > > >> > > > translated > > >> > > > > > > > version > > >> > > > > > > > > is what I would highly discourage. > > >> > > > > > > > > > > >> > > > > > > > > See the disclaimer here for reasons behind that: > > >> > > > > > > > > > > >> > > > > > > > > https://home.apache.org/~hossman/#private_q > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > Isabel > > >> > > > > > > > > -- > > >> > > > > > > > > Diese Nachricht wurde von meinem Android-Gerät mit K-9 > > >> Mail > > >> > > > > gesendet. > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > > > > >
