Summary

Convert Review Bot to use an integration and status updates.

Review Request #8442 — Created Sept. 26, 2016 and submitted Oct. 26, 2016, 3:57 p.m.

Information

Owner

david

Repository

ReviewBot

Branch

master

Bugs

Depends On

Commit

1ce3133...

Reviewers

Groups

reviewbot

People

Description

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.

Testing Done


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.

Files

Issues

Description	From	Last Updated
Can you add screenshots of the extension and integration configs, and status output?	chipx86	Oct. 12, 2016, 4:16 p.m.
While here, it'd be nice to also include the review request/changedesc ID and diff revision (this would have been helpful …	chipx86	Oct. 12, 2016, 10:47 p.m.
Do we want to use logging.exception here instead?	brennie	Oct. 12, 2016, 10:33 p.m.
Can this show all matching items, to help see where the problem is?	chipx86	Oct. 12, 2016, 10:47 p.m.
Can this also show the review request ID, changedesc, etc.? Helps when there's a bunch of entries in the log …	chipx86	Oct. 13, 2016, 11:08 a.m.
Do we want to have this lowercase, or sentence case? Same below. It might also be nice to define these …	chipx86	Oct. 13, 2016, 11:09 a.m.
This should also include identifying information. Same with other exceptions below.	chipx86	Oct. 13, 2016, 11:10 a.m.
Should have a blank line here after the docstring.	chipx86	Oct. 13, 2016, 11:11 a.m.
This is going to cause the stylesheet to show up for all integration configuration pages. This is going to be …	chipx86	Oct. 13, 2016, 1:51 p.m.
Can we use integration-config.less instead, to better match the other stylesheet filenames in the products?	chipx86	Oct. 13, 2016, 11:12 a.m.
You can just do: extension = ReviewBotExtension.instance	chipx86	Oct. 25, 2016, 3:23 p.m.
We should catch exceptions here and raise a ValidationError.	chipx86	Oct. 13, 2016, 11:42 a.m.
Docstring.	brennie	Oct. 12, 2016, 10:34 p.m.
No blank line here.	chipx86	Oct. 13, 2016, 11:43 a.m.
Move this to the top of the method.	brennie	Oct. 12, 2016, 10:34 p.m.
This should use build_server_url with a local_site_reverse.	chipx86	Oct. 13, 2016, 11:45 a.m.
These defaults are specified in a couple different places. Let's put constants somewhere and use those.	chipx86	Oct. 13, 2016, 11:48 a.m.
Let's catch exceptions here, in case there's a tools_options with bad data. Actually, every configuration being processed should be wrapped …	chipx86	Oct. 13, 2016, 11:53 a.m.
Missing a trailing period.	chipx86	Oct. 25, 2016, 11:10 a.m.
Can we localize this while we're here?	chipx86	Oct. 13, 2016, 11:54 a.m.
(dict, unused)	chipx86	Oct. 13, 2016, 11:54 a.m.
(dict, optional)	chipx86	Oct. 13, 2016, 11:54 a.m.
Can we use keyword arguments for this, to help with readability? It's a bit more mental work right now to …	chipx86	Oct. 13, 2016, 11:56 a.m.
Let's pull the field out once.	chipx86	Oct. 13, 2016, 11:56 a.m.
Let's pull out the field once.	chipx86	Oct. 13, 2016, 11:57 a.m.

Ship it!

bot/reviewbot/tasks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Do we want to use logging.exception here instead?
```
extension/reviewbotext/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Docstring.
```
extension/reviewbotext/integration.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Move this to the top of the method.
```
1. david Oct. 13, 2016, 1:51 p.m.
  I'd rather keep it next to where it's used.

The issue has been resolved. Show all issues

Can you add screenshots of the extension and integration configs, and status output?

david Oct. 13, 2016, 1:52 p.m.

I'm going to skip the extension config because I'm currently completely redoing that. For the status output, see https://reviews.reviewboard.org/r/8447/file/1319/

bot/reviewbot/tasks.py (Diff revision 1)

The issue has been resolved. Show all issues

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).

bot/reviewbot/tasks.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can this show all matching items, to help see where the problem is?
```

bot/reviewbot/tasks.py (Diff revision 1)

The issue has been resolved. Show all issues

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).

bot/reviewbot/tasks.py (Diff revision 1)

The issue has been dropped. Show all issues

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).

david Oct. 13, 2016, 1:52 p.m.

These should be lower case due to the way that they're shown in the status UI. I'm going to hold off on making them constants because I do plan to refactor this a bit later.

bot/reviewbot/tasks.py (Diff revision 1)

The issue has been resolved. Show all issues

This should also include identifying information.

Same with other exceptions below.

extension/reviewbotext/admin.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should have a blank line here after the docstring.
```

extension/reviewbotext/extension.py (Diff revision 1)

The issue has been resolved. Show all issues

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.

extension/reviewbotext/extension.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we use integration-config.less instead, to better match the other stylesheet filenames in the products?

extension/reviewbotext/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
We should catch exceptions here and raise a ValidationError.
```
extension/reviewbotext/integration.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line here.
```

extension/reviewbotext/integration.py (Diff revision 1)

The issue has been resolved. Show all issues

This should use build_server_url with a local_site_reverse.

david Oct. 13, 2016, 1:52 p.m.

Apparently get_server_url already does what I want in both cases.

extension/reviewbotext/integration.py (Diff revision 1)

The issue has been resolved. Show all issues

These defaults are specified in a couple different places. Let's put constants somewhere and use those.

extension/reviewbotext/integration.py (Diff revision 1)

The issue has been resolved. Show all issues

Let's catch exceptions here, in case there's a tools_options with bad data.

Actually, every configuration being processed should be wrapped in a try/except so other configs aren't impacted.

extension/reviewbotext/templates/admin/reviewbotext/tool/change_list.html (Diff revision 1)
The issue has been dropped. Show all issues
```
Can we localize this while we're here?
```
1. david Oct. 13, 2016, 1:52 p.m.
  This is all changing a lot so I'm going to hold off on this.
extension/reviewbotext/widgets.py (Diff revision 1)
The issue has been resolved. Show all issues
```
(dict, unused)
```
extension/reviewbotext/widgets.py (Diff revision 1)
The issue has been resolved. Show all issues
```
(dict, optional)
```

extension/reviewbotext/widgets.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we use keyword arguments for this, to help with readability? It's a bit more mental work right now to match things up.

extension/reviewbotext/widgets.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's pull the field out once.
```

extension/reviewbotext/widgets.py (Diff revision 1)

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..."

extension/reviewbotext/widgets.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Let's pull out the field once.
```

Commit:

-	8cb506c082a00950dfca3e02f1a30c12837ec900
+	be8cbcfc26775d55255988596302ae0933c50181

Diff:

Revision 2 (+625 -1600)

Show changes

	bot/reviewbot/tasks.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/tools/__init__.py
	bot/reviewbot/tools/cpplint.py
	bot/reviewbot/tools/pep8.py
	extension/setup.py
	extension/reviewbotext/admin.py
	extension/reviewbotext/extension.py
	14 more

Added Files:

extension/reviewbotext/forms.py (Diff revisions 1 - 2)

The issue has been resolved. Show all issues

You can just do:

extension = ReviewBotExtension.instance

extension/reviewbotext/integration.py (Diff revisions 1 - 2)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```

Commit:

-	be8cbcfc26775d55255988596302ae0933c50181
+	1ce3133758b6b109b208aabee1fc9222e86ff3a6

Diff:

Revision 3 (+623 -1605)

Show changes

	bot/reviewbot/tasks.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/tools/__init__.py
	bot/reviewbot/tools/cpplint.py
	bot/reviewbot/tools/pep8.py
	extension/setup.py
	extension/reviewbotext/admin.py
	extension/reviewbotext/extension.py
	14 more

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (2727dd0)