Gerrymander tips & tricks – viewing comments on a gerrit review without bots

Posted: May 21st, 2014 | Author: | Filed under: Coding Tips, Fedora, OpenStack, Virt Tools | Tags: , , , | 1 Comment »

Those who saw my original announcement of gerrymander will know that OpenStack uses the Gerrit tool for code review. One of the efforts made by the Nova (Compute) team during the last release cycle was to increase the automated testing coverage for hypervisors supported by Nova. Previously we only tested Libvirt on KVM, but now there is testing for VMWare, HyperV and XenAPI too. This is great for our code quality, but far less great for the usability of the gerrit web interface. Consider a fairly typical change I needed to look at today, the web UI for gerrit shows many comments. When doing code review of new patch versions though, I mostly don’t care about comments from the “robot” accounts doing automated testing – just the pass / fail status. I’m far more interested in comments made by humans, so I can see whether they’ve been addressed by new patch versions, or whether I agree/disagree with the comments. The gerrit web UI though doesn’t provide any way to separate this info – it just shows a huge list with human & robot comments combined. The ever growing number of robots used by OpenStack for testing mean the human comments are drowned out in the noise

Gerrit comments

This problem is one of the reasons I created the gerrymander tool. The ‘comments’ command has the ability to display all comments on a change, while filtering out those made by known bot accounts. Now there’s a little setup required, by which I mean you must edit $HOME/.gerrymander to list all the bot accounts. For those using OpenStack though, I’ve uploaded a sample config file, which lists all bot accounts, to the wiki. With this config file in effect I can now look at this same change number 87329

# gerrymander comments --color 87329
Change https://review.openstack.org/87329 (I06c2d9930e0f36a0d7057b6a0f5c9c591caac43f)

  libvirt: Use os_command_line when kernel_id is set


Patch 1 (c8d1cf4559174777be4b42b68379cf78ccd8f382)

  Jay Pipes: (jaypipes) nova/virt/libvirt/driver.py:3203

  You can condense lines 3202-3203 to:   guest.os_cmdline =
  img_props.get('os_command_line')


  Jay Pipes: (jaypipes)

  Patch Set 1: I would prefer that you didn't merge this  (1 inline comment)
  Suggestion for consolidation inline, otherwise, looks good.


  Vladan Popovic: (vladan)

  Patch Set 1:  Thanks for the review Jay.


Patch 2 (58323a7271e0f09689b94bc8632204c179934add)

  Daniel Berrange: (berrange) /COMMIT_MSG:18

  "AMI images" is a pretty obscure/non-obvious term.  This is about images with
  an explicit boot kernel set, so please say that explicitly and void the term
  "AMI"


  Daniel Berrange: (berrange)

  Patch Set 2: I would prefer that you didn't merge this  (1 inline comment)


  Jay Pipes: (jaypipes)

  Patch Set 2:  recheck bug 1307344

Patch 3 (8f3ced280d80eaee69029f4bf2a193bcba749284)

  garyk: (garyk) nova/tests/virt/libvirt/test_libvirt.py:1961

  please use self.assertIsNone


  Mohammed Naser: (mnaser)

  Patch Set 3: Looks good to me, but someone else must approve


  garyk: (garyk)

  Patch Set 3: I would prefer that you didn't merge this  (1 inline comment)
  code looks good. one minor nit


Patch 4 (5698a6c7c5195bd1d3ecf01af6fb65c19ae8a990)

  No  comments


Patch 5 (a87773776a0d39aa5591fde6caa65b92f5b17a6d)

  Jay Pipes: (jaypipes)

  Patch Set 5: Looks good to me, but someone else must approve  ++, thx Vladlan!


  Mohammed Naser: (mnaser)

  Patch Set 5: Looks good to me, but someone else must approve  recheck bug
  1307344


  Daniel Berrange: (berrange)

  Patch Set 5: I would prefer that you didn't merge this  Jenkins failures are
  genuine bugs

  Sreeram Yerrapragada: (syerrapragada)

  Patch Set 5:  recheck-vmware


Patch 6 (a1eb12f0c8281c0b01dde00b8225d742b8e832e3)

  garyk: (garyk)

  Patch Set 6: Code-Review+1

The obvious limitation here is that the file comments are not shown inline with the code, but overall I think this is still much more useful than the gerrit web UI display of comments. I can still go look at the web UI if I do need to see the context of certain inline comments. So gerrymander comments report complements the web UI nicely.