945577 - Add a mach rbt command to run ReviewBoard Tools

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Mike Conley (:mconley) (:⚙️)]

With our fancy new Review Board instance up at https://reviewboard.allizom.org/dashboard/, it'd be really excellent if we could grease the wheels and get people to try it.

One idea that's been brewing is to make it easy for developers to upload patches to Review Board via RBTools, and then to simultaneously post a link to the View Diff portion of the RB review request into the bug that the patch belongs to.

The end-result is that a reviewer need only click on an attachment in Bugzilla, and then be quickly redirected to the review request where they can immediately start reviewing.

I think a mach command making use of the RBTools WebAPI client might be a nice solution.

Comment 1

[Gregory Szorc [:gps]]

I was thinking about this as well.

I think it should be sufficient to expose rbt directly to the end-user: no need to write a one-off mach command that simply does what `rbt post` does already.

I could go into a deep discussion about virtualenvs and shell development environments and how we should do this properly. But I won't. I think a simple `mach rbt` that installs RBTools from pip and then invokes its main() should be sufficient for now.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

So does `rbt post` upload the link + review request to bmo?

Comment 3

[Gregory Szorc [:gps]]

Yeah, I missed the 2nd half of the summary. Perhaps this should be marked as a dupe of bug 774150. That's still softly blocked on having a sane Python API to Bugzilla IIRC. A quick hack would be to extract bzexport's bugzilla code into a shared library (or just copy the code). We can install pip packages via mach commands - so shared library is preferred.

Comment 4

[Mark Côté [:mcote]]

There's a fairly straightforward REST API for adding attachments; is this not sufficient?

http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/WebService/Bug.html#add_attachment

Comment 5

[(not currently active) Ted Mielczarek]

bzexport uses BzAPI, which is also fairly straightforward, but it turns out there's a lot of things to get right in order to make it useful.

Comment 6

[Gregory Szorc [:gps]]

Attachment: mach rbt command to run ReviewBoard Tools

While this patch doesn't accomplish what the bug summary requests, it
does solve a problem: making RBTools available inside the development
environment without requiring the user to do much.

History has shown that asking users to install Python packages is prone
to error and has issues interacting with m-c's virtualenv. This command
does things in the least risky way possible. It should be a sufficient
holdover until a dedicated patch uploading command is implemented in
mach. That can be bug 774150.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8343544 [details] [diff] [review]
mach rbt command to run ReviewBoard Tools

Review of attachment 8343544 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good for an interim first step. Are there any other patch related commands with rbt that might make sense as mach commands (I'm not familiar with the tool). Might be worth creating a new "Patch" category in mach for.

Comment 8

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a4ad5ae7eb7

There's problem room for a "developer productivity" category. Maybe the next command breaks the camel's back.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I think adding rbt to the development environment is a good first step.

I've got a patch up against RBTools[1] to make it easy for third-parties to register new rbt commands, so perhaps what we can do is (eventually) augment gps's work here to include another package that adds a bunch of Mozilla-specific commands to rbt (for example, mozpost, which would wrap the built-in post command, but could also attach the link to the review request to a Bugzilla bug).

Or we could add such commands to mach. I don't really feel strongly either way.

[1]: https://reviews.reviewboard.org/r/5046/

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a4ad5ae7eb7

Comment 11

[Gregory Szorc [:gps]]

Pushed a followup fix to argument parsing. I should have tested this patch more. Change was trivial, so no review needed IMO.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c5a87384e5b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c5a87384e5b

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(Since this was morphed to something else entirely.)