Convert Review Bot to use an integration and status updates.
Review Request #8442 — Created Sept. 26, 2016 and submitted
This change is a massive refactor to the interface between Review Board and
Review Bot. It involves two major pieces:Integration: Review Bot configuration now uses the new integration framework.
When the Review Bot extension is enabled, it offers a configuration page that
only has the celery broker URL and the user who Review Bot should operate as.
Other configuration is then done by creating "integration configurations". Each
configuration allows selecting a tool to run, conditions for when that tool
should be run (which can be as simple as "Always" or as complicated as rules
for specific branches or authors. The tool-specific options are then merged
into this form, rather than just giving people an opaque JSON blob to edit.Status Updates: Instead of creating regular reviews, Review Bot will now create
a review and associate it with a status update. Status updates are a new
feature that will batch the items together and give the whole thing a summary
status (such as "done-success" or "pending"). This eliminates the need to post
a review that shows that Review Bot ran but ignored all the files, since we now
have another feedback mechanism to show that Review Bot isn't wedged. It also
means we don't need to have any kind of coordination between the tools to batch
things together.As part of this, I've removed a bunch of code. A lot of this code was stuff
that only existed on the master branch and was never really used in production.
This includes:
- Review Bot can no longer give a "Ship It!" This is partly because status
update reviews don't show the ship-it state, and partly because we don't
think anyone was using it (having your automated tools tell people to ship
their code is sketchy). - Reviews no longer have the default header which shows what files were
processed or ignored. - The ToolExecution model has been completely removed, along with APIs. This
was never really used that much, and is supplanted by the status update
state. - The Profile and AutomaticRunGroup models have been removed, since their
functionality is now in the integration configurations. - The ManualPermission model has been removed, since it was just an unfinished
feature. We'll be figuring out manual execution of tools later. - Removed a bunch of other dead code.
- Enabled the extension and created some integration configurations.
- Verified that the configurations executed correctly based on their configured
conditions and settings. - Published a bunch of diffs on different review requests and saw the Review
Bot workers get notified, run their tools, and report their results. - Verified the appearance of Review Bot reviews within the new status updates.
| Description | From | Last Updated |
|---|---|---|
|
Can you add screenshots of the extension and integration configs, and status output? |
 chipx86
|
|
|
While here, it'd be nice to also include the review request/changedesc ID and diff revision (this would have been helpful … |
chipx86
|
|
|
Do we want to use logging.exception here instead? |
 brennie
|
|
|
Can this show all matching items, to help see where the problem is? |
chipx86
|
|
|
Can this also show the review request ID, changedesc, etc.? Helps when there's a bunch of entries in the log … |
chipx86
|
|
|
Do we want to have this lowercase, or sentence case? Same below. It might also be nice to define these … |
chipx86
|
|
|
This should also include identifying information. Same with other exceptions below. |
chipx86
|
|
|
Should have a blank line here after the docstring. |
chipx86
|
|
|
This is going to cause the stylesheet to show up for all integration configuration pages. This is going to be … |
chipx86
|
|
|
Can we use integration-config.less instead, to better match the other stylesheet filenames in the products? |
chipx86
|
|
|
You can just do: extension = ReviewBotExtension.instance |
chipx86
|
|
|
We should catch exceptions here and raise a ValidationError. |
chipx86
|
|
|
Docstring. |
brennie
|
|
|
No blank line here. |
chipx86
|
|
|
Move this to the top of the method. |
brennie
|
|
|
This should use build_server_url with a local_site_reverse. |
chipx86
|
|
|
These defaults are specified in a couple different places. Let's put constants somewhere and use those. |
chipx86
|
|
|
Let's catch exceptions here, in case there's a tools_options with bad data. Actually, every configuration being processed should be wrapped … |
chipx86
|
|
|
Missing a trailing period. |
chipx86
|
|
|
Can we localize this while we're here? |
chipx86
|
|
|
(dict, unused) |
chipx86
|
|
|
(dict, optional) |
chipx86
|
|
|
Can we use keyword arguments for this, to help with readability? It's a bit more mental work right now to … |
chipx86
|
|
|
Let's pull the field out once. |
chipx86
|
|
|
Let's pull out the field once. |
chipx86
|
-
-
-
While here, it'd be nice to also include the review request/changedesc ID and diff revision (this would have been helpful when I was trying to diagnose failures a little while back).
-
-
Can this also show the review request ID, changedesc, etc.? Helps when there's a bunch of entries in the log and you're trying to grep/sort through them.
Also, not sure we need to capitalize "status update" (here and below).
-
Do we want to have this lowercase, or sentence case?
Same below.
It might also be nice to define these as constants somewhere, so it's easier to read through them all (and as a reference for other implementations).
-
-
-
This is going to cause the stylesheet to show up for all integration configuration pages. This is going to be more of a problem with the JavaScript file defined below (since it can easily mess with logic on other integration pages using the same field names).
I was thinking we could use Django's form media support, but that's not actually compatible with bundles. So, I think we should update the base configuration form to allow for specifying a list of CSS and JS bundle names (as actual lists or methods that return lists, for dynamically computing based on an extension instance), and then loop through those in
djblets/forms/templates/djblets_forms/admin/change_form_page.html. (That would actually allow any form to define these, which would be nice, but we'd explicitly want to support this for integration config forms).Then you could just list those directly on the form and get them only for that form.
-
Can we use
integration-config.lessinstead, to better match the other stylesheet filenames in the products? -
-
-
-
These defaults are specified in a couple different places. Let's put constants somewhere and use those.
-
Let's catch exceptions here, in case there's a
tools_optionswith bad data.Actually, every configuration being processed should be wrapped in a
try/exceptso other configs aren't impacted. -
-
-
-
Can we use keyword arguments for this, to help with readability? It's a bit more mental work right now to match things up.
-
-
It's a bit sketchy, but it should be safe... Data's computed up-front before anything calls into
value_from_datadict.Maybe we should catch/reraise this with something like "We did this to ourselves! What have we done..."
-
- Commit:
-
8cb506c082a00950dfca3e02f1a30c12837ec900be8cbcfc26775d55255988596302ae0933c50181
- Diff:
-
Revision 2 (+625 -1600)
- Added Files: