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
