-
Notifications
You must be signed in to change notification settings - Fork 8
Adding support for v2 api version. #329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to gradually migrate our clients from v1 to v2, shouldn't we have a pref on the client instead, and do there what you do here in the devtools:
const apiVersion = Services.prefs.getCharPref("api_version", "/v1");
const serverUrl = new URL(SERVER_URL, apiVersion);
I was thinking leaving that as a single pref in Splitting it in two for the dev-tools UI just feels easier because instead of a single dropdown with 14 options we have two dropdowns. (and if/when we make a v3 it would be 21) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't really found a design that would allow us to gradually roll out clients to v2
The changes of this PR work to try things out on nightly or when running the client with MOZ_REMOTE_SETTINGS_DEVTOOLS=1. But won't be able to also support our api version migration later
A potential idea would be to use a separate preference on the client:
Services.prefs.setIntPref("services.settings.api_version", 2);And on the client have something like:
let apiVersion = Services.prefs.getIntPref("services.settings.api_version");
apiVersion = [1, 2].contains(apiVersion) ? apiVersion : 1;
and do the concatenation on the client code
| } | ||
|
|
||
| if (serverURL.endsWith("v2")) { | ||
| apiVersion = "v2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work on beta/release because of this code:
https://searchfox.org/firefox-main/rev/14c08f0368ead8bfdddec62f43e0bb5c8fd61289/services/settings/Utils.sys.mjs#53-76
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, need to adjust that logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to my patch here: https://phabricator.services.mozilla.com/D277258
Adding support for v2 api version.
Need to:
This will require mozilla/remote-settings#1104 to be deployed for forwards compatability.