Closed Bug 1413643 Opened 5 years ago Closed 5 years ago

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


(Firefox :: Normandy Client, defect)

Not set



Tracking Status
firefox57 --- fixed
firefox58 --- fixed


(Reporter: mythmon, Assigned: mythmon)




(1 file)

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 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]: 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?]:

[Has the fix been verified in Nightly?]:

[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]:

[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]:
Attachment #8924269 - Flags: approval-mozilla-beta?
QA Contact: adrian.florinescu
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 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+
Also rhelmer said he was r+'d this patch in github so we are good on that front too.
Comment on attachment 8924269 [details]
Bug 1413643 - Sync shield recipe-client v76.1 from Github (commit 10b351b)

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+
Pushed by
Sync shield recipe-client v76.1 from Github (commit 10b351b) r=RyanVM
Closed: 5 years ago
Resolution: --- → FIXED
See Also: → 1414042
Attachment #8924269 - Flags: review?(rhelmer) → review+
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.