Automated Static Analysis on Review Request - reviewboard-dev

The old Google Groups will be going away soon, but your browser is incompatible with the new version.
Message from discussion Automated Static Analysis on Review Request
Received-SPF: neutral (google.com: 66.111.4.25 is neither permitted nor denied by domain of j...@pobox.com) client-ip=66.111.4.25;
Date: Wed, 1 Feb 2012 23:38:52 +0000 (GMT)
From: John J Lee <j...@pobox.com>
To: reviewboard-dev@googlegroups.com
Subject: Re: Automated Static Analysis on Review Request
In-Reply-To: <CA+A4QR36trg25dPBZVs1pTHrVW3R2SM=+K2_4ikX-+0O+i_...@mail.gmail.com>
Message-ID: <alpine.DEB.2.00.1202012114090.2...@localhost6.localdomain6>
References: <CA+A4QR36trg25dPBZVs1pTHrVW3R2SM=+K2_4ikX-+0O+i_...@mail.gmail.com>
User-Agent: Alpine 2.00 (DEB 1167 2008-08-23)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; format=flowed; charset=US-ASCII

On Mon, 30 Jan 2012, Steven MacLeod wrote:

> I'm currently writing a Review Board extension (ReviewBot) which aims to
> automate execution of static analysis tools on proposed code.

Great.


(RB below == ReviewBoard, not ReviewBot!)

>    ** Ship-it if no problems

As I'm sure you know, review workflows differ -- for example, some 
workflows allow commit when there's at least one ship-it.  In that 
workflow you probably prefer that the bot doesn't give a ship-it 
(otherwise, it's hard to prioritize your effort as a reviewer because you 
can't see quickly which reviews have a ship-it already and which don't). 
It would be nice if ReviewBot were to be as flexible as RB itself about 
that kind of variation.

While an arbitrary hook that can perform any action is completely 
flexible, no matter how simple it is to write a plugin, there's always 
some annoying work to implement the hook and adapt it to your PEP8 tool 
(or whatever it is the plugin does).  Nothing beats just installing 
somebody else's plugin and having it just work.

So, another way to preserve that flexibility, which I think is better 
because it's more convenient for the various kinds of RB user involved, 
would be for plugins to stay out of doing concrete actions like adding 
concrete RB comments, adding issues, adding a ship-it, etc.  This is in 
contrast to your web-hook idea as you first stated it, where the hook gets 
to make whatever RB API calls it likes.  Having the plugins not be 
responsible for those actions does also have the advantage that it would 
decouple the plugins from RB.  In principle, that would mean users of 
other review tools (notably gerrit) could contribute plugins that would 
work on RB (as well as on gerrit) -- your web hook idea fits well with 
that hope.

This makes life easy for people deploying RB, because they can just 
install the PEP8 plugin somebody else wrote, and start off by leaving 
ReviewBot set to its default behaviour for what actions to perform. 
Maybe they then have five minutes later to change some ReviewBot 
configuration to set a few flags like "Don't give a ship-it".

It makes life easy for plugin authors because they get an API to code to 
that's much simpler than the full RB API (see below).  And maybe a gerrit 
user already wrote the plugin they want, so they have nothing to do (maybe 
wishful thinking, but seems there's scope for a little bit of cooperation 
here).

Probably 90% of workflows could be dealt with using built-in ReviewBot 
code plus a couple of configuration options (add a ship it or not, add 
issues or not), but there's always the option of adding another hook in 
future to customize the actions arbitrarily.


> Mike Conley and I discussed the possibilities of a WebHooks extension to
> solve this problem. When a review is published, a web request could be
> fired off to ReviewBot on another server, which could run each tool, and
> then use the API to post the reviews.

Sounds easy (to use).  What arguments would the hook take?

Per my points above re flexibility to workflow and ease of use: how about, 
rather than suggesting that plugins use the full RB API, just exposing a 
very simple ReviewBot API to the plugins, which allows only for submitting 
a set of abstract comments.  By "abstract", I just mean not actual RB 
comments, but rather along the lines of (suitably REST-encoded) tuples 
(file path, start line number, end line number, severity).  ReviewBot 
itself is then responsible for turning those into concrete RB comments, 
issues, ship-its, etc.

(Side issue: With the severity field there I'm thinking it might be useful 
to allow for indicating potential problems where the tool can't be sure 
that the problem is real.)

Of course there's nothing here to stop the plugins doing RB API calls: I'm 
just suggesting that providing a simpler alternative is easier for both 
plugin authors and RB deployers.


> Another option Christian Hammond mentioned to me was using distributed task
> queues (See python Celery: http://celeryproject.org/). This is something
> being looked in to already for distributing the diff processing, and could
> also be applied to ReviewBot.

Perhaps that solves some problems for reviewboard (fair enough).  It 
doesn't sound like it solves a problem for ReviewBot plugin authors or 
people deploying RB?


Oops, that was much longer than I intended, sorry!


John
Create a group - Google Groups - Google Home - Terms of Service - Privacy Policy
Account Options
