Sync shield-recipe-client from GitHub (commit e37261c)




2 years ago
2 years ago


(Reporter: osmose, Assigned: osmose)


Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)



(1 attachment)

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

Major feature of this sync is the initial implementation of Preference Experiments.

PRs since last sync (Commit PR# - Description):

dc28b5d PR #713 - Merge preference-experiments into master
44a7721 PR #684 - Watch experiment prefs
8009444 PR #702 - [#636] Update DRF to avoide Crashes
c7ebb9e PR #673 - Add pref experiment type checks
faba71f PR #700 - Merge master into preference-experiment
46ac267 PR #697 - recipe-client-addon: Convert PreferenceExperiments tests to mochitest.
d87b9bd PR #699 - #698: Treat malformed lastShown dates as valid in show-heartbeat.
966030c PR #696 - Fix mach lint error for useless parameter to addObserver.
a234dd6 PR #692 - Merge preference experiments into master
cef4cb4 PR #693 - Refactor TaskCluster tasks and fix linting / tests
0386d6a PR #694 - Backport preference fix
61a024a PR #691 - Do not break `this` in functions wrapped with SandboxManager.wrapAsync.
4f0ee5d PR #690 - Re-add preprocessor directive and modify to strip it.
984da2d PR #683 - Bump TC max run time to 8 hours and deadline to 1 day.
7645206 PR #681 - Inform telemetry pt 2
cef2b33 PR #659 - Stop preference experiments when the server doesn't send them
c658d82 PR #679 - Fallback for LocaleService::GetAppLocale.
f4ec9f9 PR #686 - recipe-server: Update signatures when closing approval requests.
72fd339 PR #688 - recipe-server/contract-tests: Skip cache tests on api URLs.
e41c37b PR #685 - Merge master into preference-experiments branch
08a649e PR #677 - Revert "#633 from andymikulski/433-addon-storage-durability"
c260972 PR #678 - Remove comment from
86c8973 PR #670 - Fix #652: Add option for setting user or default prefs for experiments.
7a4afa0 PR #641 - recipe-client-addon: Use nsIUpdateTimerManager to run every 24 hours
f2e2775 PR #676 - Merge master into preference-experiments branch
a4015a0 PR #682 - recipe-server: Add test for signing consistency during approval.
cfb2e45 PR #671 - Update locales/channel/countries with recipe API
f066acc PR #666 - recipe-client-addon: Separate build and test in Taskcluster CI
f9f659a PR #672 - recipe-server: Update Django, 1.10.5 -> 1.10.7
590ccc2 PR #658 - Fix ordering on recipe history
68dce0e PR #660 - Fix #612: Add ability to filter recipes by active and expired experiments.
4595abb PR #651 - recipe-server: Add per-action validation of recipe arguments
bb68434 PR #661 - #351 Add Preference filter expressions
2d10f37 PR #662 - recipe-client-addon: Use getAppLocaleAsLangTag for getting browser locale
3e2231f PR #657 - recipe-client-addon: Calculate add-on versions automatically from git tags
1e0cafd PR #655 - taskcluster: Use custom base image for TC builds
5513601 PR #649 - recipe-client-addon: Unconditionally set up logging in bootstrap
deff7bd PR #633 - addon: Add Storage durability checks (#433)
606a355 PR #656 - recipe-server: Add migration to remove recipe signatures again
4c8401c PR #613 - Merge peer-signing feature branch into master
5279341 PR #648 - Update pytest-testrail to latest version
99aaf0d PR #630 - Updated documentation for running the contract API tests
99710cf PR #654 - Merge master into preference-experiment
22e714a PR #643 - Describe ratioSample in a simpler way, and use async/await for it.
8325105 PR #653 - Remove bucketCount argument from preference experiments.
261f597 PR #642 - Add normandy.recipe to filter expressions in the add-on.
b15294a PR #644 - Remove functional tests from CI
6b4e984 PR #639 - #587: Add Sampling.ratioSample and ratio branching for preference-experiments
6e96cac PR #634 - Bug 1349576: Remove use of services.sync.numClients preference.
c7013f2 PR #623 - Preference experiment driver API
334d902 PR #629 - Bug 1349596: Validate JEXL on the server instead of the client.
bf95e3e PR #632 - Fix TaskCluster builds.
2172b15 PR #628 - Bug 1348885: Do not call Cu.reportError on errors from the sandbox.
990e830 PR #626 - recipe-server: Use more eslint rules from eslint-plugin-react
44cb46c PR #603 - Basic preference experiment action
06b91fd PR #620 - recipe-server: Handle validation errors in API gracefully
7bbc9c3 PR #624 - Refactor all uses of Task.jsm to use native async/await instead.
9912541 PR #573 - Revision history lookup fix
4566d03 PR #571 - Update state tree correctly on status change
a13c996 PR #567 - Approval/Rejection comments
e0124e8 PR #558 - #490 Revision status indicator
19f2adc PR #559 - Revise `arguments` correctly
ec48aa4 PR #561 - Implement Enable/Disable button
8b65f4a PR #562 - Add `revision` property to Recipes store
a0e1f4b PR #554 - Add basic UI for Review Requests
ff8ac52 PR #551 - Recipe status workflow
e2ed691 PR #550 - Add User API endpoint and current user id to base template
f3d244c PR #518 - Document approval request workflow and API
b954377 PR #516 - Include the approval request with the serialized recipe
eb9f71e PR #502 - Change from `update` to `revise`
9869231 PR #498 - Add comment field to ApprovalRequest
814b560 PR #438 - Approval requests
Okay, I think I did the thing correctly this time. I mostly guessed at the tests and platforms I wanted for the pre-merge try run.
Blocks: 1359664
Try run failed due to a build error. I'm working on it in a GH issue:
Comment on attachment 8861716 [details]
Bug 1359655 - Sync shield-recipe-client from GitHub (commit e37261c)

I'm swamped and it looks like we'll need to figure out the bootstrap.js thing before proceeding, so punting on this review for now.

Though - I assume this is basically just a wrap-up of stuff I've already reviewed on github? In which case this is effectively going to be a rubberstamp review - no point looking at it all again...
Attachment #8861716 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #5)
> Though - I assume this is basically just a wrap-up of stuff I've already
> reviewed on github? In which case this is effectively going to be a
> rubberstamp review - no point looking at it all again...

#718 landed, and I've updated the patch with two more merges:

e37261c PR #721 - Fix try builds of the system add-on
d9331ba PR #714 - recipe-client-addon: Lazy load applicable modules in RecipeRunner
Summary: Sync shield-recipe-client from GitHub (commit dc28b5d ) → Sync shield-recipe-client from GitHub (commit e37261c)
Try run of the new commit (before I tagged it in Github):

Failures seem intermittent, I think?
Comment on attachment 8861716 [details]
Bug 1359655 - Sync shield-recipe-client from GitHub (commit e37261c)

rs=me. Yeah, the trypush looks fine at a glance. Thanks!
Attachment #8861716 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by
Sync shield-recipe-client from GitHub (commit e37261c) r=Gijs
philor: Reading bug 1352792, it looks like this is an intermittent failure that becomes permanently failed with our patch, as well as with another seemingly-unrelated patch (bug 1320994). Is it possible to disable the test until someone can fix it, especially given the comments in the test code about working around intermittent failures? I have no idea what our guidelines around disabling tests are.

If not, are there other options besides politely bugging Paolo or others to fix it so we can land? I could try my hand at it but I'm not at all familiar with the code that's failing.
Flags: needinfo?(philringnalda)
I disabled it, since the only thing you and Andreas were doing was causing it to move to a different chunk of browser-chrome, and whatever bizarre reason it has for failing in one chunk and not another, not being willing to run in whatever chunk you're assigned is a test party foul. You should be fine to reland once that disabling patch sticks.
Flags: needinfo?(philringnalda)
Awesome, thanks for the help!
Pushed by
Sync shield-recipe-client from GitHub (commit e37261c) r=Gijs
Closed: 2 years ago
Resolution: --- → FIXED
Depends on: 1361889
Comment on attachment 8861716 [details]
Bug 1359655 - Sync shield-recipe-client from GitHub (commit e37261c)

Approval Request Comment
[Feature/Bug causing the regression]:
Ability to run preference experiments in Beta/Release 54.

[User impact if declined]:
We will be unable to test Quantum changes and other features planning to run preference experiments (such as Screenshots) in release until a system add-on update is released via Balrog. 

In either case we plan to push an update, whether it's via Balrog or via uplift, and uplift does not require us to build a new XPI and get it signed and tested on Balrog. In both cases we have a good method to revert due to being a system add-on, so the only real difference between the two is extra effort (any any consequences of uplift that I'm not aware of), so the decision to request uplift is based mostly on that.

[Is this code covered by automated tests?]:

[Has the fix been verified in Nightly?]:

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, already has approval:

Note that the email mentions a recommendation to ride 55. We had previously agreed with QE to uplift to Beta, so there's some confusion that needs to be resolved. I'm going to talk with QE tonight about it when they get online, but in the meantime I think delaying merging another day will be worse than requesting uplift now.

[List of other uplifts needed for the feature/fix]:
Just this patch.

[Is the change risky?]:
Not really. 

[Why is the change risky/not risky?]:
It's been in Nightly for several weeks, and we've run preference experiments with no major client-related issues (we have hit a few issues due to the Normandy service, which have been resolved and are unrelated to the client).

In addition, as a system add-on, if we notice breakage in release, we can push an update via Balrog without doing a new Firefox release to revert the code to the previous version of the add-on quickly.

[String changes made/needed]:
Attachment #8861716 - Flags: approval-mozilla-beta?
Comment on attachment 8861716 [details]
Bug 1359655 - Sync shield-recipe-client from GitHub (commit e37261c)

The urgency driving the uplift request was running a preference experiment to test Firefox Screenshots, which has changed it's timeline to test and ship in 55 instead of 54. This means that riding the 55 train will work fine for us, so I'm dropping the uplift request. Sorry for the confusion!
Attachment #8861716 - Flags: approval-mozilla-beta?
Product: Shield → Firefox
You need to log in before you can comment on or make changes to this bug.