Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

pyby
Copy link
Member

@pyby pyby commented Jan 30, 2025

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

  • Removed serviceURL remote configuration property as a string.
  • Introduced serviceURLs remote configuration property, a JSON dictionary where service IDs act as keys and corresponding URLs as values (similar to optional playURLs remote configuration property).
  • Removed default local configurations.

How to test

  1. Add in a local ApplicationConfiguration.json:
 "serviceURLs":
  "{\"production\":\"https://il.srf.ch\"}",
  1. Rebuild the app.

The production server should now use this base url.

Checklist

  • I have followed the project's style guidelines.
  • I have performed a self-review of my own changes.
  • I have made corresponding changes to the documentation.
  • My changes do not generate new warnings.
  • I have tested my changes and I am confident that it works as expected and doesn't introduce any known regressions.
  • I have reviewed the contribution guidelines.

@pyby pyby added the maintenance Code maintenance (issue and PR) - release notes section label Jan 30, 2025
@pyby pyby changed the title Improve remote configurations Improve remote configuration for Service URL overrides Jan 30, 2025
@pyby pyby marked this pull request as ready for review January 30, 2025 19:34
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+update-configuration-options January 30, 2025 19:37 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+update-configuration-options January 30, 2025 19:37 Inactive
@pyby pyby requested a review from waliid January 31, 2025 23:11
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+update-configuration-options January 31, 2025 23:12 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+update-configuration-options January 31, 2025 23:14 Inactive

NSDictionary *serviceURLsDictionary = [self JSONDictionaryForKey:key];
for (NSString *key in serviceURLsDictionary) {
if ([ServiceObjC.ids containsObject:key]) {
Copy link
Member

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.

Copy link
Member Author

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 :)

Copy link
Member

@defagos defagos left a 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]>
@pyby
Copy link
Member Author

pyby commented Feb 4, 2025

I got first a mixed status of rollback or improve it as well.

Why trying to improve it after discussions:

@pyby pyby requested a review from waliid February 4, 2025 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Code maintenance (issue and PR) - release notes section
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants