Just wanted to clarify my understanding. 1. We are going to use Github CODEOWNERS functionality as a feature for getting notified. 2. This does not mean only CODEOWNERS approved code will be merged for respective module. (We need to evolve community to self-sustain without getting blocked on one poc)
Regards, Sandeep On Sun, Jan 14, 2018 at 7:43 PM, sandeep krishnamurthy < [email protected]> wrote: > +1 (binding) for suggestion around framing CODEOWNERS functionality as the > watchlist. > I also feel that we should enable and find/request more than 1 person per > module to help the project. > > But, still, if it is something like +1 or watch button for modules rather > than a new PR to follow a topic, it would have been great. Something for > future :-) > > Regards, > Sandeep > > On Sun, Jan 14, 2018 at 4:18 PM, Steffen Rochel <[email protected]> > wrote: > >> 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. >> > > >> > > > > > > > > >> > > >> > > > > > > > >> > > >> > > > > > > >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > >> > > >> > > >> > >> > > >> >> > > > >> > > > >> > > >> > >> > > > > -- > Sandeep Krishnamurthy > -- Sandeep Krishnamurthy
