Change SYNC_API_VERSION to 1.5 and have SUPPORTED_PROTOCOL_VERSIONS only include this value

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Sync
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: markh, Assigned: Joseph Cameron, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 months ago
Via bug 1344480 comment 3:

SYNC_API_VERSION reports we are using 1.1, but that's no longer true. SUPPORTED_PROTOCOL_VERSIONS means that we report our clients as supporting both 1.1 and 1.5, but that's really not true either. We should fix this to avoid general confusion.

IOW:
SYNC_API_VERSION:                      "1.5",
and:
const SUPPORTED_PROTOCOL_VERSIONS = [SYNC_API_VERSION]
(Reporter)

Updated

2 months ago
Assignee: nobody → jcameron97
(Assignee)

Comment 1

2 months ago
Created attachment 8844666 [details] [diff] [review]
Patch file for fixing bug 1345023.

Initial patch file for fixing bug 1345023.
(Reporter)

Comment 2

2 months ago
Thanks for the patch! It looks good, but can you please follow https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F - specifically, pushing it to reviewboard and using an appropriate comment (eg, "Bug XXX - description. r?markh") - then I'll push it to try to ensure it works and push it to the main repo.
(Assignee)

Comment 3

2 months ago
Comment on attachment 8844666 [details] [diff] [review]
Patch file for fixing bug 1345023.

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

Bug 1345023 - Changed the SYNC_API_VERSION constant to 1.5 and changed the SUPPORTED_PROTOCOL_VERSIONS constant to 1.5. r?markh
Attachment #8844666 - Flags: review?(markh)
(Reporter)

Comment 4

2 months ago
Thanks for the patch, but are you able to follow the above instructions and push it for review to reviewboard? That will allow me to automatically do a try run and if it passed automatically land it.

Thanks!
(Assignee)

Comment 5

2 months ago
I tried to push to review, but I received some errors related to my .hgrc file. I think I previously modified the .hgrc file, but can't access the default. Is there any way I can re-download the default .hgrc file?
You can run "mach mercurial-setup". That will configure your .hgrc with the right extensions, and walk you through adding a Bugzilla API key (https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey) for MozReview.

You'll also need to add `ircnick` to your .hgrc before you can push: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 months ago
Perfect! Thank you, that did the trick! I've submitted the review request to the review board.
(Reporter)

Comment 9

2 months ago
mozreview-review
Comment on attachment 8845195 [details]
Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change.

https://reviewboard.mozilla.org/r/118386/#review120320

I've pushed this to try to make sure it works, thanks! In the meantime, can you please change the commit message to read something like: "Bug 1345023 - change SYNC_API_VERSION to 1.5. r?markh" - it will need to look something like that to get it landed if the try run succeeds.

Thanks!
(Assignee)

Comment 10

2 months ago
mozreview-review-reply
Comment on attachment 8845195 [details]
Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change.

https://reviewboard.mozilla.org/r/118386/#review120320

Thanks for all the help Mark! Although, how would I go about changing the commit message? From a similar --amend command that is found in git? 

Also, I've noticed that for some reason, the tests that are running seem to be testing my previous commits on my local repo, even though the changes from those previous commits don't show in the diff view. Would I have to revert all my previous commits and redo my bug fix to alter this?
(Reporter)

Comment 11

2 months ago
Try is failing with:

TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_httpd_sync_server.js | test_info_collections - [test_info_collections : 100] 404 == 200

(it's quite noisy for some reason, but that's buried in there). That might be related to your other patches.

(In reply to Joseph Cameron from comment #10)
> Thanks for all the help Mark! Although, how would I go about changing the
> commit message? From a similar --amend command that is found in git?

Yes, hg commit --amend exists (although I use git locally, but that's reasonably complicated to get setup for our workflows)
 
> Also, I've noticed that for some reason, the tests that are running seem to
> be testing my previous commits on my local repo, even though the changes
> from those previous commits don't show in the diff view. Would I have to
> revert all my previous commits and redo my bug fix to alter this?

It depends on how you are using hg, but it may be that you could either use the "histedit" extension to delete those earlier commits, or even export the patch you want to a file, then "hg strip" away the head revisions until you are back at one you didn't create, then reapply the patch you want.

Feel free to find us in the #sync channel on mozilla's IRC server.
Comment hidden (mozreview-request)
(Reporter)

Updated

2 months ago
Attachment #8844666 - Attachment is obsolete: true
Attachment #8844666 - Flags: review?(markh)
(Reporter)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8845195 [details]
Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change.

https://reviewboard.mozilla.org/r/118386/#review122280

Thanks for the update, but it looks like the tree you are using is a couple of months old, so has a few conflicts with changes since then. Assuming you are using mercurial, you should do |hg pull --rebase| to bring in the recent changes, and manually fix the conflict you will get in constants.js, then re-push (after fixing the other issues below.

Thanks, and feel free to ping us in the #sync channel on IRC if you need more help doing this.

::: services/sync/tests/unit/head_http_server.js:6
(Diff revision 2)
>  var Cm = Components.manager;
>  
>  // Shared logging for all HTTP server functions.
>  Cu.import("resource://gre/modules/Log.jsm");
>  const SYNC_HTTP_LOGGER = "Sync.Test.Server";
> -const SYNC_API_VERSION = "1.1";
> +const SYNC_API_VERSION = "1.5";

Unfortunately this change breaks many tests which hard-code the old 1.1. So can you please change it back to 1.1 and add a comment above it saying something like "While the sync code itself uses version 1.5, many of our tests hard-code 1.1, so we are sticking with 1.1 here."

::: services/sync/tests/unit/test_clients_engine.js:35
(Diff revision 2)
>  
>      let cleartext = rec.decrypt(Service.collectionKeys.keyForCollection("clients"));
>  
>      _("Payload is " + JSON.stringify(cleartext));
>      equal(Services.appinfo.version, cleartext.version);
>      equal(2, cleartext.protocols.length);

With your changes, this test is failing, as cleartext.protocols.length is now 1 and no longer contains 1.1
Attachment #8845195 - Flags: review?(markh)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 17

2 months ago
mozreview-review
Comment on attachment 8847446 [details]
Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change. Also altered the changed SYNC_API_VERSION constant from 1.5 to 1.1 in the head_http_server test file  .

https://reviewboard.mozilla.org/r/120416/#review122476

I'm not sure what has gone wrong here, but this patch doesn't seem to have the previous patch changes. Is it possible you remove the MozReview-Commit-ID message when rebasing? Either way, https://reviewboard.mozilla.org/r/120416/ shows 2 different patches, which isn't what we want. I'll try and look more in the morning, but we need to get a single review commit with all the right changes.

::: commit-message-128d0:1
(Diff revision 2)
> +Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change. Also altered the changed SYNC_API_VERSION constant from 1.5 to 1.1 in the head_http_server test file r?markh .

Please remove the "Also altered" sentence - this needs to be the final commit message.

::: services/sync/tests/unit/head_http_server.js:10
(Diff revision 2)
>  var Cm = Components.manager;
>  
>  // Shared logging for all HTTP server functions.
>  Cu.import("resource://gre/modules/Log.jsm");
>  const SYNC_HTTP_LOGGER = "Sync.Test.Server";
> -const SYNC_API_VERSION = "1.5";
> +const SYNC_API_VERSION = "1.1";

Please add the comment I mentioned previously.
Attachment #8847446 - Flags: review?(markh)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8847446 - Attachment is obsolete: true
(Reporter)

Comment 19

a month ago
mozreview-review
Comment on attachment 8845195 [details]
Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change.

https://reviewboard.mozilla.org/r/118386/#review122728

The try run looks great, thanks! Could you please make the trivial change below and repush it here, then I'll land it \o/

::: services/sync/tests/unit/head_http_server.js:11
(Diff revisions 3 - 4)
>  // Shared logging for all HTTP server functions.
>  Cu.import("resource://gre/modules/Log.jsm");
>  const SYNC_HTTP_LOGGER = "Sync.Test.Server";
> -const SYNC_API_VERSION = "1.5";
>  
> +// While the sync code itself uses 1.5, some of our tests hard-code 1.1, 

A minor nit, but please remove the trailing whitespace on this line.
Attachment #8845195 - Flags: review?(markh)
(Assignee)

Comment 20

a month ago
Perfect! Is it ok for me to push it as another commit?
(Reporter)

Comment 21

a month ago
(In reply to Joseph Cameron from comment #20)
> Perfect! Is it ok for me to push it as another commit?

No, you should "amend" your existing commit, so there's still only 1 commit for your work. "hg commit --amend" should do the trick
Comment hidden (mozreview-request)
(Assignee)

Comment 23

a month ago
Perfect, that worked! I've submitted for review.
(Reporter)

Comment 24

a month ago
mozreview-review
Comment on attachment 8845195 [details]
Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change.

https://reviewboard.mozilla.org/r/118386/#review122762

Awesome, thanks!
Attachment #8845195 - Flags: review?(markh) → review+

Comment 25

a month ago
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/186d77e98206
Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change. r=markh

Comment 26

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/186d77e98206
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.