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
