Summary doc for CODEOWNERS, Mention-bot, Prow: https://docs.google.com/document/d/1S8spggJsxDNYZ7aNwZN6VhLhNW372SVRezjblt-7lNQ/edit?usp=sharing This doc will get updated as we gain experience with Mention-bot and Prow.
On Wed, Jul 25, 2018 at 5:15 PM Udi Meiri <eh...@google.com> wrote: > So I configured Prow using their getting started guide (and found a bug in > it) on a test repo. > > TLDR: Prow can work for us as a review assignment tool if all potential > reviewers are also added to the https://github.com/apache org. > > Some findings: > 1. Github doesn't allow non-collaborators to be listed as reviewers. :( > But! anyone added to the Apache org on Github may be added as a reviewer. > (no write access needed) > Is this something the ASF is willing to consider? > > 2. Prow works pretty well. I've configured it to just assign code > reviewers. > Here's an example of it in action: > https://github.com/udim-org/prow-test/pull/6 > Essentially, the command we would use are: > "/cc @user" - to explicitly add a reviewer (/uncc to remove) > > Other command in the example above are not necessary. > We can still use our current PR approval and merge process. > > 3. Prow currently tries to assign 2 code reviewers, and hopefully that's > configurable. > > Still unsure: > 1. How does Prow select reviewers? Does it load balance? > > On Mon, Jul 23, 2018 at 9:51 PM Jean-Baptiste Onofré <j...@nanthrax.net> > wrote: > >> It looks interesting but I would like to see the complete video and >> explanation about prow. Especially what we concretely need. >> >> Regards >> JB >> Le 24 juil. 2018, à 04:17, Udi Meiri <eh...@google.com> a écrit: >>> >>> I was recently told about Prow >>> <https://github.com/kubernetes/test-infra/tree/master/prow>, which >>> automates testing and merging for Kubernetes and other projects. >>> It also automates assigning reviewers and suggesting approvers. Example >>> <https://github.com/kubernetes/community/pull/2381> PR, video >>> explanation <https://youtu.be/BsIC7gPkH5M?t=19m41s> >>> I propose trying out Prow, since is it's a maintained and it uses OWNERS >>> files to explicitly define both who should be reviewing and who should >>> approve a PR. >>> >>> I'm not suggesting we use it to replace Jenkins or do our merges for us. >>> >>> >>> On Tue, Jul 17, 2018 at 11:04 AM Udi Meiri <eh...@google.com> wrote: >>> >>>> +1 to generating the file. >>>> I'll go ahead and file a PR to remove CODEOWNERS >>>> >>>> On Tue, Jul 17, 2018 at 9:28 AM Holden Karau <hol...@pigscanfly.ca> >>>> wrote: >>>> >>>>> So it doesn’t support doing that right now, although if we find it’s a >>>>> problem we can specify an exclude file with folks who haven’t contributed >>>>> in the past year. Would people want me to generate that first? >>>>> >>>>> On Tue, Jul 17, 2018 at 10:22 AM Ismaël Mejía <ieme...@gmail.com> >>>>> wrote: >>>>> >>>>>> Is there a way to put inactive people as not reviewers for the blame >>>>>> case? I think it can be useful considering that a good amount of our >>>>>> committers are not active at the moment and auto-assigning reviews to >>>>>> them seem like a waste of energy/time. >>>>>> On Tue, Jul 17, 2018 at 1:58 AM Eugene Kirpichov < >>>>>> kirpic...@google.com> wrote: >>>>>> > >>>>>> > We did not, but I think we should. So far, in 100% of the PRs I've >>>>>> authored, the default functionality of CODEOWNERS did the wrong thing >>>>>> and I >>>>>> had to fix something up manually. >>>>>> > >>>>>> > On Mon, Jul 16, 2018 at 3:42 PM Andrew Pilloud <apill...@google.com> >>>>>> wrote: >>>>>> >> >>>>>> >> This sounds like a good plan. Did we want to rename the CODEOWNERS >>>>>> file to disable github's mass adding of reviewers while we figure this >>>>>> out? >>>>>> >> >>>>>> >> Andrew >>>>>> >> >>>>>> >> On Mon, Jul 16, 2018 at 10:20 AM Jean-Baptiste Onofré < >>>>>> j...@nanthrax.net> wrote: >>>>>> >>> >>>>>> >>> +1 >>>>>> >>> >>>>>> >>> Le 16 juil. 2018, à 19:17, Holden Karau <holden.ka...@gmail.com> >>>>>> a écrit: >>>>>> >>>> >>>>>> >>>> Ok if no one objects I'll create the INFRA ticket after OSCON >>>>>> and we can test it for a week and decide if it helps or hinders. >>>>>> >>>> >>>>>> >>>> On Mon, Jul 16, 2018, 7:12 PM Jean-Baptiste Onofré < >>>>>> j...@nanthrax.net> wrote: >>>>>> >>>>> >>>>>> >>>>> Agree to test it for a week. >>>>>> >>>>> >>>>>> >>>>> Regards >>>>>> >>>>> JB >>>>>> >>>>> Le 16 juil. 2018, à 18:59, Holden Karau < >>>>>> holden.ka...@gmail.com> a écrit: >>>>>> >>>>>> >>>>>> >>>>>> Would folks be OK with me asking infra to turn on blame based >>>>>> suggestions for Beam and trying it out for a week? >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Jul 16, 2018, 6:53 PM Rafael Fernandez < >>>>>> rfern...@google.com> wrote: >>>>>> >>>>>>> >>>>>> >>>>>>> +1 using blame -- nifty :) >>>>>> >>>>>>> >>>>>> >>>>>>> On Mon, Jul 16, 2018 at 2:31 AM Huygaa Batsaikhan < >>>>>> bat...@google.com> wrote: >>>>>> >>>>>>>> >>>>>> >>>>>>>> +1. This is great. >>>>>> >>>>>>>> >>>>>> >>>>>>>> On Sat, Jul 14, 2018 at 7:44 AM Udi Meiri < eh...@google.com> >>>>>> wrote: >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Mention bot looks cool, as it tries to guess the reviewer >>>>>> using blame. >>>>>> >>>>>>>>> I've written a quick and dirty script that uses only >>>>>> CODEOWNERS. >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Its output looks like: >>>>>> >>>>>>>>> $ python suggest_reviewers.py --pr 5940 >>>>>> >>>>>>>>> INFO:root:Selected reviewer @lukecwik for: >>>>>> /runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/PTransformMatchers.java >>>>>> (path_pattern: /runners/core-construction-java*) >>>>>> >>>>>>>>> INFO:root:Selected reviewer @lukecwik for: >>>>>> /runners/core-construction-java/src/main/java/org/apache/beam/runners/core/construction/SplittableParDoNaiveBounded.java >>>>>> (path_pattern: /runners/core-construction-java*) >>>>>> >>>>>>>>> INFO:root:Selected reviewer @echauchot for: >>>>>> /runners/core-java/src/main/java/org/apache/beam/runners/core/SplittableParDoViaKeyedWorkItems.java >>>>>> (path_pattern: /runners/core-java*) >>>>>> >>>>>>>>> INFO:root:Selected reviewer @lukecwik for: >>>>>> /runners/flink/build.gradle (path_pattern: */build.gradle*) >>>>>> >>>>>>>>> INFO:root:Selected reviewer @lukecwik for: >>>>>> /runners/flink/src/main/java/org/apache/beam/runners/flink/FlinkTransformOverrides.java >>>>>> (path_pattern: *.java) >>>>>> >>>>>>>>> INFO:root:Selected reviewer @pabloem for: >>>>>> /runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/DataflowRunner.java >>>>>> (path_pattern: /runners/google-cloud-dataflow-java*) >>>>>> >>>>>>>>> INFO:root:Selected reviewer @lukecwik for: >>>>>> /sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/SplittableDoFnTest.java >>>>>> (path_pattern: /sdks/java/core*) >>>>>> >>>>>>>>> Suggested reviewers: @echauchot, @lukecwik, @pabloem >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> Script is in: https://github.com/apache/beam/pull/5951 >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> What does the community think? Do you prefer blame-based or >>>>>> rules-based reviewer suggestions? >>>>>> >>>>>>>>> >>>>>> >>>>>>>>> On Fri, Jul 13, 2018 at 11:13 AM Holden Karau < >>>>>> hol...@pigscanfly.ca> wrote: >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> I'm looking at something similar in the Spark project, and >>>>>> while it's now archived by FB it seems like something like >>>>>> https://github.com/facebookarchive/mention-bot might do what we >>>>>> want. I'm going to spin up a version on my K8 cluster and see if I can >>>>>> ask >>>>>> infra to add a webhook and if it works for Spark we could ask INFRA to >>>>>> add >>>>>> a second webhook for Beam. (Or if the Beam folks are more interested in >>>>>> experimenting I can do Beam first as a smaller project and roll with >>>>>> that). >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> Let me know :) >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> On Fri, Jul 13, 2018 at 10:53 AM, Eugene Kirpichov < >>>>>> kirpic...@google.com> wrote: >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> Sounds reasonable for now, thanks! >>>>>> >>>>>>>>>>> It's unfortunate that Github's CODEOWNERS feature appears >>>>>> to be effectively unusable for Beam but I'd hope that Github might pay >>>>>> attention and fix things if we submit feedback, with us being one of the >>>>>> most active Apache projects - did anyone do this yet / planning to? >>>>>> >>>>>>>>>>> >>>>>> >>>>>>>>>>> On Fri, Jul 13, 2018 at 10:23 AM Udi Meiri < >>>>>> eh...@google.com> wrote: >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> While I like the idea of having a CODEOWNERS file, the >>>>>> Github implementation is lacking: >>>>>> >>>>>>>>>>>> 1. Reviewers are automatically assigned at each push. >>>>>> >>>>>>>>>>>> 2. Reviewer assignment can be excessive (e.g. 5 >>>>>> reviewers in Eugene's PR 5940). >>>>>> >>>>>>>>>>>> 3. Non-committers aren't assigned as reviewers. >>>>>> >>>>>>>>>>>> 4. Non-committers can't change the list of reviewers. >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> I propose renaming the file to disable the auto-reviewer >>>>>> assignment feature. >>>>>> >>>>>>>>>>>> In its place I'll add a script that suggests reviewers. >>>>>> >>>>>>>>>>>> >>>>>> >>>>>>>>>>>> On Fri, Jul 13, 2018 at 9:09 AM Udi Meiri < >>>>>> eh...@google.com> wrote: >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> Hi Etienne, >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> Yes you could be as precise as you want. The paths I >>>>>> listed are just suggestions. :) >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>> On Fri, Jul 13, 2018 at 1:12 AM Jean-Baptiste Onofré < >>>>>> j...@nanthrax.net> wrote: >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> Hi, >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> I think it's already do-able just providing the >>>>>> expected path. >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> It's a good idea especially for the core. >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> Regards >>>>>> >>>>>>>>>>>>>> JB >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> On 13/07/2018 09:51, Etienne Chauchot wrote: >>>>>> >>>>>>>>>>>>>> > Hi Udi, >>>>>> >>>>>>>>>>>>>> > >>>>>> >>>>>>>>>>>>>> > I also have a question, related to what Eugene asked >>>>>> : I see that the >>>>>> >>>>>>>>>>>>>> > code paths are the ones of the modules. Can we be >>>>>> more precise than that >>>>>> >>>>>>>>>>>>>> > to assign reviewers ? As an example, I added myself >>>>>> to runner/core >>>>>> >>>>>>>>>>>>>> > because I wanted to take a look at the PRs related to >>>>>> >>>>>>>>>>>>>> > runner/core/metrics but I'm getting assigned to all >>>>>> runner-core PRs. Can >>>>>> >>>>>>>>>>>>>> > we specify paths like >>>>>> >>>>>>>>>>>>>> > >>>>>> runners/core-java/src/main/java/org/apache/beam/runners/core/metrics ? >>>>>> >>>>>>>>>>>>>> > I know it is a bit too precise so a bit risky, but >>>>>> in that particular >>>>>> >>>>>>>>>>>>>> > case, I doubt that the path will change. >>>>>> >>>>>>>>>>>>>> > >>>>>> >>>>>>>>>>>>>> > Etienne >>>>>> >>>>>>>>>>>>>> > >>>>>> >>>>>>>>>>>>>> > Le jeudi 12 juillet 2018 à 16:49 -0700, Eugene >>>>>> Kirpichov a écrit : >>>>>> >>>>>>>>>>>>>> >> Hi Udi, >>>>>> >>>>>>>>>>>>>> >> >>>>>> >>>>>>>>>>>>>> >> I see that the PR was merged - thanks! However it >>>>>> seems to have some >>>>>> >>>>>>>>>>>>>> >> unintended effects. >>>>>> >>>>>>>>>>>>>> >> >>>>>> >>>>>>>>>>>>>> >> On my PR https://github.com/apache/beam/pull/5940 >>>>>> , I assigned a >>>>>> >>>>>>>>>>>>>> >> reviewer manually, but the moment I pushed a new >>>>>> commit, it >>>>>> >>>>>>>>>>>>>> >> auto-assigned a lot of other people to it, and I >>>>>> had to remove them. >>>>>> >>>>>>>>>>>>>> >> This seems like a big inconvenience to me, is there >>>>>> a way to disable this? >>>>>> >>>>>>>>>>>>>> >> >>>>>> >>>>>>>>>>>>>> >> Thanks. >>>>>> >>>>>>>>>>>>>> >> >>>>>> >>>>>>>>>>>>>> >> On Thu, Jul 12, 2018 at 2:53 PM Udi Meiri < >>>>>> eh...@google.com >>>>>> >>>>>>>>>>>>>> >> <mailto: eh...@google.com>> wrote: >>>>>> >>>>>>>>>>>>>> >>> :/ That makes it a little less useful. >>>>>> >>>>>>>>>>>>>> >>> >>>>>> >>>>>>>>>>>>>> >>> On Thu, Jul 12, 2018 at 11:14 AM Tim Robertson >>>>>> >>>>>>>>>>>>>> >>> < timrobertson...@gmail.com <mailto: >>>>>> timrobertson...@gmail.com>> wrote: >>>>>> >>>>>>>>>>>>>> >>>> Hi Udi >>>>>> >>>>>>>>>>>>>> >>>> >>>>>> >>>>>>>>>>>>>> >>>> I asked the GH helpdesk and they confirmed that >>>>>> only people with >>>>>> >>>>>>>>>>>>>> >>>> write access will actually be automatically >>>>>> chosen. >>>>>> >>>>>>>>>>>>>> >>>> >>>>>> >>>>>>>>>>>>>> >>>> It don't expect it should stop us using it, but >>>>>> we should be aware >>>>>> >>>>>>>>>>>>>> >>>> that there are non-committers also willing to >>>>>> review. >>>>>> >>>>>>>>>>>>>> >>>> >>>>>> >>>>>>>>>>>>>> >>>> Thanks, >>>>>> >>>>>>>>>>>>>> >>>> Tim >>>>>> >>>>>>>>>>>>>> >>>> >>>>>> >>>>>>>>>>>>>> >>>> On Thu, Jul 12, 2018 at 7:24 PM, Mikhail Gryzykhin >>>>>> >>>>>>>>>>>>>> >>>> < mig...@google.com <mailto: mig...@google.com>> >>>>>> wrote: >>>>>> >>>>>>>>>>>>>> >>>>> Idea looks good in general. >>>>>> >>>>>>>>>>>>>> >>>>> >>>>>> >>>>>>>>>>>>>> >>>>> Did you look into ways to keep this file >>>>>> up-to-date? For example we >>>>>> >>>>>>>>>>>>>> >>>>> can run monthly job to see if owner was active >>>>>> during this period. >>>>>> >>>>>>>>>>>>>> >>>>> >>>>>> >>>>>>>>>>>>>> >>>>> --Mikhail >>>>>> >>>>>>>>>>>>>> >>>>> >>>>>> >>>>>>>>>>>>>> >>>>> Have feedback < http://go/migryz-feedback>? >>>>>> >>>>>>>>>>>>>> >>>>> >>>>>> >>>>>>>>>>>>>> >>>>> >>>>>> >>>>>>>>>>>>>> >>>>> On Thu, Jul 12, 2018 at 9:56 AM Udi Meiri < >>>>>> eh...@google.com >>>>>> >>>>>>>>>>>>>> >>>>> <mailto: eh...@google.com>> wrote: >>>>>> >>>>>>>>>>>>>> >>>>>> Thanks all! >>>>>> >>>>>>>>>>>>>> >>>>>> I'll try to get the file merged today and see >>>>>> how it works out. >>>>>> >>>>>>>>>>>>>> >>>>>> Please surface any issues, such as with >>>>>> auto-assignment, here or >>>>>> >>>>>>>>>>>>>> >>>>>> in JIRA. >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>> On Thu, Jul 12, 2018 at 2:12 AM Etienne Chauchot >>>>>> >>>>>>>>>>>>>> >>>>>> < echauc...@apache.org <mailto: >>>>>> echauc...@apache.org>> wrote: >>>>>> >>>>>>>>>>>>>> >>>>>>> Hi, >>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>> I added myself as a reviewer for some modules. >>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>> Etienne >>>>>> >>>>>>>>>>>>>> >>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>> Le lundi 09 juillet 2018 à 17:06 -0700, Udi >>>>>> Meiri a écrit : >>>>>> >>>>>>>>>>>>>> >>>>>>>> Hi everyone, >>>>>> >>>>>>>>>>>>>> >>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>>> I'm proposing to add auto-reviewer-assignment >>>>>> using Github's >>>>>> >>>>>>>>>>>>>> >>>>>>>> CODEOWNERS mechanism. >>>>>> >>>>>>>>>>>>>> >>>>>>>> Initial version is >>>>>> >>>>>>>>>>>>>> >>>>>>>> here: _ >>>>>> https://github.com/apache/beam/pull/5909/files_ >>>>>> >>>>>>>>>>>>>> >>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>>> I need help from the community in determining >>>>>> owners for each >>>>>> >>>>>>>>>>>>>> >>>>>>>> component. >>>>>> >>>>>>>>>>>>>> >>>>>>>> Feel free to directly edit the PR (if you >>>>>> have permission) or >>>>>> >>>>>>>>>>>>>> >>>>>>>> add a comment. >>>>>> >>>>>>>>>>>>>> >>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>>> Background >>>>>> >>>>>>>>>>>>>> >>>>>>>> The idea is to: >>>>>> >>>>>>>>>>>>>> >>>>>>>> 1. Document good review candidates for each >>>>>> component. >>>>>> >>>>>>>>>>>>>> >>>>>>>> 2. Help choose reviewers using the >>>>>> auto-assignment mechanism. >>>>>> >>>>>>>>>>>>>> >>>>>>>> The suggestion is in no way binding. >>>>>> >>>>>>>>>>>>>> >>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>>>>>> >>>>>> >>>>>>>>>>>>>> >>>> >>>>>> >>>>>>>>>>>>>> >>>>>> >>>>>>>>>>>>>> -- >>>>>> >>>>>>>>>>>>>> Jean-Baptiste Onofré >>>>>> >>>>>>>>>>>>>> jbono...@apache.org >>>>>> >>>>>>>>>>>>>> http://blog.nanthrax.net >>>>>> >>>>>>>>>>>>>> Talend - http://www.talend.com >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> >>>>>> >>>>>>>>>> -- >>>>>> >>>>>>>>>> Twitter: https://twitter.com/holdenkarau >>>>>> >>>>> -- >>>>> Twitter: https://twitter.com/holdenkarau >>>>> >>>>
smime.p7s
Description: S/MIME Cryptographic Signature
