Hi, so I saw mention bot working <https://github.com/apache/beam/pull/6104#issuecomment-409077055> this week. How was the quality of suggestions?
Holden, I would like to start testing Prow starting next week if that's possible. I'll be opening a ticket to INFRA to give my Github bot account read access (for requesting reviews). On Fri, Jul 27, 2018, 09:37 Udi Meiri <[email protected]> wrote: > 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 <[email protected]> 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é <[email protected]> >> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>>> 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 < >>>>>>> [email protected]> 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 < >>>>>>> [email protected]> 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é < >>>>>>> [email protected]> wrote: >>>>>>> >>> >>>>>>> >>> +1 >>>>>>> >>> >>>>>>> >>> Le 16 juil. 2018, à 19:17, Holden Karau <[email protected]> >>>>>>> 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é < >>>>>>> [email protected]> wrote: >>>>>>> >>>>> >>>>>>> >>>>> Agree to test it for a week. >>>>>>> >>>>> >>>>>>> >>>>> Regards >>>>>>> >>>>> JB >>>>>>> >>>>> Le 16 juil. 2018, à 18:59, Holden Karau < >>>>>>> [email protected]> 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 < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> +1 using blame -- nifty :) >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Jul 16, 2018 at 2:31 AM Huygaa Batsaikhan < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> >>>>>>> >>>>>>>> +1. This is great. >>>>>>> >>>>>>>> >>>>>>> >>>>>>>> On Sat, Jul 14, 2018 at 7:44 AM Udi Meiri < >>>>>>> [email protected]> 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 < >>>>>>> [email protected]> 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 < >>>>>>> [email protected]> 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 < >>>>>>> [email protected]> 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 < >>>>>>> [email protected]> 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é < >>>>>>> [email protected]> 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 < >>>>>>> [email protected] >>>>>>> >>>>>>>>>>>>>> >> <mailto: [email protected]>> wrote: >>>>>>> >>>>>>>>>>>>>> >>> :/ That makes it a little less useful. >>>>>>> >>>>>>>>>>>>>> >>> >>>>>>> >>>>>>>>>>>>>> >>> On Thu, Jul 12, 2018 at 11:14 AM Tim Robertson >>>>>>> >>>>>>>>>>>>>> >>> < [email protected] <mailto: >>>>>>> [email protected]>> 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 >>>>>>> >>>>>>>>>>>>>> >>>> < [email protected] <mailto: [email protected]>> >>>>>>> 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 < >>>>>>> [email protected] >>>>>>> >>>>>>>>>>>>>> >>>>> <mailto: [email protected]>> 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 >>>>>>> >>>>>>>>>>>>>> >>>>>> < [email protected] <mailto: >>>>>>> [email protected]>> 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é >>>>>>> >>>>>>>>>>>>>> [email protected] >>>>>>> >>>>>>>>>>>>>> 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
