1185196 - Fix trailing comma in ua-update.json

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Taylor [:miketaylr]]

In Bug 1177298 I left a trailing comma, forgetting that the .json file is ... JSON and not just a JS object.

Since we load overrides from the file locally, the overrides still work--but any future updates won't make it to the server because it does a JSON validity check before syncing: https://github.com/mozilla/dynamicua_update_script/blob/master/update#L96

Sort of an embarassing patch, but here we are. I'll probably start working on a test for this next week (to avoid pulling this in the future)--but for the p11 stuff I thought it would be good to get this in now.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 1185196.-Remove-trailing-comma-so-we-have-valid-.patch

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8635663 [details] [diff] [review]
1185196.-Remove-trailing-comma-so-we-have-valid-.patch

Review of attachment 8635663 [details] [diff] [review]:
-----------------------------------------------------------------

Rubberstamp.

Comment 3

[Mike Taylor [:miketaylr]]

Thanks Richard!

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: 1185196.-Remove-trailing-comma-so-we-have-valid-.patch

Updating commit message to reflect r=rnewman, carrying forward r+.

Comment 5

[:Margaret Leibovic]

(In reply to Mike Taylor [:miketaylr] from comment #0)
> In Bug 1177298 I left a trailing comma, forgetting that the .json file is
> ... JSON and not just a JS object.
> 
> Since we load overrides from the file locally, the overrides still work--but
> any future updates won't make it to the server because it does a JSON
> validity check before syncing:
> https://github.com/mozilla/dynamicua_update_script/blob/master/update#L96
> 
> Sort of an embarassing patch, but here we are. I'll probably start working
> on a test for this next week (to avoid pulling this in the future)--but for
> the p11 stuff I thought it would be good to get this in now.

I see a relevant-looking test already in the tree:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/mochitests/test_user_agent_updates.html

Maybe there's a way we can add something simple to that test to catch this bug?

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/208c6dbcfc99

Comment 7

[Mike Taylor [:miketaylr]]

(In reply to :Margaret Leibovic from comment #5)
 > I see a relevant-looking test already in the tree:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/test/mochitests/
> test_user_agent_updates.html
> 
> Maybe there's a way we can add something simple to that test to catch this
> bug?

Yeah, that's a good plan. Right now that test is disabled (See https://bugzilla.mozilla.org/show_bug.cgi?id=1078267#c146), but one of my Q3 goals is to figure out why and (hopefully) get it re-enabled.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/208c6dbcfc99