On Thu, Jul 13, 2017 at 6:04 PM, Mark Côté <mc...@mozilla.com> wrote:
> On 2017-07-13 3:54 PM, Randell Jesup wrote: > >> On Wed, Jul 12, 2017 at 11:27 AM, Byron Jones <g...@mozilla.com> wrote: >>> >>> But indeed having also the patches in bugzilla would be good. >>>> >>>>> >>>>> no, it would be bad for patches to be duplicated into bugzilla. we're >>>> moving from bugzilla/mozreview to phabricator for code review, >>>> duplicating >>>> phabricate reviews back into the old systems seems like a step backwards >>>> (and i'm not even sure what the value is of doing so). >>>> >>> >>> I find this a strange argument. We don't know how successful >>> phrabricator >>> is going to be. The last attempt to switch review tools seems to be >>> getting killed. I think its somewhat reasonable to be skeptical of >>> further >>> review tool changes. >>> >> >> I quite agree... And I hate the idea of having the data spread across >> systems (which I also disliked in MozReview, which I tried and it ended >> up being really bad for some aspects of my ability to land patches). >> > > Mirroring comments from Phabricator to BMO is even more confusing, as we > end up with forked discussions, where some people reply only on BMO. This > happens regularly with MozReview, as a quick survey of recently fixed bugs > shows. Keeping code review in one place, loosely coupled to issue > tracking, improves readability of bugs by keeping discussion of issues > separate from specific solutions. It is also what newer systems do today > (e.g. GitHub and the full Phabricator suite), which I mention not as a > reason in and of itself but to note precedent. The plan is to move to a > separate, more modern, and more powerful code-review tool. Forked > discussions means that the bugs will always have to be consulted for code > reviews to progress, which undermines the utility of the code-review tool > itself. Loose coupling also frees us to try experiments like patches > without bugs, which has been discussed many times but has been technically > blocked from implementation. > > That said, lightweight linkages between issues and code review are very > useful. We're doing some of that, and we're open to iterating more there. > > We've been asked to be bold and innovate all across Mozilla, to try new > things and see if they work. For a long time, Mozillians have discussed > modernized workflows, with new tools that also open more avenues to > automation, but such things have been rarely attempted, and even then in a > disorganized fashion. We're finally at a time when we have the ability and > support to forge ahead, and that's what we're doing. > This. As someone who worked on MozReview, I can say unequivocally that one of the hardest parts - and I dare say the thing that held MozReview back the most - was the tight coupling with Bugzilla and the confusion and complexity that arose from it. When we deployed MozReview, we intentionally tried to not rock the boat too hard: we wanted it to be an extension of the Bugzilla-centric workflow that everyone was familiar with. Was there some boat rocking, yes: that's the nature of change. But we went out of our way to accommodate familiar workflows and tried to find the right balance between old and new. This resulted in obvious awkwardness, like trying to map ReviewBoard's "Ship It" field to Bugzilla's complicated review flag mechanism. Does a review that doesn't grant "Ship It" clear the r? flag or leave it? What happens when someone grants "Ship It" but they don't have permissions to leave review flags in Bugzilla? How do you convey r-? What about feedback flags? What happens when someone changes a review flag in Bugzilla? Then you have other awkwardness, like when you mirror the review comment to Bugzilla and it loses rich text formatting. Or someone replies to a MozReview comment on Bugzilla and you can't see that reply on MozReview. Do you disable replying to comments mirrored from MozReview? That's annoying. Do you disable the mirroring then? That's annoying too! Bi-directional sync? No way! (If you've ever implemented bi-directional sync you'll know why.) You just can't win. The usability issues stemming from trying to overlay ReviewBoard's and Bugzilla's models of how the world worked were obvious and frustrating. It took months to years to iron things out. And we arguably never got it right. Then there were technical issues. When you did things like post a series of commits to review on MozReview, this was done so as an atomic operation via a database transaction. It either worked or it didn't. But we had to mirror those reviews to Bugzilla. Bugzilla didn't have an API to atomically create multiple attachments/reviews. So not only was the publishing to Bugzilla slow because of multiple HTTP requests, but it was also non-atomic. When Bugzilla randomly failed (all HTTP requests randomly fail: it is a property of networks), the review series in Bugzilla was sometimes left in an inconsistent state because it wasn't obvious how to recover (depending on where the error occurred). So in addition to mapping alien concepts between two systems, we're trying to implement distributed transactions. It was a nightmare. And don't get me started about the technical complexity to perform automated integration tests between these systems and the pain and suffering we inflicted on MozReview developers forcing them to use fragile Docker environments so we could accurately test Bugzilla interop. It drastically slowed down development and made MozReview not pleasant to hack on. The cost to tightly couple MozReview with Bugzilla was significant. I argue it was its fatal flaw. Instead of spending time integrating automatic code linting, merge conflict notification, file watching (a feature built in to Phabricator that I think many will love), automatic reviewer selection, bug-less reviews, better Try and Autoland integration, a cleaner UI, ingestion of GitHub pull requests, etc, we spent time trying to hammer a square peg into a round hole. Instead of adding extra value via new features that weren't possible with Bugzilla/Splinter, we spent our time trying to make MozReview compatible with a dated, Bugzilla-centric workflow. While I - like everyone - have concerns about Phabricator (it is rational to question changes to high impact tools/workflows), I do support the decision to decouple code review from Bugzilla. We tried tight coupling with MozReview. And we *really* tried to make it work. But no matter what we did, it felt awkward and was brittle and time-consuming to implement. If you had the experience Mark, myself, and anyone who significantly worked on MozReview had, you would see clear as day how so many problems stemmed from the decision to tightly integrate the two tools. Then if you take a step back, approach the problem from first principles, and survey the modern tooling landscape, you realize how antiquated the workflow we were trying to support actually is. It becomes clear that the best long-term outcome is to unshackle code review from Bugzilla. It's a controversial decision and will potentially be a disruptive change, yes. But I'm pretty confident that if you had the experience that we did with MozReview and were empowered to make difficult - but justifiable decisions - you too would reach the same conclusion. I say this not as someone who was involved in the decision to stop development on MozReview, switch to Phabricator, and turn off Splinter (I wasn't really involved and was even surprised by parts of the plan like everyone else): I say this as an engineer who worked on MozReview and want what's best for Mozilla long term. Decoupling code review from Bugzilla is a justifiable, forward-thinking, and overdue change. I support the decision and applaud its ambition. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
