-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improve remote configuration for Service URL overrides #540
base: main
Are you sure you want to change the base?
Conversation
|
||
NSDictionary *serviceURLsDictionary = [self JSONDictionaryForKey:key]; | ||
for (NSString *key in serviceURLsDictionary) { | ||
if ([ServiceObjC.ids containsObject:key]) { |
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.
For the ServiceObjC
, we still have ids
, which could lead to confusion. Here is a patch. I may have gone a bit too far, so feel free to keep only the necessary changes.
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.
@waliid I reviewed your patch. ServiceObjC.environments
was in my thinking part as well.
Few missing updates in SettingsView.swift
and make check-quality
failed, so a apply it myself and try to add you as co-author :)
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.
I see that @waliid already reviewed the code, I therefore won't delve into the actual implementation itself.
Still, as I was asked for a review, I would question the need for such a configuration mechanism in the first place. What happened last week was something totally unexpected (and which should never have occurred in the first place), but we cannot have code to deal with any kind of potential failure our products could face. Otherwise we would also need to have a local copy of the metadata database in case someone drops the database, which should never occur in practice. IMHO we have to identify where safety nets must be meaningfully introduced, but not introduce them for the sake of having a safety net for each kind of potential failure.
For this reason, instead of improving what was added in #538, I would rather suggest reverting #538 now that the problem is over. If you think this mechanism is really needed, I would also suggest you align with other platforms so that the desired feature can be enabled everywhere consistently.
Co-authored-by: Walid Kayhal <[email protected]>
I got first a mixed status of rollback or improve it as well. Why trying to improve it after discussions:
|
Description
The previous implementation in #538 was an urgent fix, but the service URL override mechanism was not optimal.
This PR refines the approach by introducing a structured remote configuration property that allows each service (environment) to have its own substitution URL.
Changes Made
serviceURL
remote configuration property as a string.serviceURLs
remote configuration property, a JSON dictionary where service IDs act as keys and corresponding URLs as values (similar to optionalplayURLs
remote configuration property).How to test
ApplicationConfiguration.json
:The production server should now use this base url.
Checklist