Closed Bug 1345023 Opened 7 years ago Closed 7 years ago

Change SYNC_API_VERSION to 1.5 and have SUPPORTED_PROTOCOL_VERSIONS only include this value

Categories

(Firefox :: Sync, enhancement)

Product:
Firefox
Component:
Sync
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: markh, Assigned: jcameron97, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 2 obsolete files)

Bug 1345023 - Changed SYNC_API_VERSION to 1.5 and updated SUPPORTED_PROTOCOL_VERSIONS to reflect this change.
59 bytes, text/x-review-board-request
markh
: review+
Details 
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]
Assignee: nobody → jcameron97
Initial patch file for fixing bug 1345023.
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.
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)
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!
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
Perfect! Thank you, that did the trick! I've submitted the review request to the review board.
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!
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?
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.
Attachment #8844666 - Attachment is obsolete: true
Attachment #8844666 - Flags: review?(markh)
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 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)
Attachment #8847446 - Attachment is obsolete: true
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)
Perfect! Is it ok for me to push it as another commit?
(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
Perfect, that worked! I've submitted for 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+
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
https://hg.mozilla.org/mozilla-central/rev/186d77e98206
Status: NEW → RESOLVED
Closed: 7 years 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.