Sync shield recipe-client v76.1 from Github (commit 10b351b)

RESOLVED FIXED

Status

Shield
Add-on
RESOLVED FIXED
18 days ago
13 days ago

People

(Reporter: mythmon, Assigned: mythmon)

Tracking

unspecified

Firefox Tracking Flags

(firefox57 fixed, firefox58 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

18 days ago
Changes between v76 and v76.1

10b351b - Bug 1411368 - Automatically fix no-multi-spaces issues raised when using ESLint 4. r=mossop
08366ff - Bug 1412778 - Enable ESLint rule no-cpows-in-tests across the whole tree. r=florian
ba992dd - Cachebust Taskcluster docker image.
269c83d - PR #1100 - Revert study preferences to empty when appropriate.
32d23b0 - Fixes #1100 Reset pref studies to the correct value when stopping.

#1100 is the important change here. The other 3 are for compatibility with CI on Nightly. They should both be no-ops when merging to mozilla-central.
Comment hidden (mozreview-request)
(Assignee)

Comment 2

18 days ago
Comment on attachment 8924269 [details]
Bug 1413643 - Sync shield recipe-client v76.1 from Github (commit 10b351b)

I've discussed this with Ritu, and we decided that if there aren't any major barriers, we'd prefer to have this change in 57.

Approval Request Comment
[Feature/Bug causing the regression]:
https://github.com/mozilla/normandy/issues/1100 To summarize, when some Shield studies end, they put users in a undesired state for one reset cycle. 

[User impact if declined]:
Activity Stream and Unified Search experiments face greater risk of encountering roll-back bugs. Specifically, if we want to change studies quickly, we have to account for this bug, which may be a major problem.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
No.

[Needs manual test from QE? If yes, steps to reproduce]:
Has already been verified by Softvision.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No major risks are known.

[Why is the change risky/not risky?]:
This change is well tested and simple.

[String changes made/needed]:
None
Attachment #8924269 - Flags: approval-mozilla-beta?
(Assignee)

Updated

18 days ago
QA Contact: adrian.florinescu

Comment 3

18 days ago
This is a really severe issue and given that SHIELD might be used to rollout a bunch of features in 57 and beyond 57, the sooner this is fixed the better. Here is an example of the severity of the problem:

* 57 goes live on Nov 14th with Activity Stream turned on
* A week later a SHIELD study starts that turns AS off for a few users.
* 2 weeks later, due to severe problems in AS we decide to turn it off
* 57.0.1 is shipped with AS pref'd off
* SHIELD study ends a week later and reverts the users in control group from AS off to AS on. 

This would be a terrible outcome. I am told this bug would remain until the end-user restarts their client which could be anywhere from a few hours to days/week/months. 

I will wait for rhelmer's R+ and weighing in on the risk landing this change a few days before RC.

Comment 4

17 days ago
Comment on attachment 8924269 [details]
Bug 1413643 - Sync shield recipe-client v76.1 from Github (commit 10b351b)

Rhelmer give me the A-OK to take this patch in Beta57 over slack.
Attachment #8924269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 5

17 days ago
Also rhelmer said he was r+'d this patch in github so we are good on that front too.

Updated

17 days ago
status-firefox57: --- → affected

Comment 6

17 days ago
mozreview-review
Comment on attachment 8924269 [details]
Bug 1413643 - Sync shield recipe-client v76.1 from Github (commit 10b351b)

https://reviewboard.mozilla.org/r/195490/#review201114

Marking this r+ for the sake of being able to autoland it and get it uplifted to 57. Would still be good for Rob to get his r+ in MozReview eventually too for posterity.
Attachment #8924269 - Flags: review+

Comment 7

17 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/acc83e81c859
Sync shield recipe-client v76.1 from Github (commit 10b351b) r=RyanVM

Comment 8

17 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8c3926699de4 (FIREFOX_57b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/76b9127f9511
status-firefox57: affected → fixed

Comment 9

17 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/acc83e81c859
Status: NEW → RESOLVED
Last Resolved: 17 days ago
status-firefox58: --- → fixed
Resolution: --- → FIXED

Updated

13 days ago
See Also: → bug 1414042
You need to log in before you can comment on or make changes to this bug.