find-owners plugin for Gerrit Code Review

544 views
Skip to first unread message

Chih-hung Hsieh

unread,
Jan 30, 2017, 10:32:10 PM1/30/17
to Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Stephen Hines, Andrii Shyshkalov
Hi Gerrit Community,

I am working on a new plugin for Android and Chromium developers
to support OWNERS files. See design doc at go/android-owners.

Can someone create a new repository for me on Gerrit-Review?

Name: plugins/find-owners
Description: Plugin to check for OWNERS approval before submit and find owners for a revision.

Thank you in advance.

Chih-Hung

David Pursehouse

unread,
Jan 30, 2017, 10:35:20 PM1/30/17
to Chih-hung Hsieh, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Stephen Hines, Andrii Shyshkalov
On Tue, Jan 31, 2017 at 12:32 PM 'Chih-hung Hsieh' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:
Hi Gerrit Community,

I am working on a new plugin for Android and Chromium developers
to support OWNERS files. See design doc at go/android-owners.

Can someone create a new repository for me on Gerrit-Review?


Would it make sense to add this functionality in the existing reviewers plugin which, as far as I understand, is already being used by Google?



 
Name: plugins/find-owners
Description: Plugin to check for OWNERS approval before submit and find owners for a revision.

Thank you in advance.

Chih-Hung

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

David Pursehouse

unread,
Jan 30, 2017, 11:48:44 PM1/30/17
to Stephen Hines, Chih-hung Hsieh, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
On Tue, Jan 31, 2017 at 12:46 PM Stephen Hines <srh...@google.com> wrote:
I think that the existing reviewers plugin does not use a dedicated OWNERS file.

That's why I suggested to add it :)
 
find-owners is intended to cover more than just locating adequate reviewers, and instead be used to verify who can approve/commit changes for parts of a codebase.


There's also the existing owners plugin, which may be a more appropriate place?


 
Steve

Chih-hung Hsieh

unread,
Jan 31, 2017, 12:33:26 AM1/31/17
to David Pursehouse, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
In go/android-owners I included evaluations of the following plugins:

The Chromium's OWNERS file syntax and rules are more than 90% compatible with Android's need. The other plugins have not used OWNERS file yet, or use a different syntax, and also very different rules to pick reviewers.

Here are the summaries of my comparisons in go/android-owners:
  • The "owners" plugin .... uses YAML syntax in OWNERS files. We need to change its OWNERS file parser to recognize our syntax. It does not support per-file or file pattern owners, only per directory.
  • The "owners" plugin package includes an "owners-autoassign" plugin, which adds ALL owners in all related OWNERS files into the reviewers list automatically. We need two improvements:
    • A smarter algorithm to select only a subset of owners, like the proposals in go/cl_approval_plus2_smart or the priority and limits in Chromium's depot_tools.
    • Give the owners as suggestions, but do not add all of them into the reviewers list. In most cases, an active developer can pick reviewers easily.
  • The "reviewers" and "reviewers-by-blame" plugins select reviewers without consulting OWNERS files. ... the manually selected people in OWNERS files are usually better candidates ...

I only see a small portion of shareable code. It would be difficult to convince existing "owners" or "reviewers" plugin users to accept a large chunk of new unused code.

So I am proposing one independent plugin to serve both Chromium and Android.
It should be easier to configure multiple such plugins in one Gerrit server for different projects, instead of configure one large plugin.


On Mon, Jan 30, 2017 at 8:48 PM, David Pursehouse <david.pu...@gmail.com> wrote:
On Tue, Jan 31, 2017 at 12:46 PM Stephen Hines <srh...@google.com> wrote:
I think that the existing reviewers plugin does not use a dedicated OWNERS file.

That's why I suggested to add it :)
 
find-owners is intended to cover more than just locating adequate reviewers, and instead be used to verify who can approve/commit changes for parts of a codebase.


There's also the existing owners plugin, which may be a more appropriate place?


 
Steve

On Mon, Jan 30, 2017 at 7:35 PM, David Pursehouse <david.pu...@gmail.com> wrote:
On Tue, Jan 31, 2017 at 12:32 PM 'Chih-hung Hsieh' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:
Hi Gerrit Community,

I am working on a new plugin for Android and Chromium developers
to support OWNERS files. See design doc at go/android-owners.

Can someone create a new repository for me on Gerrit-Review?


Would it make sense to add this functionality in the existing reviewers plugin which, as far as I understand, is already being used by Google?



 
Name: plugins/find-owners
Description: Plugin to check for OWNERS approval before submit and find owners for a revision.

Thank you in advance.

Chih-Hung

--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

David Pursehouse

unread,
Jan 31, 2017, 1:50:00 AM1/31/17
to Chih-hung Hsieh, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
On Tue, Jan 31, 2017 at 2:33 PM Chih-hung Hsieh <c...@google.com> wrote:
In go/android-owners I included evaluations of the following plugins:

This link is not accessible to me.  I assume it's a Google internal thing.

 

The Chromium's OWNERS file syntax and rules are more than 90% compatible with Android's need. The other plugins have not used OWNERS file yet, or use a different syntax, and also very different rules to pick reviewers.

Here are the summaries of my comparisons in go/android-owners:
  • The "owners" plugin .... uses YAML syntax in OWNERS files. We need to change its OWNERS file parser to recognize our syntax. It does not support per-file or file pattern owners, only per directory.
  • The "owners" plugin package includes an "owners-autoassign" plugin, which adds ALL owners in all related OWNERS files into the reviewers list automatically. We need two improvements:
    • A smarter algorithm to select only a subset of owners, like the proposals in go/cl_approval_plus2_smart or the priority and limits in Chromium's depot_tools.
    • Give the owners as suggestions, but do not add all of them into the reviewers list. In most cases, an active developer can pick reviewers easily.
  • The "reviewers" and "reviewers-by-blame" plugins select reviewers without consulting OWNERS files. ... the manually selected people in OWNERS files are usually better candidates ...

I only see a small portion of shareable code. It would be difficult to convince existing "owners" or "reviewers" plugin users to accept a large chunk of new unused code.

So I am proposing one independent plugin to serve both Chromium and Android.
It should be easier to configure multiple such plugins in one Gerrit server for different projects, instead of configure one large plugin.


OK.  I've created the plugin:


The group "plugins-find-owners", which I have added you to, has the usual permissions.

 


On Mon, Jan 30, 2017 at 8:48 PM, David Pursehouse <david.pu...@gmail.com> wrote:
On Tue, Jan 31, 2017 at 12:46 PM Stephen Hines <srh...@google.com> wrote:
I think that the existing reviewers plugin does not use a dedicated OWNERS file.

That's why I suggested to add it :)
 
find-owners is intended to cover more than just locating adequate reviewers, and instead be used to verify who can approve/commit changes for parts of a codebase.


There's also the existing owners plugin, which may be a more appropriate place?


 
Steve

On Mon, Jan 30, 2017 at 7:35 PM, David Pursehouse <david.pu...@gmail.com> wrote:
On Tue, Jan 31, 2017 at 12:32 PM 'Chih-hung Hsieh' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:
Hi Gerrit Community,

I am working on a new plugin for Android and Chromium developers
to support OWNERS files. See design doc at go/android-owners.

Can someone create a new repository for me on Gerrit-Review?


Would it make sense to add this functionality in the existing reviewers plugin which, as far as I understand, is already being used by Google?



 
Name: plugins/find-owners
Description: Plugin to check for OWNERS approval before submit and find owners for a revision.

Thank you in advance.

Chih-Hung

--
--
To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

Luca Milanesio

unread,
Jan 31, 2017, 2:06:45 AM1/31/17
to Chih-hung Hsieh, David Pursehouse, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
On 31 Jan 2017, at 05:33, 'Chih-hung Hsieh' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

In go/android-owners I included evaluations of the following plugins:

The Chromium's OWNERS file syntax and rules are more than 90% compatible with Android's need. The other plugins have not used OWNERS file yet, or use a different syntax, and also very different rules to pick reviewers.

Here are the summaries of my comparisons in go/android-owners:
  • The "owners" plugin .... uses YAML syntax in OWNERS files. We need to change its OWNERS file parser to recognize our syntax.
I don't see a big problem here. Can you share the syntax of your OWNERS file?

  • It does not support per-file or file pattern owners, only per directory.

  • The "owners" plugin package includes an "owners-autoassign" plugin, which adds ALL owners in all related OWNERS files into the reviewers list automatically.
It is optional, you don't have to use it if you don't like it or you require a different mechanism.

  • We need two improvements:
    • A smarter algorithm to select only a subset of owners, like the proposals in go/cl_approval_plus2_smart or the priority and limits in Chromium's depot_tools.
I am not aware of the details of your algorithm, could you provide a patch for it?

    • Give the owners as suggestions, but do not add all of them into the reviewers list. In most cases, an active developer can pick reviewers easily.
That's a very good idea, it would add a lot of value to the current plugin.

  • The "reviewers" and "reviewers-by-blame" plugins select reviewers without consulting OWNERS files. ... the manually selected people in OWNERS files are usually better candidates ...

I only see a small portion of shareable code. It would be difficult to convince existing "owners" or "reviewers" plugin users to accept a large chunk of new unused code.

Why are you saying "unused" ? If you are providing improvements that make sense for your team, they possibly make sense for other people as well.

To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

Sebastian Schuberth

unread,
Jan 31, 2017, 4:04:22 AM1/31/17
to Repo and Gerrit Discussion, david.pu...@gmail.com, srh...@google.com, dbor...@google.com, david...@google.com, step...@google.com, tan...@google.com
On Tuesday, January 31, 2017 at 6:33:26 AM UTC+1, Chih-hung Hsieh wrote:

> Reviewers: https://gerrit.googlesource.com/plugins/reviewers
> Reviewers-by-blame: https://gerrit.googlesource.com/plugins/reviewers-by-blame

Speaking esp. about these two, I've wondering the past whether it wouldn't make sense to combine the functionality of the two into a single plugin.

Any opinions about that?

If there was such a combined plugin, for example named determine-reviewers, I'd also vote for adding the find-owners functionality as an "Determine reviewers from OWNERS file" option to it rather than creating yet another very similar plugin.

Regards,
Sebastian

luca.mi...@gmail.com

unread,
Jan 31, 2017, 4:24:00 AM1/31/17
to Sebastian Schuberth, Repo and Gerrit Discussion, david.pu...@gmail.com, srh...@google.com, dbor...@google.com, david...@google.com, step...@google.com, tan...@google.com
Agreed, if you see other ecosystems like Jenkins, there are way too many similar plugin that do the same thing in a slightly different way ... and of course they aren't compatible between each other :-(

Can we avoid creating yet another plugin for the owners problem?

Sent from my iPhone
--
--
To unsubscribe, email repo-discuss...@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.

Sebastian Schuberth

unread,
Jan 31, 2017, 4:26:38 AM1/31/17
to Luca Milanesio, Repo and Gerrit Discussion, david.pu...@gmail.com, srh...@google.com, David Borowitz, david...@google.com, step...@google.com, tan...@google.com
On Tue, Jan 31, 2017 at 10:23 AM, <luca.mi...@gmail.com> wrote:

> Can we avoid creating yet another plugin for the owners problem?

By being more restrictive when creating new plugin repos at
https://gerrit.googlesource.com/plugins/. There has to be a rationale
given why the proposed functionality cannot be added to an existing
plugin.

Regards,
Sebastian

Luca Milanesio

unread,
Jan 31, 2017, 5:03:46 AM1/31/17
to Sebastian Schuberth, Repo and Gerrit Discussion, david.pu...@gmail.com, srh...@google.com, David Borowitz, david...@google.com, step...@google.com, tan...@google.com
I do not believe that being "restrictive" is the right approach: people would just start creating them on GitHub anyway, as many people did.

There was a proposal in the past of allowing "self-service" creation of projects with a review trail: you can "propose" the create of a project and get the feedback from the community by yourself.
Then the project gets created if people have positive reviews about it :-)

Luca.

Remy Bohmer

unread,
Jan 31, 2017, 10:35:54 AM1/31/17
to Luca Milanesio, srh...@google.com, Repo and Gerrit Discussion, david...@google.com, tan...@google.com, Sebastian Schuberth, step...@google.com, David Borowitz, david.pu...@gmail.com
Hi,

Then there is also the reviewassistant plugin which has overlap with reviewers-by-blame plugin. Actually the assistant superseeds that by blame plugin.

Kind regards,

Remy

Op 31 jan. 2017 11:03 schreef "Luca Milanesio" <luca.mi...@gmail.com>:
--
--
To unsubscribe, email repo-discuss+unsubscribe@googlegroups.com

More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss+unsubscribe@googlegroups.com.

Chih-hung Hsieh

unread,
Jan 31, 2017, 3:19:51 PM1/31/17
to Luca Milanesio, David Pursehouse, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
David, thanks for the help. I apologize for not knowing in advance that my document was accessible only to Googlers.

To Luca and other plugin users, I started looking for a Gerrit solution for Android and Chromium a few months ago. So my evaluation did not include all recent features in those plugins.

Android and Chromium have their own development processes. They both inherit a subset of the OWNERS file concepts from other Google projects. Some of them are quite different from Gerrit process and ACL capabilities. I tried to make a plugin as compatible with the Gerrit system as possible, but true to Android's and Chromium's needs.

I would love to see it reused by other projects. But I am afraid that Android and Chromium will do their own customizations, not necessarily what other Gerrit served projects want now or in the future.

Please take a look of the draft find-owners documents in
The Chromium OWNERS file syntax and semantics are even more complicated in


Luca Milanesio

unread,
Jan 31, 2017, 6:58:21 PM1/31/17
to Chih-hung Hsieh, David Pursehouse, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
Hi Chin-Hung,
have you looked at the approach we've taken for the ITS-* suite?

Basically, there is a base common logic in its-base plugin and then the actual implementation of the mechanism behind what is an "issue tracker" is left to the "sub-plugins".
Something similar may apply to your plugin as well.

What I see as "custom" is the logic of defining who is the owner, which is a custom Python file whilst is a simple YAML for the owners plugin.
However, most of the other logic can be put at common factor.

Luca.

Chih-hung Hsieh

unread,
Feb 1, 2017, 1:28:07 PM2/1/17
to Luca Milanesio, David Pursehouse, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
Luca, I only had a quick look of the its-* plugins,
and I like the idea of sharing functionalities across plugins.

The find-owners plugin will provide a REST API to return all owners
information of a change patch set. Different UI plugins can
display the data and suggest reviewers. I am testing one for
the current GWT UI, and a new one for PolyGerrit (new UI) will
be written later. It would be nice if the "owners" plugin also
provide a simple API.

Similarly, there are many plugins that want to add/remove submit labels.
Hence, it is important to have simple Prolog rules like
find_owners:submit_filter(In,Out),
find_owners:remove_may_label(In,Out), and
find_owners:remove_need_label(In,Out).

The parsing, caching, and error handling of OWNERS files indeed
are the major components. The UI JavaScript and Prolog rules
are relatively smaller. It would be nice to share some of
the JavaScripts and Prolog rules, but I don't know how to
do that yet, especially with the new PolyGerrit UI.

Luca Milanesio

unread,
Feb 1, 2017, 5:24:54 PM2/1/17
to Chih-hung Hsieh, David Pursehouse, Stephen Hines, Repo and Gerrit Discussion, Dave Borowitz, David James, Stephen Li, Andrii Shyshkalov
On 1 Feb 2017, at 18:28, Chih-hung Hsieh <c...@google.com> wrote:

Luca, I only had a quick look of the its-* plugins,
and I like the idea of sharing functionalities across plugins.

Yes, me too :-)

We just need to (ehm ehm) build into Gerrit the plugins dependency mechanism: at the moment the functionality is shared only at build time, which could be good from points of view, bad for others.


The find-owners plugin will provide a REST API to return all owners
information of a change patch set. Different UI plugins can
display the data and suggest reviewers. I am testing one for
the current GWT UI, and a new one for PolyGerrit (new UI) will
be written later. It would be nice if the "owners" plugin also
provide a simple API.

Like it a lot :-)
At the moment we are constrained by the output of a Prolog rule to display "something" on the change screen: having a proper REST API would allow a richer and nicer UX.



Similarly, there are many plugins that want to add/remove submit labels.
Hence, it is important to have simple Prolog rules like
find_owners:submit_filter(In,Out),
find_owners:remove_may_label(In,Out), and
find_owners:remove_need_label(In,Out).

The parsing, caching, and error handling of OWNERS files indeed
are the major components.

Let me dig into your OWNERS format and we can come up with ideas.

The UI JavaScript and Prolog rules
are relatively smaller. It would be nice to share some of
the JavaScripts and Prolog rules, but I don't know how to
do that yet, especially with the new PolyGerrit UI.

Is there any PolyGerrit UX person to help out there?
Reply all
Reply to author
Forward
0 new messages