mr mr cote

Educational, Investigative, and Absurd Writings by M. R. Côté

Conduit's Commit Index

| Comments

As with MozReview, Conduit is being designed to operate on changesets. Since the end result of work on a codebase is a changeset, it makes sense to start the process with one, so all the necessary metadata (author, message, repository, etc.) are provided from the beginning. You can always get a plain diff from a changeset, but you can’t get a changeset from a plain diff.

Similarly, we’re keeping the concept of a logical series of changesets. This encourages splitting up a unit of work into incremental changes, which are easier to review and to test than large patches that do many things at the same time. For more on the benefits of working with small changesets, a few random articles are Ship Small Diffs, Micro Commits, and Large Diffs Are Hurting Your Ability To Ship.

In MozReview, we used the term commit series to refer to a set of one or more changesets that build up to a solution. This term is a bit confusing, since the series itself can have multiple revisions, so you end up with a series of revisions of a series of changesets. For Conduit, we decided to use the term topic instead of commit series, since the commits in a single series are generally related in some way. We’re using the term iteration to refer to each update of a topic. Hence, a solution ends up being one or more iterations on a particular topic. Note that the number of changesets can vary from iteration to iteration in a single topic, if the author decides to either further split up work or to coalesce changesets that are tightly related. Also note that naming is hard, and we’re not completely satisfied with “topic” and “iteration”, so we may change the terminology if we come up with anything better.

As I noted in my last post, we’re working on the push-to-review part of Conduit, the entrance to what we sometimes call the commit pipeline. However, technically “push-to-review” isn’t accurate, as the first process after pushing might be sending changesets to Try for testing, or static analysis to get quick automated feedback on formatting, syntax, or other problems that don’t require a human to look at the code. So instead of review repository, which we’ve used in MozReview, we’re calling it a staging repository in the Conduit world.

Along with the staging repository is the first service we’re building, the commit index. This service holds the metadata that binds changesets in the staging repo to iterations of topics. Eventually, it will also hold information about how changesets moved through the pipeline: where and when they were landed, if and when they were backed out, and when they were uplifted into release branches.

Unfortunately a simple “push” command, whether from Mercurial or from Git, does not provide enough information to update the commit index. The main problem is that not all of the changesets the author specifies for pushing may actually be sent. For example, I have three changesets, A, B, and C, and pushed them up previously. I then update C to make C′ and push again. Despite all three being in the “draft” phase (which is how we differentiate work in progress from changes that have landed in the mainline repository), only C′ will actually be sent to the staging repo, since A and B already exist there.

Thus, we need a Mercurial or Git client extension, or a separate command-line tool, to tell the commit index exactly what changesets are part of the iteration we’re pushing up—in this example, A, B, and C′. When it receives this information, the commit index creates a new topic, if necessary, and a new iteration in that topic, and records the data in a data store. This data will then be used by the review service, to post review requests and provide information on reviews, and by the autoland service, to determine which changesets to transplant.

The biggest open question is how to associate a push with an existing topic. For example, locally I might be working on two bugs at the same time, using two different heads, which map to two different topics. When I make some local changes and push one head up, how does the commit index know which topic to update? Mercurial bookmarks, which are roughly equivalent to Git branch names, are a possibility, but as they are arbitrarily named by the author, collisions are too great a possibility. We need to be sure that each topic is unique.

Another straightforward solution is to use the bug ID, since the vast majority of commits to mozilla-central are associated with a bug in BMO. However, that would restrict Conduit to one topic per bug, requiring new bugs for all follow-up work or work in parallel by multiple developers. In MozReview, we partially worked around this by using an “ircnick” parameter and including that in the commit-series identifiers, and by allowing arbitrary identifiers via the --reviewid option to “hg push”. However this is unintuitive, and it still requires each topic to be associated with a single bug, whereas we would like the flexibility to associate multiple bugs with a single topic. Although we’re still weighing options, likely an intuitive and flexible solution will involve some combination of commit-message annotations and/or inferences, command-line options, and interactive prompts.

Conduit Field Report, March 2017

| Comments

For background on Conduit, please see the previous post and the Intent to Implement.

Autoland

We kicked off Conduit work in January starting with the new Autoland service. Right now, much of the Autoland functionality is located in the MozReview Review Board extension: the permissions model, the rewriting of commit messages to reflect the reviewers, and the user interface. The only part that is currently logically separate is the “transplant service”, which actually takes commits from one repo (e.g. reviewboard-hg) and applies it to another (e.g. try, mozilla-central). Since the goal of Conduit is to decouple all the automation from code-review tools, we have to take everything that’s currently in Review Board and move it to new, separate services.

The original plan was to switch Review Board over to the new Autoland service when it was ready, stripping out all the old code from the MozReview extension. This would mean little change for MozReview users (basically just a new, separate UI), but would get people using the new service right away. After Autoland, we’d work on the push-to-review side, hooking that up to Review Board, and then extend both systems to interface with BMO. This strategy of incrementally replacing pieces of MozReview seemed like the best way to find bugs as we went along, rather than a massive switchover all at once.

However, progress was a bit slower than we anticipated, largely due to the fact that so many things were new about this project (see below). We want Autoland to be fully hooked up to BMO by the end of June, and integrating the new system with both Review Board and BMO as we went along seemed increasingly like a bad idea. Instead, we decided to put BMO integration first, and then follow with Review Board later (if indeed we still want to use Review Board as our rich-code-review solution).

This presented us with a problem: if we wouldn’t be hooking the new Autoland service up to Review Board, then we’d have to wait until the push service was also finished before we hooked them both up to BMO. Not wanting to turn everything on at once, we pondered how we could still launch new services as they were completed.

Moving to the other side of the pipeline

The answer is to table our work on Autoland for now and switch to the push service, which is the entrance to the commit pipeline. Building this piece first means that users will be able to push commits to BMO for review. Even though they would not be able to Autoland them right away, we could get feedback and make the service as easy to use as possible. Think of it as a replacement for bzexport.

Thanks to our new Scrum process (see also below), this priority adjustment was not very painful. We’ve been shipping Autoland code each week, so, while it doesn’t do much yet, we’re not abandoning any work in progress or leaving patches half finished. Plus, since this new service is also being started from scratch (although involving lots of code reuse from what’s currently in MozReview), we can apply the lessons we learned from the last couple months, so we should be moving pretty quickly.

Newness

As I mentioned above, although the essence of Conduit work right now is decoupling existing functionality from Review Board, it involves a lot of new stuff. Only recently did we realize exactly how much new stuff there was to get used to!

New team members

We welcomed Israel Madueme to our team in January and threw him right into the thick of things. He’s adapted tremendously well and started contributing immediately. Of course a new team member means new team dynamics, but he already feels like one of us.

Just recently, we’ve stolen dkl from the BMO team, where he’s been working since joining Mozilla 6 years ago. I’m excited to have a long-time A-Teamer join the Conduit team.

A new process

At the moment we have five developers working on the new Conduit services. This is more people on a single project than we’re usually able to pull together, so we needed a process to make sure we’re working to our collective potential. Luckily one of us is a certified ScrumMaster. I’ve never actually experienced Scrum-style development before, but we decided to give it a try.

I’ll have a lot more to say about this in the future, as we’re only just hitting our stride now, but it has felt really good to be working with solid organizational principles. We’re spending more time in meetings than usual, but it’s paying off with a new level of focus and productivity.

A new architecture

Working within Review Board was pretty painful, and the MozReview development environment, while amazing in its breadth and coverage, was slow and too heavily focussed on lengthy end-to-end tests. Our new design follows more of a microservice-based approach. The Autoland verification system (which checks users permissions and ensures that commits have been properly reviewed) is a separate service, as is the UI and the transplant service (as noted above, this last part was actually one of the few pieces of MozReview that was already decoupled, so we’re one step ahead there). Similarly, on the other side of the pipeline, the commit index is a separate service, and the review service may eventually be split up as well.

We’re not yet going whole-hog on microservices—we don’t plan, for starters at least, to have more than 4 or 5 separate services—but we’re already benefitting from being able to work on features in parallel and preventing runaway complexity. The book Building Microservices has been instrumental to our new design, as well as pointing out exactly why we had difficulties in our previous approach.

New operations

As the A-Team is now under Laura Thomson, we’re taking advantage of our new, closer relationship to CloudOps to try a new deployment and operations approach. This has freed us of some of the constraints of working in the data centre while letting us take advantage of a proven toolchain and process.

New technologies

We’re using Python 3.5 (and probably 3.6 at some point) for our new services, which I believe is a first for an A-Team project. It’s new for much of the team, but they’ve quickly adapted, and we’re now insulated against the 2020 deadline for Python 2, as well as benefitting from the niceties of Python 3 like better Unicode support.

We also used a few technologies for the Autoland service that are new to most of the team: React and Tornado. While the team found it interesting to learn them, in retrospect using them now was probably a case of premature optimization. Both added complexity that was unnecessary right now. React’s URL routing was difficult to get working in a way that seamlessly supported a local, Docker-based development environment and a production deployment scenario, and Tornado’s asynchronous nature led to extra complexity in automated tests. Although they are both fine technologies and provide scalable solutions for complex apps, the individual Conduit services are currently too small to really benefit.

We’ve learned from this, so we’re going to use Flask as the back end for the push services (commit index and review-request generator), for now at least, and, if we need a UI, we’ll probably use a relatively simple template approach with JavaScript just for enhancements.

Next

In my next post, I’m going to discuss our approach to the push services and more on what we’ve learned from MozReview.

Project Conduit

| Comments

In 2017, Engineering Productivity is starting on a new project that we’re calling “Conduit”, which will improve the automation around submitting, testing, reviewing, and landing commits. In many ways, Conduit is an evolution and course correction of the work on MozReview we’ve done in the last couple years.

Before I get into what Conduit is exactly, I want to first clarify that the MozReview team has not really been working on a review tool per se, aside from some customizations requested by users (line support for inline diff comments). Rather, most of our work was building a whole pipeline of automation related to getting code landed. This is where we’ve had the most success: allowing developers to push commits up to a review tool and to easily land them on try or mozilla-central. Unfortunately, by naming the project “MozReview” we put the emphasis on the review tool (Review Board) instead of the other pieces of automation, which are the parts unique to Firefox’s engineering processes. In fact, the review tool should be a replaceable part of our whole system, which I’ll get to shortly.

We originally selected Review Board as our new review tool for a few reasons:

  • The back end is Python/Django, and our team has a lot of experience working with both.

  • The diff viewer has a number of fancy features, like clearly indicating moved code blocks and indentation changes.

  • A few people at Mozilla had previously contributed to the Review Board project and thus knew its internals fairly well.

However, we’ve since realized that Review Board has some big downsides, at least with regards to Mozilla’s engineering needs:

  • The UI can be confusing, particularly in how it separates the Diff and the Reviews views. The Reviews view in particular has some usability issues.

  • Loading large diffs is slow, but also conversely it’s unable to depaginate, so long diffs are always split across pages. This restricts the ability to search within diffs. Also, it’s impossible to view diffs file by file.

  • Bugs in interdiffs and even occasionally in the diffs themselves.

  • No offline support.

In addition, the direction that the upstream project is taking is not really in line with what our users are looking for in a review tool.

So, we’re taking a step back and evaluating our review-tool requirements, and whether they would be best met with another tool or by a small set of focussed improvements to Review Board. Meanwhile, we need to decouple some pieces of MozReview so that we can accelerate improvements to our productivity services, like Autoland, and ensure that they will be useful no matter what review tool we go with. Project Conduit is all about building a flexible set of services that will let us focus on improving the overall experience of submitting code to Firefox (and some other projects) and unburden us from the restrictions of working within Review Board’s extension system.

In order to prove that our system can be independent of review tool, and to give developers who aren’t happy with Review Board access to Autoland, our first milestone will be hooking the commit repo (the push-to-review feature) and Autoland up to BMO. Developers will be able to push a series of one or more commits to the review repo, and reviewers will be able to choose to review them either in BMO or Review Board. The Autoland UI will be split off into its own service and linked to from both BMO and Review Board.

(There’s one caveat: if there are multiple reviewers, the first one gets to choose, in order to limit complexity. Not ideal, but the problem quickly gets much more difficult if we fork the reviews out to several tools.)

As with MozReview, the push-to-BMO feature won’t support confidential bugs right away, but we have been working on a design to support them. Implementating that will be a priority right after we finish BMO integration.

We have an aggressive plan for Q1, so stay tuned for updates.

BMO in 2016

| Comments

Stuff that landed in 2016

Here’s a sampling of improvements to BMO that were launched in 2016.

Improvements to bug-modal

We’ve continued to refine the modal bug view, aka the “experimental UI”. The BMO team fixed 39 bugs relating to the new interface in 2016. We’ve got a couple more blockers before we make the modal view the default, which should happen in the middle of January. We know there are a still a few outstanding bugs and some missing functionality, so we will leave the standard view available for a little while, at least until all the blockers of bug 1273046 are resolved. All future improvements to the bug view will happen only on the modal UI, which is not just more usable but also more hackable.

HTML email

There was actually no real work involved here, as HTML email was added to BMO years ago. At that time, since it was a new feature, we didn’t enable it by default… and then 4 years went by. Just a couple weeks ago a new BMO user suggested that we implement HTML emails, having no idea that the option was already there (buried in many other preferences). That was the prompting we needed to finally enable it by default.

Readable bug statuses

Emma Humphries added readable statuses prominently displayed at the top of the status panel (in the modal UI only). They quickly summarize the status of a bug in a visible place, mainly for triaging and tracking purposes.

A couple examples:

This is part of Emma’s on-going efforts to improve contributors' experiences.

Time zones

One big change we made this year was hopefully completely invisible to everyone: BMO’s database moved to UTC. When BMO was originally deployed in 1998, the database, being based in California, was set to Pacific Time. 6 years later someone suggested that UTC would be a better choice. When I took over management of the BMO team about 4.5 years ago, I was pretty horrified that a major application would be running in any time zone other than UTC, not in the least because of the confusion caused by an hour being repeated every year when PDT switched back to PST, since the presence or absence of DST is not noted in the database. However, we were never able to justify the required effort to move over to UTC, that is, until last year, as we were setting up a failover system in AWS. RDS, the natural choice for a MySQL-based application, supported only UTC, thus giving us a hard requirement to migrate. A heroic effort by dkl got us smoothly switched over in May 2016.

Memory-usage & perf improvements

We’ve known for some time that Bugzilla has a persistent memory leak. It was never a huge issue because the webheads would automatically restart Apache processes when their memory usage got too high, but it is understandably something that lurks in the back of the minds of the developers working on Bugzilla. Dylan finally got frustrated enough to fix a big leak, which resulted in the webheads restarting much less frequently, which in turn led to a performance increase. He’s been investigating and fixing other such leaks when he finds the time.

Stuff we’re wrapping up in 2017

Some of the bigger projects bled over into 2017.

Content Security Policy

We’ve been working on implementing CSP in BMO, starting with the new modal bug view. It was pretty hairy due to generated HTML, inline JavaScript, and other old web-dev techniques that make security harder. After some back and forth, we’re just about there; see bug 1286290 for progress.

Note that CSP can break browser extensions. Since the modal UI is relatively new, there are probably not too many extensions designed for it; however, we’ll be spreading CSP over time. And of course, we’ll be removing the old bug view at some point, which will definitely break some things.

Elastic Quicksearch

In the spring, Dylan hacked up a prototype of a quicksearch alternative powered by an Elasticsearch index. It’s lightning fast, so we explored setting it up in production. Of course a prototype is always easier than the real thing, and we had to do some structural work to BMO to make it possible, although that in turn has had side benefits. The indexing code is just about ready to roll out, and while we’re verifying that it works correctly, we’ll be finishing up the search code. You can also follow the main tracker bug for the whole deployment.

New stuff in 2017

We’re expecting to wrap up the above features in Q1, and we’ve already developed a road map for the first half of 2017 with some fun and long-awaited features. Emma will be going over this in another post, wearing her hat as BMO product manager, a job she has recently, and graciously, taken on herself!

MozReview UI Refactoring

| Comments

In Q3 the MozReview team started to focus on tackling various usability issues. We started off with a targetted effort on the “Finish Review” dialog, which was not only visually unappealing but difficult to use. The talented David Walsh compressed the nearly full-screen dialog into a dropdown expanded from the draft banner, and he changed button names to clarify their purpose. We have some ideas for further improvements as well.

David has now embarked on a larger mission: reworking the main review-request UI to improve clarity and discoverability. He came up with some initial designs and discussed them with a few MozReview users, and here’s the result of that conversation:

This design provides some immediate benefits, and it sets us up for some future improvements. Here are the thoughts behind the changes:

The commits table, which was one of the first things we added to stock Review Board, was never in the right place. All the surrounding text and controls reflect just the commit you are looking at right now. Moving the table to a separate panel above the commit metadata is a better, and hopefully more intuitive, representation of the hierarchical relationship between commit series and individual commit.

The second obvious change is that the commit table is now collapsed to show only the commit you are currently looking at, along with its position (e.g. “commit 3 of 5”) and navigation links to previous and next commits. This places the emphasis on the selected commit, while still conveying the fact that it is part of a series of commits. (Even if that series is actually only one commit, it is still important to show that MozReview is designed to operate on series.) To address feedback from people who like always seeing the entire series, it will be possible to expand the table and set that as a preference.

The commit title is still redundant, but removing it from the second panel left the rest of the information there looking rather abandoned and confusing. I’m not sure if there is a good fix for this.

The last functional change is the addition of a “Quick r+” button. This fixes the annoying process of having to select “Finish Review”, set the dropdown to “r+”, and then publish. It also removes the need for the somewhat redundant and confusing “Finish Review” button, since for anything other than an r+ a reviewer will most likely want to leave one or more comments explaining their action. The “Quick r+” button will probably be added after the other changes are deployed, in part because we’re not completely satisfied with its look and position.

The other changes are cosmetic, but they make various data and controls look much slicker while also being more compact.

We are also noodling around with a further enhancement:

This is a banner containing data about the current commit, which will appear when the user scrolls past the commits table. It provides a constant reminder of the current commit, and we may put in a way to skip up to the commits table and/or navigate between commits. We may also fold the draft/“Finish Review” banner into this as well, although we’re still working out what that would look like. In any case, this should help avoid unnecessary scrolling while also presenting a “you are here” signpost.

As I mentioned, these changes are part of an on-going effort to improve general usability. This refactoring gets us into position to tackle more issues:

  • Since the commits table will be clearly separated from the commit metadata, we can move the controls that affect the whole series (e.g. autoland) up to the first panel, and leave controls that affect only the current commit (right now, only “Finish Review”/“Quick r+”) with the second panel. Again this should make things more intuitive.

  • Similarly, this gives us a better mechanism for moving the remaining controls that exist only on the parent review request (“Review Summary”/“Complete Diff”) onto the individual commit review requests, alongside the other series controls. This in turns means that we’ll be able to do away with the parent review request, or at least make some radical changes to it.

MozReview usage is slowly ticking upwards, as more and more Mozillians are seeing the value of splitting their work up into a series of small, atomic commits; appreciating the smooth flow of pushing commits up for review; and especially digging the autoland functionality. We’re now hard at work to make the whole experience delightful.

BMO's Database Takes a Leap Forward

| Comments

For historical reasons (or “hysterical raisins” as gps says) that elude me, the BMO database has been in (ughhh) Pacific Time since it was first created. This caused some weirdness on every daylight savings time switch (particularly in the fall when 2:00-3:00 am technically occurs twice), but not enough to justify the work in fixing it (it’s been this way for close to two decades, so that means lots of implicit assumptions in the code).

However, we’re planning to move BMO to AWS at some point, and their standard db solution (RDS) only supports UTC. Thus we finally had the excuse to do the work, and, after a bunch of planning, developing, and reviewing, the migration happened yesterday without issues. I am unreasonably excited by this and proud to have witnessed the correction of this egregious violation of standard db principles 18 years after BMO was originally deployed.

Thanks to the BMO team and the DBAs!

How MozReview Helps

| Comments

A great post on code review is making its rounds. It’s started some discussion amongst Mozillians, and it got me thinking about how MozReview helps with the author’s points. It’s particularly interesting because apparently Twitter uses Review Board for code reviews, which is a core element of the whole MozReview system.

The author notes that it’s very important for reviewers to know what reviews are waiting on them, but also that Review Board itself doesn’t do a good job of this. MozReview fixes this problem by piggybacking on Bugzilla’s review flags, which have a number of features built around them: indicators, dashboards, notification emails, and reminder emails. People can even subscribe to the reminders for other reviewers; this is a way managers can ensure that their teams are responding promptly to review requests. We’ve also toyed around with the idea of using push notifications to notify people currently using Bugzilla that they have a new request (also relevant to the section on being “interrupt-driven”).

On the submitter side, MozReview’s core support for microcommits—a feature we built on top of Review Board, within our extensions—helps “keep reviews as small as possible”. While it’s impossible to enforce small commits within a tool, we’ve tried to make it as painless as possible to split up work into a series of small changes.

The MozReview team has made progress on automated static analysis (linters and the like), which helps submitters verify that their commits follow stylistic rules and other such conventions. It will also shorten review time, as the reviewer will not have to spend time pointing out these issues; when the review bots have given their r+s, the reviewer will be able to focus solely on the logic. As we continue to grow the MozReview team, we’ll be devoting some time to finishing up this feature.

BMO in 2015

| Comments

It’s been a whole year since my last BMO update, partly because I’ve been busy with MozReview (and blogging a lot about it), and partly because the BMO team got distracted from our goals by a few sudden priority changes, which I’ll get to later in this post.

Plans from 2014

Even with some large interruptions, we fully achieved three of our five goals for the year and made good progress on a fourth.

Alternative Bug Views

Have you tried out the new modal UI? Although not completely finished (it lacks a few features that the standard UI has), it’s very usable. I don’t remember the last time I had to switch back, and I’ve been using it for at least 6 months. Bonus: gone is the intermediate page when you change a bug’s product, a gripe from time immemorial!

Even though there are still a large number of controls, the new UI is a lot more streamlined. glob gave a brief presentation at a Mozilla Project Meeting in November if you’d like to learn more.

The part we haven’t yet undertaken is building on this new framework to provide alternate views of bug data depending on what the user is trying to accomplish. We want to experiment with stripping down the presented data to only what is needed for a particular task, e.g. developing, triaging, approving, etc. The new UI is a lot more flexible than the old, so in 2016 we’ll build out at least one new task-centric view.

GitHub Authentication

If you haven’t noticed, you can log into BMO via GitHub. If you’ve never used BMO before, you’ll be prompted to set up an account after authenticating. As with Persona, only users with no special privileges (i.e. not admins nor people in security groups) can log in via GitHub.

Auth Delegation

Originally designed to smooth the process of logging into Review Board, auth delegation for API keys is actually a general-use feature that greatly improves the user experience, not to mention security, of third-party apps by allowing them to delegate authentication to BMO. There’s now no reason for apps to directly ask for your BMO credentials!

MozReview Details

There’s now a panel just above the attachments table that shows all the MozReview commits associated with the displayed bug along with a bit of other info:

We’re currently sorting out a single method to display other relevant information, notably, status of reviews, and then we’ll add that to this table.

Improved Searchability

This is the big item we haven’t made much progress on. We’ve got a plan to mirror some data to an Elasticsearch cluster and wire it into Quick Search. We’ve even started on the implementation, but it’s not going to be ready until mid-2016. It will increase search speeds, understandably one of the more common complaints about BMO.

Curve balls

We had two sets of surprises in 2015. One was work that ended up consuming more time than we had expected, and the other was important work that suddenly got a big priority boost.

BMO Backup in AWS

The first is that we moved the BMO failover out of a data center in Phoenix and into the cloud. IT did most of the work, but we had to make a series of changes to BMO to facilitate the move. We also had a lot of testing to do to. The upside is that our new failover system has had more testing than our old one had for quite some time!

Hardened Security

In August we found out that an attacker had compromised a privileged BMO account, using a combination of a weak, reused password and an exploit in another web site. In addition to a huge forensics effort from the great security folks at Mozilla, the BMO team implemented a number of security enhancements to BMO, most notably two-factor authentication. This work naturally took high priority and is the main reason for the slippage of our big 2015 goals. Here’s to a more secure 2016!

Other Stuff

As usual, the BMO team rolled out a pile of smaller fixes, enhancements, improvements, and new features. A few notable examples include

  • The guided bug-entry form got a nice refresh. This is the form that users without the editbugs permission, i.e. new users, see when entering bugs. You can always get to it via the “Switch to the Bugzilla helper” link at the buttom of the advanced bug-entry form. Note that if you’re an employee, you’ve been given editbugs by default, so you’ve likely never seen the guided form. Check it out—Bugzilla might be friendlier to new contributors than you expect.

  • The platform settings for new bugs now default to all hardware and OSes, with a “Use my platform” button to easily set this to the reporter’s system parameters. This should help clear up some confusion between the reporter’s platform versus the platform the bug applies to.

  • The ability to block requests for review, feedback, and needinfo.

  • The preferences page is now better organized.

  • HTML bugmail has microdata to make GMail display a “View bug” button. Thanks to Ed Morley for the patch!

You can always find the exhaustive list of recent changes to BMO on the wiki or on the mozilla.tools.bmo group/mailing list.

Review Board History

| Comments

A few weeks ago, mdoglio found an article from six years ago comparing Review Board and Splinter in the context of GNOME engineering. This was a fascinating read because, without having read this article in advance, the MozReview team ended implementing almost everything the author talked about.

Firstly, I admit the comparison isn’t quite fair when you replace bugzilla.gnome.org with bugzilla.mozilla.org. GNOME doesn’t use attachment flags, which BMO relies heavily on. I haven’t ever submitted a patch to GNOME, but I suspect BMO’s use of review flags makes the review process at least a bit simpler.

The first problem with Review Board that he points out is that the “post-review command-line leaves a lot to be desired when compared to git-bz”. This was something we did early on in MozReview, all be it with Mercurial instead: the ability to push patches up to MozReview with the hg command. Admittedly, we need an extension, mainly because of interactions with BMO, but we’ve automated that setup with mach mercurial-setup to reduce the friction. Pushing commits is the area of MozReview that has seen the fewest complaints, so I think the team did a great job there in making it intuitive and easy to use.

Then we get to what the author describes as “a more fundamental problem”: “a review in Review Board is of a single diff”. As he continues, “More complex enhancements are almost always done as patchsets [emphasis his], with each patch in the set being kept as standalone as possible. … Trying to handle this explicitly in the Review Board user interface would require some substantial changes”. This was also an early feature of MozReview, implemented at the same time as hg push support. It’s a core philosophy baked into MozReview, the single biggest feature that distinguishes MozReview from pretty much every other code-review tool out there. It’s interesting to see that people were thinking about this years before we started down that road.

An interesting aside: he says that “a single diff … [is] not very natural to how people work with Git”. The article was written in 2009, as GitHub was just starting to gain popularity. GitHub users tend to push fix-up commits to address review points rather than editing the original commits. This is at least in part due to limitations present early on in GitHub: comments would be lost if the commit was updated. The MozReview team, in fact, has gotten some push back from people who like working this way, who want to make a bunch of follow-up commits and then squash them all down to a single commit before landing. People who strongly support splitting work into several logical commits and updating them in place actually tend to be Mercurial users now, especially those that use the evolve extension, which can even track bigger changes like commit reordering and insertion.

Back to Review Board. The author moves onto how they’d have to integrate Review Board with Bugzilla: “some sort of single-sign-on across Bugzilla and Review Board”, “a bugzilla extension to link to reviews”, and “a Review Board extension to update bugs in Bugzilla”. Those are some of the first features we developed, and then later improved on.

There are other points he lists that we don’t have, like an “automated process to keep the repository list in Review Board in sync with the 600+ GNOME repositories”. Luckily many people at Mozilla work on just one repo: mozilla-central. But it’s true that we have to add others manually.

Another is “reduc[ing] the amount of noise for bug reporters”, which you get if you confine all patch-specific discussion to the review tool. We don’t have this yet; to ease the transition to Review Board, we currently mirror pretty much everything to Bugzilla. I would really like to see us move more and more of code-related discussion to Review Board, however. Hopefully as more people transition to using MozReview full time, we can get there.

Lastly, I have to laugh a bit at “it has a very slick and well developed web interface for reviewing and commenting on patches”. Clearly we thought so as well, but there are those that prefer the simplicity of Splinter, even in 2015, although probably mostly from habit. Trying to reconcile these two views is very challenging.

MozReview's Parental Issues

| Comments

As mentioned in my previous post on MozReview, one of the biggest sources of confusion is the way we present the “squashed” diffs, that is, the diff that show all of the changes in a commit series, the sum of all the proposed changes. We also refer to these as “parent” review requests, since they function as something to hold all the commits together. They are stored in MozReview as separate review requests, similar to the individual commits.

The confusion results from several things:

  • The links to the parent are confusing: they are currently labelled “Complete diff” and “Review summary”. “Complete diff” doesn’t clearly indicate that it is the complete diff of all commits together, and “Review summary” is almost totally meaningless, since it doesn’t include all the reviews left on the commits themselves—only reviews left on the overview diff.

  • There is nothing in the UI that clearly indicates that you are viewing the squashed diff. The only indication is that none of the rows in the commit table are highlighted. This is particularly confusing when there is only one commit, since the squashed diff is identical to the commit diff.

  • You can leave reviews on the squashed diff, but you can’t leave a “ship it”. This is because we are enforcing reviewers to review individual commits. However, because there isn’t much to distinguish parent review requests from commit review requests, it can look like the review dialog is just broken.

There are a few simple things we can do to fix these problems: use better link names, put a big “This is an overview of the commit series” message, and/or put a warning “You must review individual commits” on the review dialog. But really, we need to step back and think about the way we present the squashed diffs, and if they even make sense as a concept in MozReview.

To reiterate, squashed diffs provide a complete view of a whole commit series. The concept of a commit series doesn’t exist in core Review Board (nor does it exist in many other code-review tools), but it’s central to the idea of the repository-centric approach (like in GitHub pull requests). We added this concept by storing metadata resulting from pushes to tie commit series together with a parent, and we added UI elements like the commits table.

There are three broad ways we can deal with squashed diffs going forward. We need to settle on one and make the associated UI changes to make our model clear to users.

  1. Remove squashed diffs altogether.

    This is the simplest option. Squashed diffs aren’t actually technically necessary, and they can distract reviewers from the individual commits, which is where they should be spending most of their time, since, in most cases, this is how the code will be landing in the main repository. Some other repository-centric review tools, like Critic, don’t have the concept of an overview diff, so there are precedents. However, it might be a bit heavy handed to tell reviewers that they can’t view all the commits as a single diff (at least, without pulling them down locally).

  2. Continue to allow reviews, of some sort, on squashed diffs.

    This is what we have now: reviewers can leave reviews (at the moment, comments only) on squashed diffs. If we decide we want to continue to allow users to leave reviews on squashed diffs, we’ll need to both figure out a better UI to distinguish them from the individual commits and also settle several open questions:

    • Should reviewers be able to grant ship its (i.e. r+s) on squashed diffs? This would imply that the commits probably haven’t been reviewed individually, which would defeat the purpose of a commit-centric system. That said, reviewer time is very important, so we could have a trade off to support more work flows.

    • Conversely, should reviewers be able to leave comments on the parent diff? For simplicity, we could allow reviewers to leave a “ship it” review on a squashed diff that would apply to all commits but force them to leave any comments on diffs on the commits themselves. This would essentially remove the ability to review squashed diffs themselves but would leave the convenience of saying “this is all good”.

    • If we do want to allow review comments on squashed diffs, how should they be consolidated with the reviews on individual commits? Right now, reviews (general comments and comments on diffs) for the squashed diff and all commits are all on separate pages/views. Giving one view into all activity on a commit series would be ideal if we want to support squashed-diff reviews. Arguably, this would be valuable even if we didn’t have reviews on squashed diffs.

    For comparison, GitHub pull requests support this model. There are three tabs in a pull request: “Files changed”, which is the squashed diff; “Commits”, which is a list of commits with links to the individual commit diffs; and “Conversation”, which shows comments on the commits and on the squashed diff (along with other events like updates to the commits). The way they are presented is a little confusing (comments on the squashed diff are just labelled “<user> commented on the diff”, whereas comments on the diffs are of the form “<user> commented on <file> in <commit hash>”), but it is a useful single view. However, note that pull requests do not have the concept of a “ship it” or “r+”, which makes the GitHub interface simpler.

    This approach would support multiple reviewer work flows, but it is also the most complicated, both in terms of UX and technical implementation, and it waters down the philosophy behind MozReview.

  3. Provide read-only overview diffs.

    The third approach is to keep squashed diffs but make them read only. They could be used as reference, to get a big picture of the whole series, but since they are read only, they would be easily distinguishable from commits and would force reviewers to look at the individual commits. This is really just option 1 above, with a reference view of the whole series. It would be more work than option 1 but less than option 2, and would preserve the philosophy.

The MozReview team has been leaning towards option 3. We have a mock-up that strips away a lot of the UI that would be useless in this scenario and makes the intention clear. It’s not the prettiest, but it wouldn’t take too much work to get here:

However, we’d like to hear user feedback before making any decisions. Whichever option we go with, we’ll come up with a plan to get there that ideally will have incremental improvements, depending on the complexity of the full solution, so that we can start to fix things right away.