Phabricator Update, July 2017

[Mark Côté]

Hi all, here's a brief update on the project to deploy and integrate
Phabricator at Mozilla:

* Development Phabricator instance is up at
https://mozphab.dev.mozaws.net/, authenticated via
bugzilla-dev.allizom.org.

* Development, read-only UI for Lando (the new automatic-landing
service) has been deployed.

* Work is proceeding on matching viewing restrictions on Phabricator
revisions (review requests) to associated confidential bugs.

* Work is proceeding on the internals of Lando to land Phabricator
revisions to the autoland Mercurial branch.

* Pre-release of Phabricator, without Lando, targeted for mid-August.

* General release of Phabricator and Lando targeted for late September
or early October.

* MozReview and Splinter turned off in early December.

I have a blog post up with many more details:
https://mrcote.info/blog/2017/07/11/phabricator-update/

More to come!

Mark

[Chris Pearce]

What is the status of push-to-review support?

Chris.

[Mark Côté]

We're currently trying to figure that out. It's unlikely that it will
be available for the initial launch of Phabricator, but we hope to have
it not too long after. I'll have an update in a couple weeks.

Mark

[Martin Thomson]

On Wed, Jul 12, 2017 at 6:41 AM, Mark Côté <mc...@mozilla.com> wrote:
> * MozReview and Splinter turned off in early December.

Is this bugzilla-wide? I know that other project use splinter still.
Will those projects be able to use phabricator for their projects?

For instance, NSS uses a separate instance of phabricator, but not all
the developers are using it already. I don't think that we have NSPR
setup yet.

[Mark Côté]

Yes, we welcome other Mozilla-related projects to use the new
Phabricator cluster. In fact, we're looking for projects outside of
Firefox/mozilla-central to be part of our pre-release period, which will
help us validate the Bugzilla integration. Migrating NSS over to the
main instance is one candidate, and NSPR sounds like a possibility too.

To answer the other part of your question, MozReview will be disabled
for active use across the board, but it is currently used by a small
number of projects. Splinter will be disabled on a per-product basis,
as there may be some projects that can't, won't, or shouldn't be
migrated to Phabricator.

Mark

[Mike Hommey]

Splinter is still a nice UI to look at patches already attached to bugs.
Please don't disable it.

Mike

[Byron Jones]

>> To answer the other part of your question, MozReview will be disabled for
>> active use across the board, but it is currently used by a small number of
>> projects. Splinter will be disabled on a per-product basis, as there may be
>> some projects that can't, won't, or shouldn't be migrated to Phabricator.
>
> Splinter is still a nice UI to look at patches already attached to bugs.
> Please don't disable it.

excellent point; thanks for that feedback.

instead of disabling splinter for phabricator backed products, we could
make it a read-only patch viewer.



--
glob — engineering productivity — moz://a

[Martin Thomson]

On Wed, Jul 12, 2017 at 1:34 PM, Byron Jones <gl...@mozilla.com> wrote:
> instead of disabling splinter for phabricator backed products, we could make
> it a read-only patch viewer.

Given the number of bugs that exist with patches attached, that would
be greatly appreciated. I would also assume that attaching patches
won't stop completely either.

[Ben Kelly]

It would be nice if patch information was always contained within the bug.
Scattering it across tools makes it harder to figure out happened later
when you are researching why changes were made.

Consider that we are talking about "turning off" mozreview now. Will all
the bugzilla links to those reviews go dead? Or do we have to maintain a
second service in read-only mode forever? This would not be a problem if
the patch/review information was also stored in bugzilla instead of solely
placed in a separate tool.

[smaug]

Yeah, I live in the assumption that bugzilla bugs will contain all the review information also
in phabricator era. But indeed having also the patches in bugzilla would be good.

[Dylan Hardison]

> On Jul 11, 2017, at 22:24, Mike Hommey <m...@glandium.org> wrote:
>
> Splinter is still a nice UI to look at patches already attached to bugs.
> Please don't disable it.
>

> Mike

As a note, we currently support viewing GitHub Pull Requests in splinter as if they were patches
example: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1331305&attachment=8884864

(this has been possible for almost a year, but there were various issues preventing it from working in the data center)

I would probably follow the route that RedHat Bugzilla is taking and make use of the lovely https://diff2html.xyz/ library,
which may provide a much better patch viewing experience.

[Byron Jones]

> Yeah, I live in the assumption that bugzilla bugs will contain all the
> review information also
> in phabricator era.

i believe the current plan is to mirror just the outcome of the review
to bugzilla (ie. that a review exists, and set the review flag).
if comments should be mirrored to bugzilla has been a topic of much
discussion, so this may change.

i'm of the opinion that *not* mirroring comments to bugzilla is the
right thing to do. phabricator provides a better experience for
tracking reviews and notifications (eg. you can watch files/directories
and be notified when reviews are posted that touch that code). one of
my largest concerns is duplicating comments into bugzilla splits review
dialog between two systems making it harder to follow conversations
(unless you do bidirectional sync, which has its own set of problems).

> 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).

[Milan Sreckovic]

On 12-Jul-17 11:27, Byron Jones 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 believe the answer to this depends on the answer to the "what happens
to mozreview patches" after we switch to phabricator. If the answer was
to be "you lose all of those", then the argument could be "we don't want
to lose the patches once we switch from phabricator to product X in 20
years".

--
- Milan (mi...@mozilla.com)

[Byron Jones]

> Consider that we are talking about "turning off" mozreview now. Will all
> the bugzilla links to those reviews go dead? Or do we have to maintain a
> second service in read-only mode forever?

the patches will be archived in some form.

how this looks is yet to be fully fleshed out - ideas currently include
archiving and updating bugzilla to point to their new location (eg. s3),
or uploading patches directly to bugzilla.

[Milan Sreckovic]

Perfect, love it.

One thing that hasn't been explicitly mentioned, and I hope switching to
phabricator would fix it (though it does sounds like an orthogonal
issue) - the patches that are attached to bugzilla are often not the
ones that actually landed, because last minute changes were made and
landed manually. Will that get better with phabricator?

On 12-Jul-17 11:54, Byron Jones wrote:
>
>> Consider that we are talking about "turning off" mozreview now. Will
>> all
>> the bugzilla links to those reviews go dead? Or do we have to
>> maintain a
>> second service in read-only mode forever?
>

> the patches will be archived in some form.
>
> how this looks is yet to be fully fleshed out - ideas currently
> include archiving and updating bugzilla to point to their new location
> (eg. s3), or uploading patches directly to bugzilla.
>

--

- Milan (mi...@mozilla.com)

[Byron Jones]

Milan Sreckovic wrote:
> One thing that hasn't been explicitly mentioned, and I hope switching to
> phabricator would fix it (though it does sounds like an orthogonal
> issue) - the patches that are attached to bugzilla are often not the
> ones that actually landed, because last minute changes were made and
> landed manually. Will that get better with phabricator?

the switch to phabricator won't directly address that issue.
requiring autoland will.

this is a topic that came up on this list when requiring landing through
autoland was proposed -
https://groups.google.com/d/msg/mozilla.dev.platform/hp5_6OAKV_c/JNWydrDVBAAJ

it's a policy decision that hasn't been completely decided.


how our team currently works when using mozreview is if there's an r+
with "fix on commit" changes required, we'd make those changes, push up
the revised version to mozreview, carry forward the r+, and use autoland
to land.

[Ben Kelly]

On Wed, Jul 12, 2017 at 11:27 AM, Byron Jones <gl...@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.

Also, I have to say the whole phabricator thing has been kind of presented
as a fait accompli. As someone who continues to use splinter on a daily
basis I'm 1) skeptical and 2) kind of unhappy.

Maybe phabricator will be great, but I'll believe it when I see it. Please
don't force future data and workflow into an as-yet unproven tool.

Thanks.

Ben

[Daniel Veditz]

On Wed, Jul 12, 2017 at 8:54 AM, Byron Jones <gl...@mozilla.com> wrote:

> Consider that we are talking about "turning off" mozreview now. Will all
>> the bugzilla links to those reviews go dead? Or do we have to maintain a
>> second service in read-only mode forever?
>>
>

> the patches will be archived in some form.
>

​The final patches are available in the source repo. Will you also include
the interim versions and the important review comments that are sometimes
needed to understand a particular change was made?
​
-
​Dan Veditz​

[Boris Zbarsky]

On 7/12/17 11:54 AM, Byron Jones wrote:
> or uploading patches directly to bugzilla.

But still rewriting existing links (including from the mirrored review
comment comments, so it's clear which diff the review comments applied
to), right?

-Boris

[Botond Ballo]

On Wed, Jul 12, 2017 at 11:54 AM, Byron Jones <gl...@mozilla.com> wrote:
>
>> Consider that we are talking about "turning off" mozreview now. Will all
>> the bugzilla links to those reviews go dead? Or do we have to maintain a
>> second service in read-only mode forever?
>

> the patches will be archived in some form.
>

> how this looks is yet to be fully fleshed out - ideas currently include

> archiving and updating bugzilla to point to their new location (eg. s3), or

> uploading patches directly to bugzilla.

Note that merely uploading a MozReview patch to bugzilla potentially
loses some information: in MozReview, all of the patch's ancestors
were available in a repository such that you could pull the patch and
apply it locally without having to rebase and potentially encounter
conflicts. That's not true of plain patches uploaded to bugzilla.

Cheers,
Botond

[smaug]

On 07/12/2017 04:20 PM, Ben Kelly wrote:

Yeah, I live in the assumption that bugzilla bugs will contain all the review information also

in phabricator era. But indeed having also the patches in bugzilla would be good.

[Randell Jesup]

I would consider it a massive fail if we disabled at least read-only
splinter viewing of patches. As noted before..... let's make sure we
don't lose things we agreed on as issues.

--
Randell Jesup, Mozilla Corp
remove "news" for personal email

[Randell Jesup]

>On Wed, Jul 12, 2017 at 11:27 AM, Byron Jones <gl...@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).

>Also, I have to say the whole phabricator thing has been kind of presented
>as a fait accompli. As someone who continues to use splinter on a daily
>basis I'm 1) skeptical and 2) kind of unhappy.
>
>Maybe phabricator will be great, but I'll believe it when I see it. Please
>don't force future data and workflow into an as-yet unproven tool.

This while sequence strikes me as "Let's launch our new boat, and oh by
the way we're going to sink the old boat(s) so no one is tempted to not
move to the new boat. And, BTW, we're leaving behind some important
things, and we'll think about how we can deal with those sometime, maybe
before we launch, maybe later, maybe never."

While this may force people onto the new boat, I'm seriously unconvinced
this will be better in the short term, or that it won't end up slowing
down N*100 developers, either for a while or permanently. And the lack
of transparency in developing this plan is also very concerning.

IMO

[Joe Hildebrand]

I'm responding at the top of the thread here so that I'm not singling out any particular response.

We didn't make clear in this process how much work Mark and his team did ahead of the decision to gather feedback from senior engineers on both Selena and my teams, and how deeply committed the directors have been in their support of this change.

Seeing a need for more modern patch reviewing at Mozilla, Laura Thomson's team did an independent analysis of the most popular tools available on the market today. Working with the NSS team to pilot their selected tool, they found that Phabricator is a good fit for our development approach (coincidentally a good enough fit that the Graphics team was also piloting Phabricator in parallel). After getting the support of the Engineering directors, they gathered feedback from senior engineers and managers on the suggested approach, and tweaked dates and features to match up with our release cycles more appropriately.

Although there may be other risks that weren't identified in our approach, I'm personally satisfied that what we have in front of us is strictly better than where we are today. Of course, we'll have to sand and fill a little bit to perfect the new Phabricator approach, and deal with our transition away from existing systems such as Splinter and MozReview. However, that tweaking would be required no matter what tool we chose, or when we chose to make a change.

Therefore, I would appreciate that if you feel the need to further comment on this thread, we focus on what can be done to make this transition successful, rather than appearing to be standing in the way of progress.

For example, "I'm concerned about X; I wonder if we could do Y to mitigate that concern?" is a much more powerful approach than "X!" at this point.

—
Joe Hildebrand

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[David Anderson]

On Thursday, July 13, 2017 at 1:38:18 PM UTC-7, Joe Hildebrand wrote:
> I'm responding at the top of the thread here so that I'm not singling out any particular response.
>
> We didn't make clear in this process how much work Mark and his team did ahead of the decision to gather feedback from senior engineers on both Selena and my teams, and how deeply committed the directors have been in their support of this change.
>
> Seeing a need for more modern patch reviewing at Mozilla, Laura Thomson's team did an independent analysis of the most popular tools available on the market today. Working with the NSS team to pilot their selected tool, they found that Phabricator is a good fit for our development approach (coincidentally a good enough fit that the Graphics team was also piloting Phabricator in parallel). After getting the support of the Engineering directors, they gathered feedback from senior engineers and managers on the suggested approach, and tweaked dates and features to match up with our release cycles more appropriately.

The problem is this hasn't been transparent. It was announced as an edict, and I don't remember seeing any public discussion beforehand. If senior engineers were consulted, it wasn't many of them - and the only person I know who was, was consulted after the decision was made.

I've contributed thousands of patches over many years and haven't really seen an explanation of how this change will make my development process easier. Maybe it will, or maybe it won't. It probably won't be a big deal because this kind of tooling is not really what makes development hard. I don't spend most of my day figuring out how to get a changeset from one place to another.

The fact is that no one really asked us beforehand, "What would make development easier?" We're just being told that Phabricator will. That's why people are skeptical.

[Mark Côté]

On 2017-07-13 3:54 PM, Randell Jesup wrote:
>> On Wed, Jul 12, 2017 at 11:27 AM, Byron Jones <gl...@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.

Mark

[Gregory Szorc]

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.

[Boris Zbarsky]

On 7/13/17 9:04 PM, Mark Côté wrote:
> It is also what newer systems
> do today (e.g. GitHub and the full Phabricator suite)

I should note that with GitHub what this means is that you get
discussion on the PR that should have gone in the issue, with the result
that people following the issue don't see half the relevant discussion.
In particular, it's common to go off from "reviewing this line of code"
to "raising questions about what the desired behavior is", which is
squarely back in issue-land, not code-review land.

Unfortunately, I don't know how to solve that problem without
designating a "central point where all discussion happens" and ensuring
that anything outside it is mirrored there...

-Boris

[Jim Blandy]

Many people seem to be asking, essentially: What will happen to old bugs?
I'm trying to follow the discussion, and I'm not clear on this myself.

For example, "Splinter will be turned off." For commenting and reviewing,
okay, understood. What about viewing patches on old bugs?

Yes, Phabricator will segregate review comments and general discussion. But
keeping all the comments in one thread is a mixed blessing, too; for
example, check out bug 1271650 and try to tell me what the status of each
patch is. "Well, those patches should be split across several bugs." Okay,
but that fragments the conversation too.

There's an inherent tension here, and Ye Olde Bugzilla Patch Review Process
is more of a lovingly polished turd than a good solution.

[Jim Blandy]

Yeah, this is kind of the opposite of "No New Rationale".

https://air.mozilla.org/friday-plenary-rust-and-the-community/

[Boris Zbarsky]

On 7/14/17 1:31 AM, Jim Blandy wrote:
> But keeping all the comments in one thread is a mixed blessing, too

Absolutely.

I guess what I'm saying is we should try to have some guidelines for
when it's appropriate to take the discussion back to the bug instead of
continuing it in the review...

-Boris

[wax miguel]

lets all try

I'm

> On 14 Jul 2017, at 13:48, Jim Blandy <jbl...@mozilla.com> wrote:
>
> Yeah, this is kind of the opposite of "No New Rationale".
>
> https://air.mozilla.org/friday-plenary-rust-and-the-community/
>
>
>> On Thu, Jul 13, 2017 at 2:49 PM, David Anderson <bail...@gmail.com> wrote:
>>

[wax miguel]

I'm

[Mark Côté]

On 2017-07-14 1:31 AM, Jim Blandy wrote:
> Many people seem to be asking, essentially: What will happen to old bugs?
> I'm trying to follow the discussion, and I'm not clear on this myself.
>
> For example, "Splinter will be turned off." For commenting and reviewing,
> okay, understood. What about viewing patches on old bugs?

Migration-specific issues are still in flux, since we have still have
plenty of time to sort them out. I'll be clarifying the migration plan
as we get closer to turning off old tools. So far we've agreed that
keeping Splinter in read-only mode to view old patches is totally fine,
and Dylan (BMO lead) mentioned that there are even nicer diff viewers
that we can integrate with BMO, like https://diff2html.xyz/, which is
currently in use by Red Hat's Bugzilla installation.

Mark

[Milan Sreckovic]

Replying in general, to a random message :)

I don't have the numbers, but I imagine reviews are happening in the
hundreds every day (if we land almost 300 patches.) So, I wouldn't
expect the conversation about adding/removing/changing tools involved in
reviews to be any less complicated, passionate and involved as what
we're having here :)

I believe Mark is putting together an e-mail to give us a better place
to continue these conversations; something with meta bugs for "enable
phabricator", "disable splinter", etc. This should let us track the
issues that are raised, discuss decisions as to which of those should be
blockers, and have a better record of what actually gets resolved and how.

This should leave us *just* with the decision making and communication
rollout issues that were raised; I know we are continuously discussing
those and trying to get better at it, but it is a separate conversation
from the technical issues, so it's probably OK that it remains separate.

- Milan (mi...@mozilla.com)

[Ben Kelly]

Also a random reply.

I think this kind of effort is more likely to be successful if it gets
input and buy-in from the key stakeholders. In this case that would be the
most frequent reviewers.

It would be nice to run a bugzilla query to find the top 10 or 20
reviewers. Talk to these folks, solve their problems, and get buy-in for a
solution. The rest of the org will likely follow their lead.

Right now, though, I talk to people who spend all day reviewing and they
tell me no one ever talked to them.

I know feedback was collected, but maybe not from this group.

Anyway, just a suggestion.

Ben

On Jul 14, 2017 11:34 AM, "Milan Sreckovic" <msrec...@mozilla.com> wrote:

> Replying in general, to a random message :)
>
> I don't have the numbers, but I imagine reviews are happening in the
> hundreds every day (if we land almost 300 patches.) So, I wouldn't expect
> the conversation about adding/removing/changing tools involved in reviews
> to be any less complicated, passionate and involved as what we're having
> here :)
>
> I believe Mark is putting together an e-mail to give us a better place to
> continue these conversations; something with meta bugs for "enable
> phabricator", "disable splinter", etc. This should let us track the issues
> that are raised, discuss decisions as to which of those should be blockers,
> and have a better record of what actually gets resolved and how.
>
> This should leave us *just* with the decision making and communication
> rollout issues that were raised; I know we are continuously discussing
> those and trying to get better at it, but it is a separate conversation
> from the technical issues, so it's probably OK that it remains separate.
>
>
> On 13-Jul-17 15:41, Randell Jesup wrote:
>

> - Milan (mi...@mozilla.com)

[Mike Hommey]

On Fri, Jul 14, 2017 at 01:00:51PM -0400, Ben Kelly wrote:
> Also a random reply.
>
> I think this kind of effort is more likely to be successful if it gets
> input and buy-in from the key stakeholders. In this case that would be the
> most frequent reviewers.
>
> It would be nice to run a bugzilla query to find the top 10 or 20
> reviewers. Talk to these folks, solve their problems, and get buy-in for a
> solution. The rest of the org will likely follow their lead.
>
> Right now, though, I talk to people who spend all day reviewing and they
> tell me no one ever talked to them.
>
> I know feedback was collected, but maybe not from this group.

Feedback was collected from a selected set of the people who do the most
reviews. I'm one of them. I don't think I'm breaking any secret by
saying that the list of people who received the consultation email at
the same time as I did were maire, mconley, bz, ehsan, smaug, billm and
mattn.

That was a generic consultation as to what a review tool should provide
or not, before any decision was made on a specific product.

I guess there should be another round for implementation details, if
there isn't one already.

Mike

[Ben Kelly]

On Jul 14, 2017 6:27 PM, "Mike Hommey" <m...@glandium.org> wrote:

On Fri, Jul 14, 2017 at 01:00:51PM -0400, Ben Kelly wrote:
> I know feedback was collected, but maybe not from this group.

Feedback was collected from a selected set of the people who do the most
reviews. I'm one of them. I don't think I'm breaking any secret by
saying that the list of people who received the consultation email at
the same time as I did were maire, mconley, bz, ehsan, smaug, billm and
mattn.

That's good!

That was a generic consultation as to what a review tool should provide
or not, before any decision was made on a specific product.

I guess there should be another round for implementation details, if
there isn't one already.

Yea, getting buy-in and consensus for a solution from those heavy reviewers
would probably increase the chance of success.

Thanks.

Ben

[Gabriele Svelto]

On 14/07/2017 05:39, Boris Zbarsky wrote:
> I should note that with GitHub what this means is that you get
> discussion on the PR that should have gone in the issue, with the result
> that people following the issue don't see half the relevant discussion.
> In particular, it's common to go off from "reviewing this line of code"
> to "raising questions about what the desired behavior is", which is
> squarely back in issue-land, not code-review land.
>
> Unfortunately, I don't know how to solve that problem without
> designating a "central point where all discussion happens" and ensuring
> that anything outside it is mirrored there...

Yeah, we frequently had that problem with Gaia issues as part of Firefox
OS. Reviews were done on GitHub's PR so the bugzilla entries were often
empty (even for bugs with huge, complex patch-sets). To gather any kind
of information one had to go and check the comments on the PR itself
which not only was less-than-optimal but made life a lot harder for
those not directly involved with development.

It's not a dealbreaker for me but it's something that should be kept in
mind.

Gabriele

[Edmund Wong]

Joe Hildebrand wrote:
> I'm responding at the top of the thread here so that I'm not singling out any particular response.
>
> We didn't make clear in this process how much work Mark and his team did ahead of the decision to gather feedback from senior engineers on both Selena and my teams, and how deeply committed the directors have been in their support of this change.

Really? So? What exactly is your point?

1) That Mark and his team put a lot of effort?
2) You gathered feedback from senior engineers?
3) The directors are deeply committed with this change?

So you've asked senior engineers because they do the brunt
of the reviews. Is this the new transparent way about selecting
a tool which *everyone* uses and not just the selected
few? [No... I'm not complaining because I wasn't selected.
I'm complaining because Mozilla is doing a pdf.js with
Splinter.]

Don't get me wrong. I have absolutely no doubt Mark and his
team placed a lot of effort in setting this up. Just as a lot
of effort was put behind MozReview and Splinter. But the
thing is, when someone at the top issues an edict that
they want something 'modern', someone has to put the effort
in, so saying what you said is just a red herring. It
doesn't explain the rationale or even the criteria in
selecting the new tool.

>
> Seeing a need for more modern patch reviewing at Mozilla, Laura Thomson's team did an independent analysis of the most popular tools
available on the market today. Working with the NSS team to pilot

What exactly does a "more modern patch reviewing" mean? What
is the set of criteria that would deem a tool to be 'modern'?

All I am seeing is what I saw with PDF.js; that is, Mozilla is moving
away from in-house solutions to external ones. Fine. That is
Moco's choice.

And yes, I have used Phabricator before so I know what it looks like.
I'm just disagreeing with the necessity of setting up an external
tool to Bugzilla's Splinter.

But I suppose, I'll wait and see how this pans out. Perhaps two
years down the road, something more 'modern' will come along and
we'll go through this again.

A company such as Mozilla that's purporting to support choice is
certainly not really practicing what it preaches.

In this specific example, we have seen Mozilla break the 8th
edict of the Mozilla Manifesto; that is:

"Transparent community-based processes promote participation,
accountability and trust."

The choice of Phabricator was neither transparent, nor
community-based. So how is this going to promote participation,
accountability and trust? Good luck with that.

Again... I will wait and see how this Phabricator pans out.
Worse comes to worse, I just copy and paste the code and
review it in the comment section.

> Therefore, I would appreciate that if you feel the need to further comment on this thread, we focus on what can be done to make this transition successful, rather than appearing to be standing in the way of progress.

"standing in the way of progress"? Ouch. That is really harsh.
Has that "if you're not with us, you're against us" type of vibe.
Seriously... was that even necessary?

Well... if that's the case, I'll just wait until gets released
and have a go again.

Edmund

[jw...@mozilla.com]

So I for one, tend to like to read commentary on interesting bugs inline in my e-mail with said bug, so that I can get some context for the "why" along with "patch created" --> "comment on patch" --> "[context of patch] why did this line change" ---> ! Aha, I can help with context there! (or Maybe changing that hunk of that file is interesting to me and I can provide useful feedback).

So, a few concrete suggestions on how I imagine we can meet my desire of having patch comment/review/etc in bug, without having the downsides called out in this thread (no deep thought on difficulty of implementation for these ideas, I trust those reading to evaluate utility against the difficulty):

* Comments on patches get emailed to the people in the bug contact lists (using similar X-Bugzilla Headers)
* Comments/Changes in phabricator get viewable in bugzilla directly.
- Patch Creation/Obsolescence (options below)
* New patches in patch list, like mozreview/traditional
* An integrated table that merely queries a phabricator API
* A bmo comment
* An entry inline with bmo comments to indicate phabricator patch created/obsoleted, but without being part of bmo in any other way.
- Patch flag setting (Depends partly on solution(s) chosen above)
* Actual bmo flag flipped
* Table of phabricator reviews updated
* A bmo comment saying state of review changed
* An entry inline with bmo comments but not actually a comment
- Patch commentary
* Actual BMO comment
* A `phabricator` comment viewed in bmo (iframe/direct paste/whatever) as a read-only thing
* A BMO comment but `reply` opens in phabricator
* A BMO comment that says "review [x] updated in phabricator: [link]"
* A inline history entry showing that a review comment happened in phabricator, with link.
* ...etc?

The bottom line for me, is that I'd like to be able to see, at a glance, that "stuff" happened in the review tool when I'm looking at a bug in bmo. There are various degrees of this that could make me happier than unhappy, though I'm aware that some of my own desires conflict directly with desires of others, and in relative difficulty to implement/maintain.

I'd love to hear the outcome of this subthread, regardless of what choice (if any) of what I proposed, is made.

~Justin Wood (Callek)

[Gregory Szorc]

> On Jul 15, 2017, at 23:36, Gabriele Svelto <gsv...@mozilla.com> wrote:
>
>> On 14/07/2017 05:39, Boris Zbarsky wrote:

>> I should note that with GitHub what this means is that you get
>> discussion on the PR that should have gone in the issue, with the result
>> that people following the issue don't see half the relevant discussion.
>> In particular, it's common to go off from "reviewing this line of code"
>> to "raising questions about what the desired behavior is", which is
>> squarely back in issue-land, not code-review land.
>>
>> Unfortunately, I don't know how to solve that problem without
>> designating a "central point where all discussion happens" and ensuring
>> that anything outside it is mirrored there...
>

> Yeah, we frequently had that problem with Gaia issues as part of Firefox
> OS. Reviews were done on GitHub's PR so the bugzilla entries were often
> empty (even for bugs with huge, complex patch-sets). To gather any kind
> of information one had to go and check the comments on the PR itself
> which not only was less-than-optimal but made life a lot harder for
> those not directly involved with development.

If the bug is only serving as an anchor to track code review, then the question we should be asking is "do we even need a bug."

I've explored this a bit in https://gregoryszorc.com/blog/2015/01/16/bugzilla-and-the-future-of-firefox-development/. That post was written 2.5 years ago. If you replace "MozReview" with "Phabricator," many of the points I made are still relevant. We're just taking a drastically different path. Also, it was written very early in MozReview's development and reflected more of a long term workflow we wanted to enable. It became apparent that many things would not be easily achievable or were not reasonable at that time. That's why we dropped/paused many of the more drastic ideas. Making the decision to decouple review from issue tracking with Phabricator brings many of them back to the table.

[Mike Hoye]

On 7/16/17 11:10 PM, Edmund Wong wrote:
> Joe Hildebrand wrote:
>> I'm responding at the top of the thread here so that I'm not singling out any particular response.
>>
>> We didn't make clear in this process how much work Mark and his team did ahead of the decision to gather feedback from senior engineers on both Selena and my teams, and how deeply committed the directors have been in their support of this change.
> Really? So? What exactly is your point?

This sarcasm is unwarranted.

Given that we've been talking about this stuff for years now, I think
it's very clear that we haven't come to this point by "somebody at the
top issuing an edict that they want something modern"; the decision to
commit to Phabricator was ultimately announced on May 11th, and that
decision wasn't made unilaterally or lightly.

We've been discussing how to improve our tooling and processes in open
forums for years now, most frequently on dev-platform. Despite that,
it's true that a lot of the major influences of this decision have been
mostly invisible to people who aren't involved in the day to day work on
infrastructure and tooling, in the way that icebergs are mostly invisible.

Some of the things that haven't made it into this thread that have
influenced this decision include:

- a real and pressing requirement to update our commit access policies
and practices, and backstop them with reliable tooling (a discussion you
participated in, as I recall),
- the significant engineering burdens (including opportunity cost and
bus-factor risk) of being the sole owner/user of a major piece of
infrastructure software that's not our core product, and
- the much more pernicious set of hidden costs we incur from _failing_
to standardize on certain tools and processes.

It's a very serious drag on product development and contributor
participation if everyone - paid senior engineers and new contributors
alike - needs to use _this_ tool and process and coding style (and and
and) to participate in one part of the project, and _that_ etc. etc. to
participate in another. Or, sometimes, even other corners of the same
codebase.

Each of those on their own is a huge deal, probably big enough to
warrant a major change in our tooling and processes. And it's true that
we haven't done a great job of explicitly spelling out what a big deal
they are, and how much they've informed our decision-making. But while
we support - and will always support - user choice in our products,
there aren't enough engineering hours in a year for us to support a
bunch of different ways of shipping, securing, measuring and improving
the tools and processes that provide those choices _to_ our users.

So, yes, we could have done a better job of communicating here and
making our process more transparent, but I'm very confident that this
decision will make life a lot better for Mozilla's casual contributors
and broader community as much as for Mozilla's full-time engineers. If
you'd like to dig into the details of "why" I'd be happy to do so. And
if we can do that without unnecessary snark, all the better.


- mhoye

[Mark Côté]

I filed a central tracker bug for production Phabricator deployment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1381498. I have filed
blockers and dependencies for a variety of related tasks as discussed in
these threads.

Mark

[Edmund Wong]

Mike Hoye wrote:

>
> Given that we've been talking about this stuff for years now, I think
> it's very clear that we haven't come to this point by "somebody at the
> top issuing an edict that they want something modern"; the decision to
> commit to Phabricator was ultimately announced on May 11th, and that
> decision wasn't made unilaterally or lightly.

It's hardly fair to say that "we've been talking about this stuff for
years now" when this is the very first time it's been posted. There
was no public notification of the intention of doing away with
splinter/MozReview. Sure, there may have been talks but were they
public and not just hidden in some obscure thread?

>
> We've been discussing how to improve our tooling and processes in open
> forums for years now, most frequently on dev-platform. Despite that,
> it's true that a lot of the major influences of this decision have been
> mostly invisible to people who aren't involved in the day to day work on
> infrastructure and tooling, in the way that icebergs are mostly invisible.

Not exactly a good analogy there, considering icebergs can also
sink ships.

So this discussion has been pretty much invisible from people who use
the tools and only for the select few.

> Some of the things that haven't made it into this thread that have
> influenced this decision include:
>
> - a real and pressing requirement to update our commit access policies
> and practices, and backstop them with reliable tooling (a discussion you
> participated in, as I recall),

Yes. I did participate in that. Though I'm not entirely sure
what came off that discussion.

> - the significant engineering burdens (including opportunity cost and
> bus-factor risk) of being the sole owner/user of a major piece of
> infrastructure software that's not our core product, and

Oh hell yeah. This truck/bus factor is a definite PITA, and
I applaud any effort in fixing this.

> - the much more pernicious set of hidden costs we incur from _failing_
> to standardize on certain tools and processes.

I agree as well.

>
> It's a very serious drag on product development and contributor
> participation if everyone - paid senior engineers and new contributors
> alike - needs to use _this_ tool and process and coding style (and and
> and) to participate in one part of the project, and _that_ etc. etc. to
> participate in another. Or, sometimes, even other corners of the same
> codebase.

Oh... hell yeah.

You've mentioned quite a lot of pain points in the whole process, and
I certainly agree with them. However, it would've been a better
process if there was some sort of public declaration of intention
of moving to a tool which would streamline the whole review and
commit process. While there may have been a smattering of discussions
here of commit policies, there wasn't a concise, for lack of a better
word, glue to point out that there was going to be this change.

The point is this. We have bugzilla and having a review process
that enable people to review patches without needing to jump to
another tool, would be more effective than needing to jump
from one tool (bugzilla) to another one. If Phabricator does
the job well, then so be it.

Thank you for your clarifications, Mike.

Edmund

[Ben Kelly]

On Mon, Jul 17, 2017 at 12:27 PM, Gregory Szorc <gsz...@mozilla.com> wrote:

> If the bug is only serving as an anchor to track code review, then the
> question we should be asking is "do we even need a bug."
>

In my experience the answer to this is "yes, we need a bug". I very rarely
have a one-to-one mapping between bugs and review requests. Sometimes
there are many reviews per bug. Sometimes there are no reviews in a bug,
but it still contains useful information. All the meta information around
bug blockers/dependents is orthogonal to reviews.

If you want to get rid of bugzilla in favor of your new tool, you are
really creating a new bug tracker with a built-in review tool, IMO.

[Mark Côté]

On 2017-07-17 8:46 PM, Edmund Wong wrote:
> Mike Hoye wrote:
>
>>
>> Given that we've been talking about this stuff for years now, I think
>> it's very clear that we haven't come to this point by "somebody at the
>> top issuing an edict that they want something modern"; the decision to
>> commit to Phabricator was ultimately announced on May 11th, and that
>> decision wasn't made unilaterally or lightly.
>
> It's hardly fair to say that "we've been talking about this stuff for
> years now" when this is the very first time it's been posted. There
> was no public notification of the intention of doing away with
> splinter/MozReview. Sure, there may have been talks but were they
> public and not just hidden in some obscure thread?

It was announced in May
(https://groups.google.com/forum/#!topic/mozilla.tools/4qroY2Iia9I),
linked to in this forum:
https://groups.google.com/d/msg/mozilla.dev.platform/qh5scX3Gk2U/xCWe8jrOAQAJ

Mark

[Edmund Wong]

Mark Côté wrote:
> It was announced in May
> (https://groups.google.com/forum/#!topic/mozilla.tools/4qroY2Iia9I),
> linked to in this forum:
> https://groups.google.com/d/msg/mozilla.dev.platform/qh5scX3Gk2U/xCWe8jrOAQAJ

I stand corrected, thanks. I would've thought that'd be put in
moz.dev.planning or at least cross-posted since it is a planning
issue.

Edmund

[Edmund Wong]

Hi Joe,

I just want to publicly apologize for being sarcastic in my original
post to you.

I could've found a better voice and the frustration clouded my
judgement.

I'm sorry.

Edmund

[Randell Jesup]

>On 7/13/17 9:04 PM, Mark Côté wrote:
>> It is also what newer systems
>> do today (e.g. GitHub and the full Phabricator suite)
>
>I should note that with GitHub what this means is that you get discussion
>on the PR that should have gone in the issue, with the result that people
>following the issue don't see half the relevant discussion. In particular,
>it's common to go off from "reviewing this line of code" to "raising
>questions about what the desired behavior is", which is squarely back in
>issue-land, not code-review land.

Yes, exactly. Once the data is in two places, there's no way to avoid
some discussions occuring in the "wrong" place. And Github shows it
clearly happens all the time, so you still have to flip back and forth
between looking at N review-streams of comments, and the bug, and maybe
looking at the dates of comments, in order to understand everything.

>Unfortunately, I don't know how to solve that problem without designating a
>"central point where all discussion happens" and ensuring that anything
>outside it is mirrored there...

Ditto - I think it's inherent if you can reply to review comments in the
separate tool. Making the review-tool *worse* for commenting
(features/ease-wise) can actually kinda help, at the cost of hurting
responding to review comments. Googe's Issue via codereview tooling
kinda takes this approach, from what I've seen, with *very*
non-integrated patches vs bugs.

GPS's comments about the pain of syncing also makes total sense, in that
MozReview tried to mostly attach to existing API points of bugzilla and
do things noone anticipated would be done.

I don't think limiting comments in phabricator is a viable option, nor
making it less-featured, so we may have to eat the pain and lash people
with wet noodles if they post in the "wrong" place.

--
Randell Jesup, Mozilla Corp
remove ".news" for personal email

[Randell Jesup]

Good! your recent posting made me believe you'd gone back to "splinter
must be totally turned off". Thanks. I still have significant
questions about how security-bug access and review (and security of such
items) will work; is there a design? What has changed since our
discussion in May, and have you met with the security engineers and
major security reviewers/triagers?

Thanks

--
Randell Jesup, Mozilla Corp

remove "news" for personal email

[Mark Côté]

Some platform-security people were presented with the integration plan, but there wasn't much commentary. Most of the discussion has been around implementation details, some of which have been quite tricky.

Not much has changed from the original plan except that we are looking into mirroring the CC list over as well as the security groups. Ideally we will have an exact match of the set of users who can view a bug and the set that can view associated revisions.

Some of this stuff has already landed on our development instance. We'll have some docs and screencasts to show how it works.

Mark