1349348 - Sync shield-recipe-client from GitHub (commit ded0b7)

Status: RESOLVED FIXED

(Firefox :: Normandy Client, defect)

Description

[Michael Cooper [:mythmon]]

+++ This bug was initially created as a clone of Bug #1336501 +++

PRs since last sync:

81329c7 Merge pull request #614 from Osmose/clear-classify-cache - Clear client classification cache during mock-server test runs.
c08ea2d Merge pull request #615 from Osmose/fix-tc - Do not exit early during TaskCluster builds.
338d8b7 Merge pull request #605 from mythmon/dont-ship-tests - recipe-client-addon: Don't include test files in build
9f56c33 Merge pull request #583 from mythmon/new-url - Update canonical site url to normandy.services.mozilla.com
c2e6d84 Merge pull request #568 from Osmose/long-heartbeat - Fix long show-heartbeat messages
64242f3 Merge pull request #557 from Osmose/default-browser-filter - Add missing filter expression context to add-on.
9a3c254 Merge pull request #555 from Osmose/disabled-recipes - Fix #544: Correctly send parameters passed to NormandyApi.
a10a949 Merge pull request #510 from mythmon/api-index - Make classify-client never use the CDN (via self-describing API)
6e614c2 Merge pull request #514 from mythmon/build-xpi - recipe-client-addon: test and build xpi in TC
239f5dc Merge pull request #546 from mythmon/event-emitter-log-debug - recipe-client-addon: Log events with no listener as DEBUG, not INFO
801a117 Merge pull request #477 from mythmon/no-shadow-linting - recipe-client-addon: Add no-shadow lint rule from m-c

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

Review commit: https://reviewboard.mozilla.org/r/122482/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122482/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8849708 [details]
Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

https://reviewboard.mozilla.org/r/122482/#review124654

rs=me, but you probably want to trypush before landing this. It'd also be a good idea to actually mark the moved+altered files as file moves, though I guess given the canonical history is on github it might not be that important.

Comment 3

[Michael Cooper [:mythmon]]

Comment on attachment 8849708 [details]
Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

https://reviewboard.mozilla.org/r/122482/#review124654

I ran this in try, and things look good to me.

I'm not sure how I could mark things as moved in mercurial. Of course I could figure out what files that applies to, and run the commands to mark them as moved, but I don't think that is something that I want to be doing for every one of these merges. Is there a way to get mercurial to guess at files that moved, like git does?

Also, I'm not very concerned about preservering a nice history, because as you pointed out, the canonical history is on GitHub. This commit is a squash of a bunch of git commits. If we really wanted to be preserving history, we would include each of those commit separately (which I don't think we should be doing right now).

Comment 4

[:Gijs (he/him)]

(In reply to Mike Cooper [:mythmon] from comment #3)
> I'm not sure how I could mark things as moved in mercurial.

edit the commit with hg histedit, then:

hg mv foo.jsm bar.jsm --after

> Of course I
> could figure out what files that applies to, and run the commands to mark
> them as moved, but I don't think that is something that I want to be doing
> for every one of these merges. Is there a way to get mercurial to guess at
> files that moved, like git does?

I'm not sure why marking file moves (I think there's fewer than 5 in this commit?) is that onerous... perhaps that's just stockholm syndrome on my part. I think git's move stuff has to do with how it tracks files, though, based on what I saw after the sha1 collision stuff got published, so I'm not sure it's even possible for hg to do the same. Maybe :gps knows.

Comment 5

[Gregory Szorc [:gps]]

Git doesn't track copies and renames explicitly. Instead, it relies on run-time behavior to brute force content similarity to attribute copies and renames.

If you are importing commits from Git to Mercurial, first, we have tools in version-control-tools that can do this more robustly than whatever has been hacked up here. This is how we import Servo, for example. But, if you insist on doing it yourself, just install Mercurial 4.1 and activate the "automv" extension, which will automatically add rename metadata when new files are committed.

Comment 6

[Michael Cooper [:mythmon]]

Thanks for the advice, :gps. As of right now we aren't doing a straight import of commits. We are treating mozilla-central more as build target, similar to how we build XPIs. This isn't ideal, and we'd like to change to something more like what Servo is doing at some point.

Automv is exactly the kind of thing I was hoping existed. I'll use that for now.

Comment 7

[Michael Cooper [:mythmon]]

Comment on attachment 8849708 [details]
Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122482/diff/1-2/

Comment 8

[Michael Cooper [:mythmon]]

I enabled automv, remade the commit, and pushed it up to mozreview. :Gijs, can you confirm that this looks right?

Comment 9

[:Gijs (he/him)]

(In reply to Mike Cooper [:mythmon] from comment #8)
> I enabled automv, remade the commit, and pushed it up to mozreview. :Gijs,
> can you confirm that this looks right?

This looks the same to me? There don't seem to be any move annotations in it. See here for an example of how mozreview shows those normally: https://reviewboard.mozilla.org/r/122362/diff/3

At this point, given the history is in github anyway, I don't know that it makes sense to spend more time on it. Let's land this and try to get it right next time. I guess I'm also hoping that we will at some point in the near-ish future stop having to move files around as often... :-)

Comment 10

[Michael Cooper [:mythmon]]

Comment on attachment 8849708 [details]
Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122482/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/705c76502231
Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs r=Gijs

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/705c76502231

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Talos (maybe others, too, haven't looked) failures started adding a line referencing Shield after this landed:
https://treeherder.mozilla.org/logviewer.html#?job_id=86367475&repo=autoland&lineNumber=1622

Comment 14

[Michael Cooper [:mythmon]]

Filed bug 1351465 for that issue.

Comment 15

[Michael Cooper [:mythmon]]

Comment on attachment 8849708 [details]
Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

Approval Request Comment
[Feature/Bug causing the regression]: Improvements to Shield self-repair driver
[User impact if declined]: When the current Aurora train switches to beta, the version of self-repair will regress.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: This version of the code is already been tested for Beta and Aurora
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is well tested code that is going to be shipped directly on Beta soon.
[String changes made/needed]: None

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8849708 [details]
Bug 1349348 - Sync shield-recipe-client from GitHub (ded0b7). rs=Gijs

Take this to avoid regression. Aurora54+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e48f9d2488ec