<div dir="ltr"><div dir="ltr">Hi everyone,<br><br>As someone who cares a lot about code review (both as a developer and as a reviewer), I care a lot about the tools I work with and the workflows I’ve built around them over the years.  I’m sure I’m not the only one there.<br><br>What Mark wrote about below brings changes to some parts of our code review workflows and I think it’s fair to assume that some people will find the change unpleasant.  I think it’s important to know that decisions like this aren’t made lightly though, and the team working on Phabricator has looked into making this integration work as much as possible, before coming to this final conclusion.<br><br>What I’ve learned from the experience of the folks who worked closely on this project over the past 3-4 years is that integrating an existing code review system with an existing issue management system is an extremely difficult task.  There are certainly huge benefits to be reaped if a tight integration is achieved, but there are great costs to also consider, and the trade-off is tricky to get right.<br><br>As much as deep down in my heart, I secretly wish nothing would ever change so that I could hold on to my old habits until who-knows-when, I realize that sometimes it’s prudent to do what’s hard -- shaking off old habits and workflows and pick up new ones, in the interest of the greater good.<br><br>In this case, even with my reviewer hat on, I think it would certainly be a mistake for us to have tried as hard as we did for MozReview to integrate BMO and Phabricator together at the expense of holding back the deployment of Phabricator and new features in it.  If our smart engineers working on this hard problem have looked at this integration issue, have tried to make it work, and have decided down the line that it’s best to correct course, my hunch is to trust them and consider maybe it’s time to change some of my old habits on how I interact with code reviews on Bugzilla.<br><br>Thanks,<br>Ehsan<br></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 12, 2018 at 10:37 AM Mark Côté <<a href="mailto:mcote@mozilla.com">mcote@mozilla.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">
<p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt" id="m_3363039229700518290gmail-docs-internal-guid-7bc550fa-7fff-946b-6d10-6c0e1883cad9"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">To reduce confusion and a growing maintenance burden, the Engineering Workflow team plans to remove two pieces of Phabricator-Bugzilla integration:</span></p><br><ol style="margin-top:0pt;margin-bottom:0pt"><li dir="ltr" style="list-style-type:decimal;font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">The setting of r+ flags on the stub attachments in that link to Phabricator revisions (<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1490687" target="_blank">https://bugzilla.mozilla.org/show_bug.cgi?id=1490687</a>).</span></p></li><li dir="ltr" style="list-style-type:decimal;font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">The Phabricator table in Bugzilla’s “My Requests” page (<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1487422" target="_blank">https://bugzilla.mozilla.org/show_bug.cgi?id=1487422</a>).</span></p></li></ol><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We plan on adding one more piece of integration: a panel on bug views (show_bug.cgi) that shows the status of Phabricator revisions in Phabricator’s terms, similar to the old MozReview table (<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1489706" target="_blank">https://bugzilla.mozilla.org/show_bug.cgi?id=1489706</a>).</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">The “stub attachments” will remain for the time being in order to facilitate tracking non-review attachment flags (checkin-needed, etc). </span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap"># Rationale and background</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">There have been a lot of questions about our decisions surrounding Bugzilla–Phabricator integration. We’ve expounded on those in various threads over the last year and a half, but I will try to go into more specifics.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">At the start of the Phabricator project, having learned a lot from MozReview, we consciously decided to limit the amount of integration to Bugzilla. This not only reduces upfront costs and maintenance burden but also avoids the complexity and ambiguity inherent in mixing two different systems together. Aside from necessary linkages like authentication, accounts, and security groups, the only other integration we implemented was the setting of r+ statuses on stub attachments and, later, adding Phabricator requests to BMO’s “My Dashboard”.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">Unfortunately both of these have had bugs and caused confusion. Since comments aren’t mirrored, the plain r+s were sometimes misleading if the revisions (or Phabricator’s email notifications) weren’t also consulted before landing. The requests view on “My Dashboard” suffered from bugs that resulted in missing requests and was further impacted by our experiments with reviewer groups that have no real analog in Bugzilla.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap"># Differing models</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">The central problem is that the models behind the two systems—Bugzilla’s attachments and flags, Phabricator’s revisions and reviews—are very different. Phabricator’s revisions have a much richer and more complex set of metadata than Bugzilla attachments. It is essentially impossible to represent Phabricator states as Bugzilla flags while preserving anything close to the expected semantics of the latter; consulting Phabricator would often be needed to make sense of any translated representation in Bugzilla. Further, Phabricator’s model is also subject to changes and additions (for example, the draft revision state, which is still being modified), which creates additional fragility and maintenance burden. Finally, not all the details we need are even exposed through APIs, in part because they are in flux.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap"># Adding revision statuses to bugs</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">That said, we realize that seeing at a glance the state of revisions associated with a given bug is very useful. We are building support into Bugzilla to view revision data without translation into Bugzilla’s terms to avoid any confusion as to the true state of revisions.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">While we could also dump data from Phabricator’s dashboard into Bugzilla’s “My Dashboard”, it would be much more work and more difficult to maintain, since Phabricator’s dashboard itself is being updated. Furthermore, as all code reviews are transitioned to Phabricator, the importance of this dashboard will grow, and the number of requests in Bugzilla will shrink. Thus relying on Phabricator to do what it does best is the better solution for the future.</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap"># Acknowledging difficulties</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We are aware that splitting code review out into a separate system is a huge change. We realize that, with our new tools and the decisions we have made around integration, we are asking you to change your workflows by setting up different email rules, looking in different places for the data you need, communicating with other Mozillians in different ways, and perhaps even establishing new practices and norms around code review. It will take time to adapt. However we are already seeing benefits in terms of automation that we haven’t previously been able to do (just one example: a user set up Herald rules to notify of changes that impact localization), and we will continue to build on this framework to accomplish goals that have been talked about for many years. Allowing the tools to do what they are best at lets us focus on new functionality, including suggested reviewers, Try support for Lando, Lando notifications, fully automated landings, and other items on our road map (</span><a href="https://wiki.mozilla.org/Engineering_Workflow/Road_Map" style="text-decoration:none" target="_blank"><span style="font-size:11pt;font-family:Arial;color:rgb(17,85,204);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:underline;vertical-align:baseline;white-space:pre-wrap">https://wiki.mozilla.org/Engineering_Workflow/Road_Map</span></a><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">).</span></p><br><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre-wrap">We appreciate your feedback and support as we work to improve the tools you use every day.</span></p><div><br></div><div>Mark<br></div></div>
_______________________________________________<br>
firefox-dev mailing list<br>
<a href="mailto:firefox-dev@mozilla.org" target="_blank">firefox-dev@mozilla.org</a><br>
<a href="https://mail.mozilla.org/listinfo/firefox-dev" rel="noreferrer" target="_blank">https://mail.mozilla.org/listinfo/firefox-dev</a><br>
</blockquote></div><br clear="all"><br>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Ehsan<br></div></div>