Re: The future of commit access policy for core Firefox

[Randell Jesup]

Responding to an old thread... (had saved as draft but didn't realize I
hadn't sent).  Only replying to .platform here since last time I
cross-post-replied it didn't show up.

>I want to explicitly state that we're moving towards
>N=~10 people having raw push access to the Firefox repos with the majority
>of landings occurring via autoland (via Conduit via MozReview/Bugzilla).
>This reduces our attack surface area (less exposure to compromised SSH
>keys),

Reducing our attack surface is (on the surface) good... but...

>establishes better auditing (history of change will be on
>Bugzilla/MozReview and not on a random local machine),

I presume you mean rebases and nit-handling. I see this as unimportant.

>eliminates potential
>for bugs or variance with incorrect rebasing done on local machines,

Ok, but I don't see this as critical, and rarely if ever see issues with this.

>and allows us to do extra things as part of the landing/rebase, such as
>automatically reformat code to match unified code styles,

Oh, that is something that would be a *massive* annoyance and likely
involve destroying a bunch of history info (or rather hiding it from
easy access).  

>do binding generation, etc. They say all problems in computer science
>can be solved by another level of indirection.

"Can be" and "Is a good idea" are two very separate things... :-) :-)

>Deferred landing (autoland) is such an
>indirection and it solves a lot of problems.

"A lot of problems" - please, this needs very concrete enumeration and
discussion.  I do see one (SSH key compromise), though this isn't the
only way to deal with that.  The rest listed above I don't see as
compelling when weighed against the costs (direct and indirect) of this
proposal.  I also agree with a lot of when Ehsan said.

Also, the SSH issue - compromised SSH keys are certainly an avenue for
compromise of our binaries, but there are a whole bunch more ways that
could still happen, ways that don't leave the forensic trails checkins
would (as ekr mentioned).  How many times (that we know of) has SSH
compromise led to a rogue checkin that got to release? (feel free to
answer that privately, for anyone who knows.)

>It will be happening. Most of
>us will lose access to push directly to the Firefox repos. We won't be
>losing ability to initiate a push, however. For the record, I'm not in
>favor of making this change until the tooling is such that it won't be a
>significant workflow/productivity regression. This is a reason why it
>hasn't been made yet.

I don't see how this proposal will achieve the goal you state as a
blocker.  (E.g. ehsan's and other's comments.)

>A handful have commented on whether a rebase invalidates a previous r+.
>This is a difficult topic. Strictly speaking, a rebase invalidates
>everything because a change in a commit being rebased over could invalidate
>assumptions. Same goes for a merge commit. In reality, most rebases are
>harmless and we shouldn't be concerned with their existence.

This speaks against some of the benefits mentioned above.

>In the cases where it does matter, we can help prevent things from
>falling through the cracks by having the review tool detect when
>in-flight reviews touch the same files / topics and route them to the
>same reviewer(s) and/or notify the different reviewer(s) so people are
>aware of potential for conflict.  This feature was conceived before
>MozReview launched but (sadly) hasn't been implemented.

I can't imagine how this will work (and be effective) in practice.

>There have been a few comments on interdiffs when rebases are in play. I
>want to explicitly state that there is no single "correct" way to display a
>diff much less an interdiff much less an interdiff when a rebase is
>involved. This is why tools like Git have multiple diffing algorithms
>(minimal, patience, histogram, Myers). This is why there are different ways
>of rendering a diff (unified, side-by-side, 3-way). Rendering a simple diff
>is hard. Rendering an interdiff is harder. Unfortunately, central review
>tools force a specific rendering on users. ReviewBoard does allow some
>tweaking of diff behavior (such as ignore whitespace). But no matter what
>options you use, someone will be unhappy with it. An example is MozReview's
>handling of interdiffs. It used to hide lines changed by a rebase that
>weren't changed in the commit up for review. But that was slightly buggy in
>some situations and some people wanted to actually see those changes, so
>the behavior was changed to show all file changes in the interdiff. This
>made a different set of people unhappy because the changes were spurious
>and contributed noise. In summary, you can't please all the people all the
>time. So you have to pick a reasonable default then debate about adding
>complexity to placate the long tail or handle corner cases. Standard
>product design challenges. On top of that, technical users are the 1% and
>are a very difficult crowd to please.

I've dealt with these 

Re: The future of commit access policy for core Firefox

[Gregory Szorc]

On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor  wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to
> outline a set of key goals for any change we make.  These goals have been
> informed by a set of stakeholders from across the project including the
> engineering, security, release and QA teams.  It's inevitable that any
> significant change will disrupt longstanding workflows.  As a result, it is
> critical that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>- Compromising a single individual's credentials must not be
>sufficient to land malicious code into our products.
>- Two-factor auth must be a requirement for all users approving or
>pushing a change.
>- The change that gets pushed must be the same change that was
>approved.
>- Broken commits must be rejected automatically as a part of the
>commit process.
>
>
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
>
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.
>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates with 
> review
>   flags.
>   - Reviewers and any other approvers interact with the changeset as
>   today (including ReviewBoard if preferred), with Bugzilla flags as the
>   canonical source of truth.
>   - Upon approval, the changeset will be pushed into autoland.
>   - If the push is successful, the change is merged to
>   mozilla-central, and the bug updated.
>
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution.  If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
>

(Responding to several topics from the top post because I don't want to
spam with multiple replies.)

I think a lot of people are conflating "push access" with "ability to land
something." The distinction is the former grants you privileges to `hg
push` directly to repos on hg.mozilla.org and the latter allows delegation
of this ability. It is easy to conflate them because today "push access"
(level 3) gives you permission to trigger the landing (via the autoland
service via MozReview). But this is only because it was convenient to
implement this way. I want to explicitly state that we're moving towards
N=~10 people having raw push access to the Firefox repos with the majority
of landings occurring via autoland (via Conduit via MozReview/Bugzilla).
This reduces our attack surface area (less exposure to compromised SSH
keys), establishes better auditing (history of change will be on
Bugzilla/MozReview and not on a random local machine), eliminates potential
for bugs or variance with incorrect rebasing done on local machines, and
allows us to do extra things as part of the landing/rebase, such as
automatically reformat code to match unified code styles, do binding
generation, etc. They say all problems in computer science can be solved by
another level of indirection. Deferred landing (autoland) is such an
indirection and it solves a lot of problems. It will be happening. Most of
us will lose access to push directly to the Firefox repos. We won't be
losing ability to initiate a push, however. For the record, I'm not in
favor of making this change until the tooling is such that it won't be a
significant workflow/productivity regression. This is a reason why it
hasn't been made yet.

A handful have commented on whether a rebase invalidates a previous r+.
This is a difficult topic. Strictly speaking, a rebase invalidates
everything because a 

Re: The future of commit access policy for core Firefox

[Steven MacLeod]

On Fri, Mar 17, 2017 at 3:55 AM, Bobby Holley  wrote:

> On Fri, Mar 17, 2017 at 12:41 AM, Jean-Yves Avenard  >
> wrote:
>
> > And yet, despite many people’s concerns, it appears that policy of
> > removing r+ whenever a new push has been made effective.
> >
>
> To my knowledge, mconnor's proposal has yet to get anywhere close to an
> actual policy. Was there a behavior change in the MozReview/autoland
> workflow to do this? That sounds like something we should fix, but in the
> mean time it seems fine to me to push your patch manually to inbound.
>

There has been no behavior change (MozReview landing approval and
carryforwards have not been changed in quite a while)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Bobby Holley]

On Fri, Mar 17, 2017 at 12:41 AM, Jean-Yves Avenard 
wrote:

> And yet, despite many people’s concerns, it appears that policy of
> removing r+ whenever a new push has been made effective.
>

To my knowledge, mconnor's proposal has yet to get anywhere close to an
actual policy. Was there a behavior change in the MozReview/autoland
workflow to do this? That sounds like something we should fix, but in the
mean time it seems fine to me to push your patch manually to inbound.


>
> And so, here I am with a r+ requesting to fix a comment, I have to ask for
> r+ again from someone not in my timezone and already on week-end.
>
> Turn around time, from 30 minutes to 3.5 days…. How is that making our
> tree safer?
>
> JY
>
> > On 15 Mar 2017, at 4:15 pm, Mike Hoye  wrote:
> >
> >
> >
> > On 2017-03-14 7:10 PM, Jean-Yves Avenard wrote:
> >> /me just loves when a new set of “rules” are put in place to prevent a
> problem that has never existed so far and will be a hindrance to everyone
> in the future.
> >
> > Two dozen or so of our most veteran engineers are deeply involved in
> this discussion. Their time and attention are extraordinarily valuable, and
> there is no question about their commitment to Mozilla's success. And yet:
> here they are, working through the details.
> >
> > On top of everything else that's been said here, maybe take a moment to
> reflect on that.
> >
> > - mhoye
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
>
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Jean-Yves Avenard]

And yet, despite many people’s concerns, it appears that policy of removing r+ 
whenever a new push has been made effective.

And so, here I am with a r+ requesting to fix a comment, I have to ask for r+ 
again from someone not in my timezone and already on week-end.

Turn around time, from 30 minutes to 3.5 days…. How is that making our tree 
safer?

JY

> On 15 Mar 2017, at 4:15 pm, Mike Hoye  wrote:
> 
> 
> 
> On 2017-03-14 7:10 PM, Jean-Yves Avenard wrote:
>> /me just loves when a new set of “rules” are put in place to prevent a 
>> problem that has never existed so far and will be a hindrance to everyone in 
>> the future.
> 
> Two dozen or so of our most veteran engineers are deeply involved in this 
> discussion. Their time and attention are extraordinarily valuable, and there 
> is no question about their commitment to Mozilla's success. And yet: here 
> they are, working through the details.
> 
> On top of everything else that's been said here, maybe take a moment to 
> reflect on that.
> 
> - mhoye
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform



smime.p7s
Description: S/MIME cryptographic signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Mike Hoye]

On 2017-03-14 7:10 PM, Jean-Yves Avenard wrote:

/me just loves when a new set of “rules” are put in place to prevent a problem 
that has never existed so far and will be a hindrance to everyone in the future.


Two dozen or so of our most veteran engineers are deeply involved in 
this discussion. Their time and attention are extraordinarily valuable, 
and there is no question about their commitment to Mozilla's success. 
And yet: here they are, working through the details.


On top of everything else that's been said here, maybe take a moment to 
reflect on that.


- mhoye
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Jean-Yves Avenard]

> On 12 Mar 2017, at 9:40 pm, Ehsan Akhgari  wrote:
> And I still don't understand what the proposal means with rebases in
> practice.  What if, after automation tries to land your change after you
> got your final r+ the final rebase fails and you need to do a manual
> rebase?  Do you need to get another r+?  What happens if you're touching
> one of our giant popular headers and someone else manages to land a
> conflict while your reviewer gets back to you?


+1, this is a very common scenario for me…

/me just loves when a new set of “rules” are put in place to prevent a problem 
that has never existed so far and will be a hindrance to everyone in the future.

smime.p7s
Description: S/MIME cryptographic signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[zbraniecki]

On Monday, March 13, 2017 at 7:45:44 AM UTC-7, Byron Jones wrote:
> David Burns wrote:
> > We should try mitigate the security problem and fix our nit problem 
> > instead of bashing that we can't handle re-reviews because of nits.
> one way tooling could help here is to allow the reviewer to make minor 
> changes to the patch before it lands.
> ie.  "r+, fix typo in comment before landing" would become "r+, i fixed 
> the comment typo"

I don't think it's realistic to expect already-overloaded reviewers to do even 
more.

In my experience, reviewer's time is worth ~4 times more than patch author's 
time.
That's a completely arbitrary number, but represents how in my experience the 
load balance work.

So, I'd actually say we should do everything possible to *minimize* the amount 
of time required from the reviewer, rather than increasing it.

And I also don't think that increasing the number of reviewers would fix it. 
Reviewer by nature is often a senior engineer trying to balance out writing 
patches that very few people can, and review patches that less experienced 
engineers wrote.

Their time is insanely valuable because neither of those tasks can be easily 
done by the person requesting the review.

Of course there are exceptions like peer-reviews and rubber-stamping of a 
patch, but in general, I'd like us to think about shifting the burden onto 
automation / patch author to do as much work as possible before the reviewer 
commits their time.
And once their done, once again we should imho limit the time we expect from 
the reviewer for any follow-up reviews.

For that reason, the lack of interdiff in rebase scenario in MR is a major 
hassle in my experience. And the idea that the reviewer has to re-review 
multiple times or edit the patch themselves, as a step in the wrong direction.

Also, the idea that "anyone can re-review the patch" is very shaky. It would 
not work in the most crucial and delicate areas where the number of people 
familiar with the area is just low. Say, accessibility, graphics, 
internationalization, security etc.

In those lines, there's often a single person in the organization who can 
comfortably review the patch, and if they're in a different timezone, then 
asking a random reviewer on IRC for a review on nits is an illusion if the nits 
are anything beyond "update the comment".

On top of that, the idea also taps into the concern I raised above. Cognitive 
load required for a reviewer to step into a bug, skim through all comments, 
patch history and latest review with request for nits to understand if the nits 
represent the original reviewer request is also non trivial.

The way it's presented in this thread feels like a utopian vision where anyone 
can just take a quick glance and stamp an r+, but in reality it'll either add 
significantly to the load of already overloaded group in our project, or become 
an illusion of security with people just accepting everything from people they 
know.

I'm actually concerned that in the era where most projects go in the direction 
of streamlining the development and reducing the bureaucracy as much as 
possible (post-landing reviews, peer-reviews etc.), we're talking about adding 
another hoop to jump through.

I'm all for increased security (2FA etc.), but unless there's an unspoken set 
of cases where security of our project has been compromised by a change in the 
patch that was added after r+, I'd like to question if we're really at the 
point where we need such tradeoff.

zb.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Ehsan Akhgari]

On 2017-03-12 4:53 PM, smaug wrote:
> On 03/12/2017 10:40 PM, Ehsan Akhgari wrote:
>> On 2017-03-11 9:23 AM, smaug via governance wrote:
>>> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
 On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
 governa...@lists.mozilla.org> wrote:
>
> I'd be ok to do a quick r+ if interdiff was working well.

 Depending on the relative timezones of the reviewer and reviewee, that
 could delay landing by 24 hours or even a whole weekend.

>>> The final r+, if it is just cosmetic changes wouldn't need to be done by
>>> the same reviewer.
>>
>> OK, but what is the exact time zone efficient way to ensure that no huge
>> amount of delay is added for the turn around of this final review round?
>>
>> Let's be realistic.  In practice, adding one extra person in the process
>> of landing of the patches that currently land with r+-with-nits *will*
>> slow them down.  And we should expect that it will slow them down by
>> factors such as time zones, people missing bugmail, etc,
> 
> Why we need to expect that? In my mind the process would be closer to
> "I want to land this patch, asking ok-to-land on irc." And then someone
> who has the rights to say ok to that would
> mark the patch after seeing that the interdiff doesn't add anything
> inherently bad.
> (well working interdiff and tooling in general is rather critical)

Depending on the timezone, finding people on IRC may be challenging.
For example, many of our colleagues in Taipei may find this kind of IRC
reviews difficult if the people who need to look at the change are on
the other side of the planet.

>> all of the
>> reasons why currently one review can end up taking a week,
> I see this being _very_ different to normal reviews.
> 
> I do understand that people don't want extra process. Overhead is always
> overhead.
> But the overhead here might not be as bad as feared.
> 
>> it may end up
>> being that final round of review after this proposed change.
>>
>> And I still don't understand what the proposal means with rebases in
>> practice.  What if, after automation tries to land your change after you
>> got your final r+ the final rebase fails and you need to do a manual
>> rebase?  Do you need to get another r+?  What happens if you're touching
>> one of our giant popular headers and someone else manages to land a
>> conflict while your reviewer gets back to you?
>>
>>> Perhaps we shouldn't even call the last step a review. It would be
>>> "ok-to-land".
>>> r+ without asking any changes would implicitly contain that
>>> "ok-to-land".
>>> (if rebasing causes some changes, that would then need explicit
>>> "ok-to-land")
>>
>> Are you suggesting a change to the nature of the review process for the
>> last step with the rename?
> 
> The last step here would be quite different to normal reviews. It is
> just a rather quick glance-over that no
> obviously evil code is about to land. (as I said, well working interdiff
> is absolutely critical to make this working)

OK, this distinction is key.  To me this seems adding process for the
sake of adding process.  Is this really going to be all that much better
at preventing vulnerabilities from sneaking in than what we have today?

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[James Graham]

On 13/03/17 14:45, Byron Jones wrote:

David Burns wrote:

We should try mitigate the security problem and fix our nit problem
instead of bashing that we can't handle re-reviews because of nits.

one way tooling could help here is to allow the reviewer to make minor
changes to the patch before it lands.
ie.  "r+, fix typo in comment before landing" would become "r+, i fixed
the comment typo"



Assuming you mean "and land without further review", I don't see how 
this has different security properties from r+-with-nits in the — 
reasonably common — case that the patch author is at least as trusted as 
the reviewer (e.g. both L3 today).


I do think that tooling to support multiple authors collaborating on a 
single branch is a good thing independent of the changes discussed in 
this thread.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Byron Jones]

David Burns wrote:
We should try mitigate the security problem and fix our nit problem 
instead of bashing that we can't handle re-reviews because of nits.
one way tooling could help here is to allow the reviewer to make minor 
changes to the patch before it lands.
ie.  "r+, fix typo in comment before landing" would become "r+, i fixed 
the comment typo"


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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[David Burns]

As the manager of the sheriffs, I am in favour of this proposal.

The reasons why are as follow (and to note there are only 3 paid sheriffs
to try cover the world):

* A number of r+ with nits land up in the sheriffs queue for
checkin-needed. This then puts the onus on the sheriffs, not the reviewer
that the right thing has been done. The sheriffs do no have the context
knowledge of the patch, never mind the knowledge of the system being
changed.

* The throughput of patches into the trees is only going to increase. If
there are failures, and the sheriffs need to back out, this can be a long
process depending on the failure leading to pile ups of broken patches.

A number of people have complained that using autoland doesn't allow us to
fail forward on patches. While that is true, there is the ability to do
T-shaped try pushes to make sure that you at least compile on all
platforms. This can easily done from Mozreview (Note: I am not suggesting
we move to mozreview).

Regarding burden on reviewers, the comments in this thread just highlight
how broken our current process is by having to flag individual people for
reviews. This leads to the a handful of people doing 50%+ of reviews on the
code. We, at least visibly, don't do enough to encourage new committers
that would lighten the load to allow the re-review if there are nits. Also,
we need to do work to automate the removal of nits to limit the amount of
re-reviews that reviewer can do.

We should try mitigate the security problem and fix our nit problem instead
of bashing that we can't handle re-reviews because of nits.

David

On 9 March 2017 at 21:53, Mike Connor  wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to
> outline a set of key goals for any change we make.  These goals have been
> informed by a set of stakeholders from across the project including the
> engineering, security, release and QA teams.  It's inevitable that any
> significant change will disrupt longstanding workflows.  As a result, it is
> critical that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>
>- Compromising a single individual's credentials must not be
>sufficient to land malicious code into our products.
>- Two-factor auth must be a requirement for all users approving or
>pushing a change.
>- The change that gets pushed must be the same change that was
>approved.
>- Broken commits must be rejected automatically as a part of the
>commit process.
>
>
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
>
>
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.
>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates with 
> review
>   flags.
>   - Reviewers and any other approvers interact with the changeset as
>   today (including ReviewBoard if preferred), with Bugzilla flags as the
>   canonical source of truth.
>   - Upon approval, the changeset will be pushed into autoland.
>   - If the push is successful, the change is merged to
>   mozilla-central, and the bug updated.
>
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution.  If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
>
> -- Mike
>
> ___
> firefox-dev mailing list
> firefox-...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>
>
___
dev-platform mailing list

Re: The future of commit access policy for core Firefox

[smaug]

On 03/10/2017 12:59 AM, Bobby Holley wrote:

At a high level, I think the goals here are good.

However, the tooling here needs to be top-notch for this to work, and the
standard approach of flipping on an MVP and dealing with the rest in
followup bugs isn't going to be acceptable for something so critical to our
productivity as landing code. The only reason more developers aren't
complaining about the MozReview+autoland workflow right now is that it's
optional.

The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
not to pay attention to new workflows because they have too much else on
their plate. The onus needs to be on the team deploying this to have the
highest-volume committers using the new system happily and voluntarily
before it becomes mandatory. That probably means that the team should not
have any deadline-oriented incentives to ship it before it's ready.

Also, getting rid of "r+ with comments" is a non-starter.


FWIW, with my reviewer hat on, I think that is not true, _assuming_ there is a 
reliable interdiff for patches.
And not only MozReview patches but for all the patches. (and MozReview 
interdiff isn't reliable, it has dataloss issues so it
doesn't really count there.).
I'd be ok to do a quick r+ if interdiff was working well.



-Olli



bholley


On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor  wrote:


(please direct followups to dev-planning, cross-posting to governance,
firefox-dev, dev-platform)


Nearly 19 years after the creation of the Mozilla Project, commit access
remains essentially the same as it has always been.  We've evolved the
vouching process a number of times, CVS has long since been replaced by
Mercurial & others, and we've taken some positive steps in terms of
securing the commit process.  And yet we've never touched the core idea of
granting developers direct commit access to our most important
repositories.  After a large number of discussions since taking ownership
over commit policy, I believe it is time for Mozilla to change that
practice.

Before I get into the meat of the current proposal, I would like to outline
a set of key goals for any change we make.  These goals have been informed
by a set of stakeholders from across the project including the engineering,
security, release and QA teams.  It's inevitable that any significant
change will disrupt longstanding workflows.  As a result, it is critical
that we are all aligned on the goals of the change.


I've identified the following goals as critical for a responsible commit
access policy:


- Compromising a single individual's credentials must not be sufficient
to land malicious code into our products.
- Two-factor auth must be a requirement for all users approving or
pushing a change.
- The change that gets pushed must be the same change that was approved.
- Broken commits must be rejected automatically as a part of the commit
process.


In order to achieve these goals, I propose that we commit to making the
following changes to all Firefox product repositories:


- Direct commit access to repositories will be strictly limited to
sheriffs and a subset of release engineering.
   - Any direct commits by these individuals will be limited to fixing
   bustage that automation misses and handling branch merges.
- All other changes will go through an autoland-based workflow.
   - Developers commit to a staging repository, with scripting that
   connects the changeset to a Bugzilla attachment, and integrates
with review
   flags.
   - Reviewers and any other approvers interact with the changeset as
   today (including ReviewBoard if preferred), with Bugzilla flags as
the
   canonical source of truth.
   - Upon approval, the changeset will be pushed into autoland.
   - If the push is successful, the change is merged to mozilla-central,
   and the bug updated.

I know this is a major change in practice from how we currently operate,
and my ask is that we work together to understand the impact and concerns.
If you find yourself disagreeing with the goals, let's have that discussion
instead of arguing about the solution.  If you agree with the goals, but
not the solution, I'd love to hear alternative ideas for how we can achieve
the outcomes outlined above.

-- Mike
___
dev-planning mailing list
dev-plann...@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-planning



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Eric Rescorla]

On Mon, Mar 13, 2017 at 12:22 AM, Frederik Braun  wrote:

> On 12.03.2017 04:08, Cameron Kaiser wrote:
> > On 3/10/17 4:38 AM, Masatoshi Kimura wrote:
> >> On 2017/03/10 6:53, Mike Connor wrote:
> >>> - Two-factor auth must be a requirement for all users approving or
> >>> pushing a change.
> >>
> >> I have no mobile devices. How can I use 2FA?
> >>
> >> Previously I was suggested to buy a new PC and SSD only to shorten the
> >> build time. Now do I have to buy a smartphone only to contribute
> >> Mozilla? What's the next?
> >
> > I actually use a Perl script to do HOTP. And there are things like
> > Firekey. No mobile device necessary.
>
> Doesn't that defeat the point of a second factor?
>

Not entirely, no, because it is not replayable.

-Ekr


>
> >
> > Cameron Kaiser
> > tier-3-2-1-contact!
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Frederik Braun]

On 12.03.2017 04:08, Cameron Kaiser wrote:
> On 3/10/17 4:38 AM, Masatoshi Kimura wrote:
>> On 2017/03/10 6:53, Mike Connor wrote:
>>> - Two-factor auth must be a requirement for all users approving or
>>> pushing a change.
>>
>> I have no mobile devices. How can I use 2FA?
>>
>> Previously I was suggested to buy a new PC and SSD only to shorten the
>> build time. Now do I have to buy a smartphone only to contribute
>> Mozilla? What's the next?
> 
> I actually use a Perl script to do HOTP. And there are things like
> Firekey. No mobile device necessary.

Doesn't that defeat the point of a second factor?

> 
> Cameron Kaiser
> tier-3-2-1-contact!
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[smaug]

On 03/12/2017 10:40 PM, Ehsan Akhgari wrote:

On 2017-03-11 9:23 AM, smaug via governance wrote:

On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:

On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
governa...@lists.mozilla.org> wrote:


I'd be ok to do a quick r+ if interdiff was working well.


Depending on the relative timezones of the reviewer and reviewee, that
could delay landing by 24 hours or even a whole weekend.


The final r+, if it is just cosmetic changes wouldn't need to be done by
the same reviewer.


OK, but what is the exact time zone efficient way to ensure that no huge
amount of delay is added for the turn around of this final review round?

Let's be realistic.  In practice, adding one extra person in the process
of landing of the patches that currently land with r+-with-nits *will*
slow them down.  And we should expect that it will slow them down by
factors such as time zones, people missing bugmail, etc,


Why we need to expect that? In my mind the process would be closer to
"I want to land this patch, asking ok-to-land on irc." And then someone who has 
the rights to say ok to that would
mark the patch after seeing that the interdiff doesn't add anything inherently 
bad.
(well working interdiff and tooling in general is rather critical)



all of the
reasons why currently one review can end up taking a week,

I see this being _very_ different to normal reviews.

I do understand that people don't want extra process. Overhead is always 
overhead.
But the overhead here might not be as bad as feared.


it may end up
being that final round of review after this proposed change.

And I still don't understand what the proposal means with rebases in
practice.  What if, after automation tries to land your change after you
got your final r+ the final rebase fails and you need to do a manual
rebase?  Do you need to get another r+?  What happens if you're touching
one of our giant popular headers and someone else manages to land a
conflict while your reviewer gets back to you?


Perhaps we shouldn't even call the last step a review. It would be
"ok-to-land".
r+ without asking any changes would implicitly contain that "ok-to-land".
(if rebasing causes some changes, that would then need explicit
"ok-to-land")


Are you suggesting a change to the nature of the review process for the
last step with the rename?


The last step here would be quite different to normal reviews. It is just a 
rather quick glance-over that no
obviously evil code is about to land. (as I said, well working interdiff is 
absolutely critical to make this working)

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Ehsan Akhgari]

On 2017-03-11 9:23 AM, smaug via governance wrote:
> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
>> On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
>> governa...@lists.mozilla.org> wrote:
>>>
>>> I'd be ok to do a quick r+ if interdiff was working well.
>>
>> Depending on the relative timezones of the reviewer and reviewee, that
>> could delay landing by 24 hours or even a whole weekend.
>>
> The final r+, if it is just cosmetic changes wouldn't need to be done by
> the same reviewer.

OK, but what is the exact time zone efficient way to ensure that no huge
amount of delay is added for the turn around of this final review round?

Let's be realistic.  In practice, adding one extra person in the process
of landing of the patches that currently land with r+-with-nits *will*
slow them down.  And we should expect that it will slow them down by
factors such as time zones, people missing bugmail, etc, all of the
reasons why currently one review can end up taking a week, it may end up
being that final round of review after this proposed change.

And I still don't understand what the proposal means with rebases in
practice.  What if, after automation tries to land your change after you
got your final r+ the final rebase fails and you need to do a manual
rebase?  Do you need to get another r+?  What happens if you're touching
one of our giant popular headers and someone else manages to land a
conflict while your reviewer gets back to you?

> Perhaps we shouldn't even call the last step a review. It would be
> "ok-to-land".
> r+ without asking any changes would implicitly contain that "ok-to-land".
> (if rebasing causes some changes, that would then need explicit
> "ok-to-land")

Are you suggesting a change to the nature of the review process for the
last step with the rename?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Eric Rescorla]

On Fri, Mar 10, 2017 at 9:03 PM, L. David Baron  wrote:

> On Friday 2017-03-10 19:33 -0800, Eric Rescorla wrote:
> > We have been using Phabricator for our reviews in NSS and its interdiffs
> > work pretty well
> > (modulo rebases, which are not so great), and it's very easy to handle
> LGTM
> > with
> > nits and verify the nits.
>
> For what it's worth, I think proper interdiffs have two columns of
> [ -+] at the beginning of each line, not one column like diffs do.
> I've gotten used to reading interdiffs as diff -u of a diff -u, and
> while it takes a little getting used to, but once you're used to it,
> it actually represents what an interdiff is and is quite usable.  I
> think anything that pretends that something like this:
>
>   // Frame has a LayerActivityProperty property
>   FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY)
>
> + // Frame owns anonymous boxes whose style contexts it will need to
> update during
> + // a stylo tree traversal.
> + FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES)
> +
>  +// If this bit is set, then reflow may be dispatched from the current
>  +// frame instead of the root frame.
> -+FRAME_STATE_BIT(Generic, 55, NS_FRAME_DYNAMIC_REFLOW_ROOT)
> ++FRAME_STATE_BIT(Generic, 56, NS_FRAME_DYNAMIC_REFLOW_ROOT)
>  +
>   // Set for all descendants of MathML sub/supscript elements (other than
> the
>   // base frame) to indicate that the SSTY font feature should be used.
>   FRAME_STATE_BIT(Generic, 58, NS_FRAME_MATHML_SCRIPT_DESCENDANT)
>
> can be represented with only one column of [+- ] at the beginning
> is going to fail for some substantial set of important cases (such
> as rebases, as you mention).
>

The systems I am most familiar with (Phabricator, Rietveld), present
interdiffs
by allowing you to look at the diff between any revision of the CL
(including
the base revision that's the code as-is). This works pretty well for
anything other
than rebases (and is actually rather equivalent to the UI you get with
Github when people update PRs by pushing new commits onto the branch).

What sort of UI you want for rebases seems like a bit more of an open
question. i can imagine several things:

1. For simple rebases (where there's not much interaction with the
surrounding
code, you probably just want to see the patch in context as if the rebase
hadn't
happened.
2. For more complicated rebases (where there is real interaction with the
code that was rebased onto), you want to see the difference between
base1 + CL1 and base2 + CL2, but with the diffs that are due to the
rebase distinguished from the ones that are due to the CL1-CL2
difference (this what Phabricator does by marking the rebase artifacts
as lighter).
3. The design you suggest (which, at least for me, seems like it's
harder to read than designs where the tools provide more support).

It seems like designs here are evolving and it's probably going to be a
matter of personal taste, at least for a while

-Ekr


> (That's a piece of interdiff from rebasing my own patch queue
> earlier this week.)
>
> -David
>
> --
> 턞   L. David Baron http://dbaron.org/   턂
> 턢   Mozilla  https://www.mozilla.org/   턂
>  Before I built a wall I'd ask to know
>  What I was walling in or walling out,
>  And to whom I was like to give offense.
>- Robert Frost, Mending Wall (1914)
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Cameron Kaiser]

On 3/10/17 4:38 AM, Masatoshi Kimura wrote:

On 2017/03/10 6:53, Mike Connor wrote:

- Two-factor auth must be a requirement for all users approving or
pushing a change.


I have no mobile devices. How can I use 2FA?

Previously I was suggested to buy a new PC and SSD only to shorten the
build time. Now do I have to buy a smartphone only to contribute
Mozilla? What's the next?


I actually use a Perl script to do HOTP. And there are things like 
Firekey. No mobile device necessary.


Cameron Kaiser
tier-3-2-1-contact!
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[gsquelart]

On Sunday, March 12, 2017 at 1:23:45 AM UTC+11, smaug wrote:
> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
> > On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
> > gov...@lists.mozilla.org> wrote:
> >>
> >> I'd be ok to do a quick r+ if interdiff was working well.
> >
> > Depending on the relative timezones of the reviewer and reviewee, that
> > could delay landing by 24 hours or even a whole weekend.
> >
> The final r+, if it is just cosmetic changes wouldn't need to be done by the 
> same reviewer.
> 
> Perhaps we shouldn't even call the last step a review. It would be 
> "ok-to-land".
> r+ without asking any changes would implicitly contain that "ok-to-land".
> (if rebasing causes some changes, that would then need explicit "ok-to-land")
> 
> 
> 
> 
> > In general there seems to be a large amount of support in this thread for
> > continuing to allow the r+-with-minor-fixes option.
> 
> Yeah. I don't object that, but I also think that having final approval to 
> land the patch might not really be that bad
> (assuming the tools are working well enough).
> 
> >
> > Nick

If we really want to enforce a final review to prevent unwanted code to land, 
Mozilla (as a whole) will incur some costs: Reviewers spending time 
re-reviewing; delays to land (worse across tz, and which could result in the 
need to rebase again, and therefore another round of ok-to-land); and mounting 
anger at all these papercuts.

So if Mozilla is really committed to this solution, and is ready to bear the 
associated financial costs, I would suggest we recruit people who would do just 
these tasks! Could be existing sheriffs, and/or volunteering peers, and/or new 
staff with distinct job descriptions.

Hopefully they'd rubber-stamp 99% of last-minute changes, and only the more 
complex changes would be referred back to the author and reviewer.
As a bonus they could also handle trivial autoland merge issues.

In the end it should cost a similar amount of money, but would lessen the other 
costs (delays and frustration).


And while I've got the mic, a small suggestion for mozreview to help with some 
of this: We need a way for the author to add a comment explaining what was done 
between pushes (rebase, nit-fixing, other notes to reviewer, etc.); this 
comment would only appear in Bugzilla and mozreview, not in the actual patch 
commit description.
(Could be a paragraph starting with "notes-to-reviewer:", to be removed before 
autolanding.)

- Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[smaug]

On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:

On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
governa...@lists.mozilla.org> wrote:


I'd be ok to do a quick r+ if interdiff was working well.


Depending on the relative timezones of the reviewer and reviewee, that
could delay landing by 24 hours or even a whole weekend.


The final r+, if it is just cosmetic changes wouldn't need to be done by the 
same reviewer.

Perhaps we shouldn't even call the last step a review. It would be "ok-to-land".
r+ without asking any changes would implicitly contain that "ok-to-land".
(if rebasing causes some changes, that would then need explicit "ok-to-land")





In general there seems to be a large amount of support in this thread for
continuing to allow the r+-with-minor-fixes option.


Yeah. I don't object that, but I also think that having final approval to land 
the patch might not really be that bad
(assuming the tools are working well enough).



Nick



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Gabor Krizsanits]

On Sat, Mar 11, 2017 at 7:23 AM, Nicholas Nethercote  wrote:

>
> Depending on the relative timezones of the reviewer and reviewee, that
> could delay landing by 24 hours or even a whole weekend.
>
>
As someone working from Europe and 90% of the time with people from the
West Coast, thank
you very much for bringing up the timezone argument. r+-with-minor-fixes is
an absolute must for
some of us.

Gabor
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Nicholas Nethercote]

On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
governa...@lists.mozilla.org> wrote:
>
> I'd be ok to do a quick r+ if interdiff was working well.

Depending on the relative timezones of the reviewer and reviewee, that
could delay landing by 24 hours or even a whole weekend.

In general there seems to be a large amount of support in this thread for
continuing to allow the r+-with-minor-fixes option.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[L. David Baron]

On Friday 2017-03-10 19:33 -0800, Eric Rescorla wrote:
> We have been using Phabricator for our reviews in NSS and its interdiffs
> work pretty well
> (modulo rebases, which are not so great), and it's very easy to handle LGTM
> with
> nits and verify the nits.

For what it's worth, I think proper interdiffs have two columns of
[ -+] at the beginning of each line, not one column like diffs do.
I've gotten used to reading interdiffs as diff -u of a diff -u, and
while it takes a little getting used to, but once you're used to it,
it actually represents what an interdiff is and is quite usable.  I
think anything that pretends that something like this:

  // Frame has a LayerActivityProperty property
  FRAME_STATE_BIT(Generic, 54, NS_FRAME_HAS_LAYER_ACTIVITY_PROPERTY)
  
+ // Frame owns anonymous boxes whose style contexts it will need to update 
during
+ // a stylo tree traversal.
+ FRAME_STATE_BIT(Generic, 55, NS_FRAME_OWNS_ANON_BOXES)
+ 
 +// If this bit is set, then reflow may be dispatched from the current
 +// frame instead of the root frame.
-+FRAME_STATE_BIT(Generic, 55, NS_FRAME_DYNAMIC_REFLOW_ROOT)
++FRAME_STATE_BIT(Generic, 56, NS_FRAME_DYNAMIC_REFLOW_ROOT)
 +
  // Set for all descendants of MathML sub/supscript elements (other than the
  // base frame) to indicate that the SSTY font feature should be used.
  FRAME_STATE_BIT(Generic, 58, NS_FRAME_MATHML_SCRIPT_DESCENDANT)

can be represented with only one column of [+- ] at the beginning
is going to fail for some substantial set of important cases (such
as rebases, as you mention).

(That's a piece of interdiff from rebasing my own patch queue
earlier this week.)

-David

-- 
턞   L. David Baron http://dbaron.org/   턂
턢   Mozilla  https://www.mozilla.org/   턂
 Before I built a wall I'd ask to know
 What I was walling in or walling out,
 And to whom I was like to give offense.
   - Robert Frost, Mending Wall (1914)


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Eric Rescorla]

On Fri, Mar 10, 2017 at 7:23 PM, smaug via governance <
governa...@lists.mozilla.org> wrote:

> On 03/10/2017 12:59 AM, Bobby Holley wrote:
>
>> At a high level, I think the goals here are good.
>>
>> However, the tooling here needs to be top-notch for this to work, and the
>> standard approach of flipping on an MVP and dealing with the rest in
>> followup bugs isn't going to be acceptable for something so critical to
>> our
>> productivity as landing code. The only reason more developers aren't
>> complaining about the MozReview+autoland workflow right now is that it's
>> optional.
>>
>> The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
>> not to pay attention to new workflows because they have too much else on
>> their plate. The onus needs to be on the team deploying this to have the
>> highest-volume committers using the new system happily and voluntarily
>> before it becomes mandatory. That probably means that the team should not
>> have any deadline-oriented incentives to ship it before it's ready.
>>
>> Also, getting rid of "r+ with comments" is a non-starter.
>>
>
> FWIW, with my reviewer hat on, I think that is not true, _assuming_ there
> is a reliable interdiff for patches.
> And not only MozReview patches but for all the patches. (and MozReview
> interdiff isn't reliable, it has dataloss issues so it
> doesn't really count there.).
> I'd be ok to do a quick r+ if interdiff was working well.
>

Without taking a position on the broader proposal, I agree with this.

We have been using Phabricator for our reviews in NSS and its interdiffs
work pretty well
(modulo rebases, which are not so great), and it's very easy to handle LGTM
with
nits and verify the nits.

-Ekr


>
>
>
> -Olli
>
>
>> bholley
>>
>>
>> On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor  wrote:
>>
>> (please direct followups to dev-planning, cross-posting to governance,
>>> firefox-dev, dev-platform)
>>>
>>>
>>> Nearly 19 years after the creation of the Mozilla Project, commit access
>>> remains essentially the same as it has always been.  We've evolved the
>>> vouching process a number of times, CVS has long since been replaced by
>>> Mercurial & others, and we've taken some positive steps in terms of
>>> securing the commit process.  And yet we've never touched the core idea
>>> of
>>> granting developers direct commit access to our most important
>>> repositories.  After a large number of discussions since taking ownership
>>> over commit policy, I believe it is time for Mozilla to change that
>>> practice.
>>>
>>> Before I get into the meat of the current proposal, I would like to
>>> outline
>>> a set of key goals for any change we make.  These goals have been
>>> informed
>>> by a set of stakeholders from across the project including the
>>> engineering,
>>> security, release and QA teams.  It's inevitable that any significant
>>> change will disrupt longstanding workflows.  As a result, it is critical
>>> that we are all aligned on the goals of the change.
>>>
>>>
>>> I've identified the following goals as critical for a responsible commit
>>> access policy:
>>>
>>>
>>> - Compromising a single individual's credentials must not be
>>> sufficient
>>> to land malicious code into our products.
>>> - Two-factor auth must be a requirement for all users approving or
>>> pushing a change.
>>> - The change that gets pushed must be the same change that was
>>> approved.
>>> - Broken commits must be rejected automatically as a part of the
>>> commit
>>> process.
>>>
>>>
>>> In order to achieve these goals, I propose that we commit to making the
>>> following changes to all Firefox product repositories:
>>>
>>>
>>> - Direct commit access to repositories will be strictly limited to
>>> sheriffs and a subset of release engineering.
>>>- Any direct commits by these individuals will be limited to
>>> fixing
>>>bustage that automation misses and handling branch merges.
>>> - All other changes will go through an autoland-based workflow.
>>>- Developers commit to a staging repository, with scripting that
>>>connects the changeset to a Bugzilla attachment, and integrates
>>> with review
>>>flags.
>>>- Reviewers and any other approvers interact with the changeset as
>>>today (including ReviewBoard if preferred), with Bugzilla flags as
>>> the
>>>canonical source of truth.
>>>- Upon approval, the changeset will be pushed into autoland.
>>>- If the push is successful, the change is merged to
>>> mozilla-central,
>>>and the bug updated.
>>>
>>> I know this is a major change in practice from how we currently operate,
>>> and my ask is that we work together to understand the impact and
>>> concerns.
>>> If you find yourself disagreeing with the goals, let's have that
>>> discussion
>>> instead of arguing about the solution.  If you agree with the goals, but
>>> not the solution, I'd 

Re: The future of commit access policy for core Firefox

[Frank-Rainer Grahl]

Ehsan Akhgari wrote:

Even with a single reviewer, I often times end up making some trivial
changes to my patches to fix stupid mistakes and issues that I know the
reviewer doesn't care enough to want to look at before landing.  In
general our code review process has a lot of flexibility built into it,
and reviewers generally understand that the goal ultimately is to ensure
the quality of the produced code, so depending on the circumstances as a
reviewer I can treat a patch on different levels of scrutiny, from
anywhere between checking the actual landed patch and complaining if
something wasn't done in the way I asked to r+ing asking for a lot of
changes and trusting the author will do the right thing without needing
me look over their work more.

...

Same here. Automation is fine if everything goes according to plan but pushing 
manually is much less time consuming if something goes wrong e.g. a patch 
needs trivial changes to un-bitrot it. So there should still be a way to just 
push manually if needed or desired.


FRG
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Frank-Rainer Grahl]

Me too. Have a phone which does what I need (calling people and be called:) ) 
No need for a mobile device which has its own security and usability problems.


FRG

Masatoshi Kimura wrote:

On 2017/03/10 6:53, Mike Connor wrote:

- Two-factor auth must be a requirement for all users approving or
pushing a change.


I have no mobile devices. How can I use 2FA?

Previously I was suggested to buy a new PC and SSD only to shorten the
build time. Now do I have to buy a smartphone only to contribute
Mozilla? What's the next?




___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[jwood]

On Friday, March 10, 2017 at 7:38:57 AM UTC-5, Masatoshi Kimura wrote:
> On 2017/03/10 6:53, Mike Connor wrote:
> >- Two-factor auth must be a requirement for all users approving or
> >pushing a change.
> 
> I have no mobile devices. How can I use 2FA?
> 
> Previously I was suggested to buy a new PC and SSD only to shorten the
> build time. Now do I have to buy a smartphone only to contribute
> Mozilla? What's the next?

Fwiw, Yubikey's are generally suitable 2FA devices, and are significantly 
cheaper than smartphones. I'd not be surprised if key contributors who need 
commit rights were able to apply to mozilla in order to acquire a yubikey 
should they be unable to procure on themselves. (I'm not a policy maker, so do 
NOT take my statement as any sort of promise)

~Justin Wood (Callek)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Masatoshi Kimura]

On 2017/03/10 14:00, Mike Hommey wrote:
> While we're talking about drag on productivity, there's already one that
> comes from autoland already, which is that you can't easily land fixups
> for stupid mistakes (like, a build failure on one platform, or other
> lame things that happen when things land).
> 
> You either have to get a sheriff to land the fixup for you on autoland,
> or to back you out, in which case you're back to square one and need to
> reland the whole thing.
> 
> If on top of that, you also need another round of reviews...

Yes, it is really frustrating and wasting bandwidth of both developers
and shriffs. It is one of reasons I dislike mozreview/autoland.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Masatoshi Kimura]

On 2017/03/10 6:53, Mike Connor wrote:
>- Two-factor auth must be a requirement for all users approving or
>pushing a change.

I have no mobile devices. How can I use 2FA?

Previously I was suggested to buy a new PC and SSD only to shorten the
build time. Now do I have to buy a smartphone only to contribute
Mozilla? What's the next?

-- 
vyv03...@nifty.ne.jp
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Mike Hommey]

On Thu, Mar 09, 2017 at 08:25:17PM -0800, zbranie...@mozilla.com wrote:
> As others stated, the idea that patch cannot be altered after r+ has a 
> massive effect on productivity. I can't overstate how much it would impact 
> day-to-day work for engineers, and I don't really see an easy way out.
> 
> Even if we added "approval to land with minor changes" there's a) no way to 
> distinguish minor fro major, and b) reviewers will either start using it as a 
> default, or keep forgetting about it.
> 
> I like the direction, but I honestly believe that this single idea would make 
> working with Gecko a massive PITA.
> With autoland my path to central from when I get all the required reviews is 
> already ~24h because I push the "land" button around 2pm PST and it gets 
> merged into central around 3am, so I can only follow-up the next day.
> 
> I recently introduced a regression not caught by me, my reviewer or tests. It 
> wasn't major enough to warrant panic mode, but I'm sure it irritated people 
> with spawned warnings and of course it has some impact on our nightly users.
> I landed the follow up within 20 minutes of discovering the bug, but since it 
> wen't through autoland, it took two nightly builds and a full day before 
> users stopped reporting dups of the bug.
> 
> Now, if you add to that, that every minor change I make after my reviewer 
> approved my patch I need to get a re-review (and most of my reviewers are in 
> a different timezone), it'll basically at the very best add just another 24h 
> to the cycle.
> If it's Friday, or my reviewer is busy with other stuff or on PTO, it'll add 
> a couple days.

While we're talking about drag on productivity, there's already one that
comes from autoland already, which is that you can't easily land fixups
for stupid mistakes (like, a build failure on one platform, or other
lame things that happen when things land).

You either have to get a sheriff to land the fixup for you on autoland,
or to back you out, in which case you're back to square one and need to
reland the whole thing.

If on top of that, you also need another round of reviews...

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[zbraniecki]

As others stated, the idea that patch cannot be altered after r+ has a massive 
effect on productivity. I can't overstate how much it would impact day-to-day 
work for engineers, and I don't really see an easy way out.

Even if we added "approval to land with minor changes" there's a) no way to 
distinguish minor fro major, and b) reviewers will either start using it as a 
default, or keep forgetting about it.

I like the direction, but I honestly believe that this single idea would make 
working with Gecko a massive PITA.
With autoland my path to central from when I get all the required reviews is 
already ~24h because I push the "land" button around 2pm PST and it gets merged 
into central around 3am, so I can only follow-up the next day.

I recently introduced a regression not caught by me, my reviewer or tests. It 
wasn't major enough to warrant panic mode, but I'm sure it irritated people 
with spawned warnings and of course it has some impact on our nightly users.
I landed the follow up within 20 minutes of discovering the bug, but since it 
wen't through autoland, it took two nightly builds and a full day before users 
stopped reporting dups of the bug.

Now, if you add to that, that every minor change I make after my reviewer 
approved my patch I need to get a re-review (and most of my reviewers are in a 
different timezone), it'll basically at the very best add just another 24h to 
the cycle.
If it's Friday, or my reviewer is busy with other stuff or on PTO, it'll add a 
couple days.

zb.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Edmund Wong]

Mike Connor wrote:
> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
> 
> 
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
> 
> Before I get into the meat of the current proposal, I would like to outline
> a set of key goals for any change we make.  These goals have been informed
> by a set of stakeholders from across the project including the engineering,
> security, release and QA teams.  It's inevitable that any significant
> change will disrupt longstanding workflows.  As a result, it is critical
> that we are all aligned on the goals of the change.
> 
> 
> I've identified the following goals as critical for a responsible commit
> access policy:
> 
> 
>- Compromising a single individual's credentials must not be sufficient
>to land malicious code into our products.
>- Two-factor auth must be a requirement for all users approving or
>pushing a change.
>- The change that gets pushed must be the same change that was approved.
>- Broken commits must be rejected automatically as a part of the commit
>process.
> 
> 

Aside for the first one, the other items seem to be mere
'implementation/application details', rather than actual goals.

 - Protect Firefox repositories to the best of our abilities and
resources availability by implementing a set of rules and factors
to prevent unauthorized access/modifications to the core content.


> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:

By all Firefox product repositories, I'm assuming mainly m-*,
and integration/m-i?

And with this new proposed process, this means the doing away
of integration/m-i as it would be superfluous if an autoland-esque
repo is used.

What about other repositories?  Tier 2, etc..  i.e. comm-*?

> 
> 
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.

>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates
> with review
>   flags.

So m-i is obsoleted.  Autoland is the defacto push repo?

1) Developer submits a patch to bugzilla or reviewboard
2) it gets reviewed and/or approved
3) it then gets pushed to autoland [tbh, I don't know how autoland
works, so does someone push to autoland, or does bugzilla do it
for us? -rhetorical question.. can find out off-list]
4) sheriffs watch the autoland tree and backout whatever bustages
   happen and every so often, they merge autoland to m-c.  If
   patches need to be uplifted, they are done by sheriffs.

So we're pretty much back to the similar vein of m-* and
mozilla-inbound (except now, it's autoland).

Is this the gist of it?

Edmund
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[danderson]

On Thursday, March 9, 2017 at 1:53:50 PM UTC-8, Mike Connor wrote:
> 
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.
>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates
> with review
>   flags.
>   - Reviewers and any other approvers interact with the changeset as
>   today (including ReviewBoard if preferred), with Bugzilla flags as the
>   canonical source of truth.
>   - Upon approval, the changeset will be pushed into autoland.
>   - If the push is successful, the change is merged to mozilla-central,
>   and the bug updated.

Requiring secure accounts, and not being able to land bustage, sound like good 
goals. But these proposals sound like a lot of serious productivity-hampering 
restrictions. It's not explained what actual problems are being solved, or why 
our current system fails to meet these goals and needs drastic changes.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Ehsan Akhgari]

On 2017-03-09 6:51 PM, Justin Dolske wrote:
> Similar to "r+ with fixes" are cases where a patch (or patch series)
> requires reviews from a multitude of people. Which means variable delays in
> getting reviews, during which things may slightly bitrot or be trivially
> modified based on feedback from other reviewers. Code refactorings are one
> example of this. Having to re-request review from everyone, perhaps
> multiple times, would seem pretty painful. :)

Even with a single reviewer, I often times end up making some trivial
changes to my patches to fix stupid mistakes and issues that I know the
reviewer doesn't care enough to want to look at before landing.  In
general our code review process has a lot of flexibility built into it,
and reviewers generally understand that the goal ultimately is to ensure
the quality of the produced code, so depending on the circumstances as a
reviewer I can treat a patch on different levels of scrutiny, from
anywhere between checking the actual landed patch and complaining if
something wasn't done in the way I asked to r+ing asking for a lot of
changes and trusting the author will do the right thing without needing
me look over their work more.

> From previous discussions, it sounded like there was a middle ground in
> still requiring all landings to go through automation (i.e., a strong link
> between what actually landed and what was posted in the bug),

Which, to some extent, goes against the flexibility of an author when
they're trusted like in the above scenario.  :(  This is pretty
disrupting to my personal productivity, for whatever that's worth (but
I'll of course deal with it if I have to).  I just don't understand why
this is desirable as a goal.

> but allow
> some slop in what was formally reviewed (i.e., a weak link between last
> review and what's was asked to land.) The automation might at least require
> that you can't land a "r=sparky" unless "sparky" did at some point grant
> r+. At the very least, this makes it somewhat easier to compare what landed
> with what was last reviewed (with current tools, thanks to ReviewBoard's
> revision diffing).

FTR we have real actual experience with this: for years I have had to
append a=me etc to work around various hooks.  I don't understand how
something in the commit message can help here, if for example the
requirement is for the reviewer to have to r+ the thing that gets
autolanded every time.

> I suppose this policy also implies no more "Typo fix, no bug" landings.

That is also unacceptable.

> But
> I never really liked those anyway, so I'm fine with that. Bugs are cheap! :)
> 
> Justin
> 
> On Thu, Mar 9, 2017 at 3:11 PM,  wrote:
> 
>> On Friday, March 10, 2017 at 10:53:50 AM UTC+13, Mike Connor wrote:
>>> (please direct followups to dev-planning, cross-posting to governance,
>>> firefox-dev, dev-platform)
>>>
>>>
>>> Nearly 19 years after the creation of the Mozilla Project, commit access
>>> remains essentially the same as it has always been.  We've evolved the
>>> vouching process a number of times, CVS has long since been replaced by
>>> Mercurial & others, and we've taken some positive steps in terms of
>>> securing the commit process.  And yet we've never touched the core idea
>> of
>>> granting developers direct commit access to our most important
>>> repositories.  After a large number of discussions since taking ownership
>>> over commit policy, I believe it is time for Mozilla to change that
>>> practice.
>>>
>>> Before I get into the meat of the current proposal, I would like to
>> outline
>>> a set of key goals for any change we make.  These goals have been
>> informed
>>> by a set of stakeholders from across the project including the
>> engineering,
>>> security, release and QA teams.  It's inevitable that any significant
>>> change will disrupt longstanding workflows.  As a result, it is critical
>>> that we are all aligned on the goals of the change.
>>>
>>>
>>> I've identified the following goals as critical for a responsible commit
>>> access policy:
>>>
>>>
>>>- Compromising a single individual's credentials must not be
>> sufficient
>>>to land malicious code into our products.
>>>- Two-factor auth must be a requirement for all users approving or
>>>pushing a change.
>>>- The change that gets pushed must be the same change that was
>> approved.
>>>- Broken commits must be rejected automatically as a part of the
>> commit
>>>process.
>>>
>>>
>>> In order to achieve these goals, I propose that we commit to making the
>>> following changes to all Firefox product repositories:
>>>
>>>
>>>- Direct commit access to repositories will be strictly limited to
>>>sheriffs and a subset of release engineering.
>>>   - Any direct commits by these individuals will be limited to fixing
>>>   bustage that automation misses and handling branch merges.
>>>- All other changes will go through an autoland-based workflow.
>>>   - 

Re: The future of commit access policy for core Firefox

[Mike Hommey]

On Thu, Mar 09, 2017 at 03:51:07PM -0800, Justin Dolske wrote:
> I suppose this policy also implies no more "Typo fix, no bug" landings. But
> I never really liked those anyway, so I'm fine with that. Bugs are cheap! :)

Considering the overhead of creating a bug, attaching a patch,
requesting a review, landing, vs. just landing typo fixes, it's hard to
say bugs are cheap. (plus, you now have 2 persons involved instead of
one)

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Justin Dolske]

Similar to "r+ with fixes" are cases where a patch (or patch series)
requires reviews from a multitude of people. Which means variable delays in
getting reviews, during which things may slightly bitrot or be trivially
modified based on feedback from other reviewers. Code refactorings are one
example of this. Having to re-request review from everyone, perhaps
multiple times, would seem pretty painful. :)

>From previous discussions, it sounded like there was a middle ground in
still requiring all landings to go through automation (i.e., a strong link
between what actually landed and what was posted in the bug), but allow
some slop in what was formally reviewed (i.e., a weak link between last
review and what's was asked to land.) The automation might at least require
that you can't land a "r=sparky" unless "sparky" did at some point grant
r+. At the very least, this makes it somewhat easier to compare what landed
with what was last reviewed (with current tools, thanks to ReviewBoard's
revision diffing).

I suppose this policy also implies no more "Typo fix, no bug" landings. But
I never really liked those anyway, so I'm fine with that. Bugs are cheap! :)

Justin

On Thu, Mar 9, 2017 at 3:11 PM,  wrote:

> On Friday, March 10, 2017 at 10:53:50 AM UTC+13, Mike Connor wrote:
> > (please direct followups to dev-planning, cross-posting to governance,
> > firefox-dev, dev-platform)
> >
> >
> > Nearly 19 years after the creation of the Mozilla Project, commit access
> > remains essentially the same as it has always been.  We've evolved the
> > vouching process a number of times, CVS has long since been replaced by
> > Mercurial & others, and we've taken some positive steps in terms of
> > securing the commit process.  And yet we've never touched the core idea
> of
> > granting developers direct commit access to our most important
> > repositories.  After a large number of discussions since taking ownership
> > over commit policy, I believe it is time for Mozilla to change that
> > practice.
> >
> > Before I get into the meat of the current proposal, I would like to
> outline
> > a set of key goals for any change we make.  These goals have been
> informed
> > by a set of stakeholders from across the project including the
> engineering,
> > security, release and QA teams.  It's inevitable that any significant
> > change will disrupt longstanding workflows.  As a result, it is critical
> > that we are all aligned on the goals of the change.
> >
> >
> > I've identified the following goals as critical for a responsible commit
> > access policy:
> >
> >
> >- Compromising a single individual's credentials must not be
> sufficient
> >to land malicious code into our products.
> >- Two-factor auth must be a requirement for all users approving or
> >pushing a change.
> >- The change that gets pushed must be the same change that was
> approved.
> >- Broken commits must be rejected automatically as a part of the
> commit
> >process.
> >
> >
> > In order to achieve these goals, I propose that we commit to making the
> > following changes to all Firefox product repositories:
> >
> >
> >- Direct commit access to repositories will be strictly limited to
> >sheriffs and a subset of release engineering.
> >   - Any direct commits by these individuals will be limited to fixing
> >   bustage that automation misses and handling branch merges.
> >- All other changes will go through an autoland-based workflow.
> >   - Developers commit to a staging repository, with scripting that
> >   connects the changeset to a Bugzilla attachment, and integrates
> > with review
> >   flags.
> >   - Reviewers and any other approvers interact with the changeset as
> >   today (including ReviewBoard if preferred), with Bugzilla flags as
> the
> >   canonical source of truth.
> >   - Upon approval, the changeset will be pushed into autoland.
> >   - If the push is successful, the change is merged to
> mozilla-central,
> >   and the bug updated.
> >
> > I know this is a major change in practice from how we currently operate,
> > and my ask is that we work together to understand the impact and
> concerns.
> > If you find yourself disagreeing with the goals, let's have that
> discussion
> > instead of arguing about the solution.  If you agree with the goals, but
> > not the solution, I'd love to hear alternative ideas for how we can
> achieve
> > the outcomes outlined above.
> >
> > -- Mike
>
>
> How will uplifts work? Can only sheriffs land them?
>
> This would do-away with "r+ with comments addressed". Reviewers typically
> only say this for patches submitted by people they trust. So removing "r+
> with comments" would mean reviewers would need to re-review code, causing
> an extra delay and extra reviewing load. Is there some way we can keep the
> "r+ with comments addressed" behaviour for trusted contributors?
>
> 

Re: The future of commit access policy for core Firefox

[Eric Rescorla]

On Thu, Mar 9, 2017 at 3:11 PM,  wrote:

> On Friday, March 10, 2017 at 10:53:50 AM UTC+13, Mike Connor wrote:
> > (please direct followups to dev-planning, cross-posting to governance,
> > firefox-dev, dev-platform)
> >
> >
> > Nearly 19 years after the creation of the Mozilla Project, commit access
> > remains essentially the same as it has always been.  We've evolved the
> > vouching process a number of times, CVS has long since been replaced by
> > Mercurial & others, and we've taken some positive steps in terms of
> > securing the commit process.  And yet we've never touched the core idea
> of
> > granting developers direct commit access to our most important
> > repositories.  After a large number of discussions since taking ownership
> > over commit policy, I believe it is time for Mozilla to change that
> > practice.
> >
> > Before I get into the meat of the current proposal, I would like to
> outline
> > a set of key goals for any change we make.  These goals have been
> informed
> > by a set of stakeholders from across the project including the
> engineering,
> > security, release and QA teams.  It's inevitable that any significant
> > change will disrupt longstanding workflows.  As a result, it is critical
> > that we are all aligned on the goals of the change.
> >
> >
> > I've identified the following goals as critical for a responsible commit
> > access policy:
> >
> >
> >- Compromising a single individual's credentials must not be
> sufficient
> >to land malicious code into our products.
> >- Two-factor auth must be a requirement for all users approving or
> >pushing a change.
> >- The change that gets pushed must be the same change that was
> approved.
> >- Broken commits must be rejected automatically as a part of the
> commit
> >process.
> >
> >
> > In order to achieve these goals, I propose that we commit to making the
> > following changes to all Firefox product repositories:
> >
> >
> >- Direct commit access to repositories will be strictly limited to
> >sheriffs and a subset of release engineering.
> >   - Any direct commits by these individuals will be limited to fixing
> >   bustage that automation misses and handling branch merges.
> >- All other changes will go through an autoland-based workflow.
> >   - Developers commit to a staging repository, with scripting that
> >   connects the changeset to a Bugzilla attachment, and integrates
> > with review
> >   flags.
> >   - Reviewers and any other approvers interact with the changeset as
> >   today (including ReviewBoard if preferred), with Bugzilla flags as
> the
> >   canonical source of truth.
> >   - Upon approval, the changeset will be pushed into autoland.
> >   - If the push is successful, the change is merged to
> mozilla-central,
> >   and the bug updated.
> >
> > I know this is a major change in practice from how we currently operate,
> > and my ask is that we work together to understand the impact and
> concerns.
> > If you find yourself disagreeing with the goals, let's have that
> discussion
> > instead of arguing about the solution.  If you agree with the goals, but
> > not the solution, I'd love to hear alternative ideas for how we can
> achieve
> > the outcomes outlined above.
> >
> > -- Mike
>
>
> How will uplifts work? Can only sheriffs land them?
>
> This would do-away with "r+ with comments addressed". Reviewers typically
> only say this for patches submitted by people they trust. So removing "r+
> with comments" would mean reviewers would need to re-review code, causing
> an extra delay and extra reviewing load. Is there some way we can keep the
> "r+ with comments addressed" behaviour for trusted contributors?
>

One potential approach would be to keep it for contributors who themselves
were L3
committers in the current framework (and met a similar bar in the future).

The way to rationalize this is that those people could always set up a sock
puppet
account, submit a patch, and then review it themselves and it would be
landed,
so the policy is ultimately "sign off by one L3 committer"

-Ekr


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

Re: The future of commit access policy for core Firefox

[chris . ryan . pearce]

On Friday, March 10, 2017 at 10:53:50 AM UTC+13, Mike Connor wrote:
> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
> 
> 
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
> 
> Before I get into the meat of the current proposal, I would like to outline
> a set of key goals for any change we make.  These goals have been informed
> by a set of stakeholders from across the project including the engineering,
> security, release and QA teams.  It's inevitable that any significant
> change will disrupt longstanding workflows.  As a result, it is critical
> that we are all aligned on the goals of the change.
> 
> 
> I've identified the following goals as critical for a responsible commit
> access policy:
> 
> 
>- Compromising a single individual's credentials must not be sufficient
>to land malicious code into our products.
>- Two-factor auth must be a requirement for all users approving or
>pushing a change.
>- The change that gets pushed must be the same change that was approved.
>- Broken commits must be rejected automatically as a part of the commit
>process.
> 
> 
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
> 
> 
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.
>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates
> with review
>   flags.
>   - Reviewers and any other approvers interact with the changeset as
>   today (including ReviewBoard if preferred), with Bugzilla flags as the
>   canonical source of truth.
>   - Upon approval, the changeset will be pushed into autoland.
>   - If the push is successful, the change is merged to mozilla-central,
>   and the bug updated.
> 
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution.  If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
> 
> -- Mike


How will uplifts work? Can only sheriffs land them?

This would do-away with "r+ with comments addressed". Reviewers typically only 
say this for patches submitted by people they trust. So removing "r+ with 
comments" would mean reviewers would need to re-review code, causing an extra 
delay and extra reviewing load. Is there some way we can keep the "r+ with 
comments addressed" behaviour for trusted contributors?

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Eric Rescorla]

First, let me state that I am generally in support of this type of change.

More comments below.

On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor  wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to
> outline a set of key goals for any change we make.  These goals have been
> informed by a set of stakeholders from across the project including the
> engineering, security, release and QA teams.  It's inevitable that any
> significant change will disrupt longstanding workflows.  As a result, it is
> critical that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>
>- Compromising a single individual's credentials must not be
>sufficient to land malicious code into our products.
>
> I think this is a good long-term goal, but I don't think it's one we need
to achieve now.
At the moment, I would settle for "only a few privileged people can
single-handedly
land malicious code into our products". In support of this, I would note
that what you
propose below is insufficient to achieve your stated objective, because
there will
still be single person admin access to machines.


>- Two-factor auth must be a requirement for all users approving or
>pushing a change.
>- The change that gets pushed must be the same change that was
>approved.
>- Broken commits must be rejected automatically as a part of the
>commit process.
>
>
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
>
>
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.
>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>
> I think this is a good eventual goal, but in the short term, I think it's
probably useful
to keep checkin-needed floating around, especially given the somewhat iffy
state
of the toolchain.


>
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates with 
> review
>   flags.
>   - Reviewers and any other approvers interact with the changeset as
>   today (including ReviewBoard if preferred), with Bugzilla flags as the
>   canonical source of truth.
>   - Upon approval, the changeset will be pushed into autoland.
>
> Implicit in this is some sort of hierarchy of reviewers (tied to what was
previous L3 commit?)
that says who can review a patch. Otherwise, I can just create a dummy
account, r+ my own
patch, and Land Ho! IIRC Chromium does this by saying that LGTM implies
autolanding
only if the reviewer could have landed the code themselves.

-Ekr



>- If the push is successful, the change is merged to mozilla-central,
>   and the bug updated.
>
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution.  If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
>
> -- Mike
>
> ___
> firefox-dev mailing list
> firefox-...@mozilla.org
> https://mail.mozilla.org/listinfo/firefox-dev
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[Bobby Holley]

At a high level, I think the goals here are good.

However, the tooling here needs to be top-notch for this to work, and the
standard approach of flipping on an MVP and dealing with the rest in
followup bugs isn't going to be acceptable for something so critical to our
productivity as landing code. The only reason more developers aren't
complaining about the MozReview+autoland workflow right now is that it's
optional.

The busiest and most-productive developers (ehsan, bz, dbaron, etc) tend
not to pay attention to new workflows because they have too much else on
their plate. The onus needs to be on the team deploying this to have the
highest-volume committers using the new system happily and voluntarily
before it becomes mandatory. That probably means that the team should not
have any deadline-oriented incentives to ship it before it's ready.

Also, getting rid of "r+ with comments" is a non-starter.

bholley


On Thu, Mar 9, 2017 at 1:53 PM, Mike Connor  wrote:

> (please direct followups to dev-planning, cross-posting to governance,
> firefox-dev, dev-platform)
>
>
> Nearly 19 years after the creation of the Mozilla Project, commit access
> remains essentially the same as it has always been.  We've evolved the
> vouching process a number of times, CVS has long since been replaced by
> Mercurial & others, and we've taken some positive steps in terms of
> securing the commit process.  And yet we've never touched the core idea of
> granting developers direct commit access to our most important
> repositories.  After a large number of discussions since taking ownership
> over commit policy, I believe it is time for Mozilla to change that
> practice.
>
> Before I get into the meat of the current proposal, I would like to outline
> a set of key goals for any change we make.  These goals have been informed
> by a set of stakeholders from across the project including the engineering,
> security, release and QA teams.  It's inevitable that any significant
> change will disrupt longstanding workflows.  As a result, it is critical
> that we are all aligned on the goals of the change.
>
>
> I've identified the following goals as critical for a responsible commit
> access policy:
>
>
>- Compromising a single individual's credentials must not be sufficient
>to land malicious code into our products.
>- Two-factor auth must be a requirement for all users approving or
>pushing a change.
>- The change that gets pushed must be the same change that was approved.
>- Broken commits must be rejected automatically as a part of the commit
>process.
>
>
> In order to achieve these goals, I propose that we commit to making the
> following changes to all Firefox product repositories:
>
>
>- Direct commit access to repositories will be strictly limited to
>sheriffs and a subset of release engineering.
>   - Any direct commits by these individuals will be limited to fixing
>   bustage that automation misses and handling branch merges.
>- All other changes will go through an autoland-based workflow.
>   - Developers commit to a staging repository, with scripting that
>   connects the changeset to a Bugzilla attachment, and integrates
> with review
>   flags.
>   - Reviewers and any other approvers interact with the changeset as
>   today (including ReviewBoard if preferred), with Bugzilla flags as
> the
>   canonical source of truth.
>   - Upon approval, the changeset will be pushed into autoland.
>   - If the push is successful, the change is merged to mozilla-central,
>   and the bug updated.
>
> I know this is a major change in practice from how we currently operate,
> and my ask is that we work together to understand the impact and concerns.
> If you find yourself disagreeing with the goals, let's have that discussion
> instead of arguing about the solution.  If you agree with the goals, but
> not the solution, I'd love to hear alternative ideas for how we can achieve
> the outcomes outlined above.
>
> -- Mike
> ___
> dev-planning mailing list
> dev-plann...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-planning
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform