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

Remote configuration for Data Provider Service URL overrides #540

Merged
merged 9 commits into from
Feb 27, 2025

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.

Update: after feedback from SRF devs and Pillarbox devs:

  • always be explicit as de default fallback url to use.
  • Avoid complex property, so that in blocker situation, a simple value is easy to set.

This PR refines the approach by introducing a structured remote configuration property that allows each service (environment) to have its own substitution URL.

This PR refines the approach by introducing three remote configuration properties that allow each Integration Layer environment to have its own substitution URL.

Changes Made

  • Removed serviceURL remote configuration property as a string.
  • Introduced dataProviderProductionServiceURL, dataProviderStageServiceURL and dataProviderTestServiceURL remote configuration properties, optional strings which can override default Integration Layer urls.
  • Removed default local configurations for the removed serviceURL property.
  • Change local preference "user default" property name from PlaySRGSettingServiceIdentifier to PlaySRGSettingServiceEnvironment.
    • ℹ️ The server environment will be reset to production, as the name changed.

How to test

  1. Add in a local ApplicationConfiguration.json:
"dataProviderProductionServiceURL": "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
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]>
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+update-configuration-options February 4, 2025 15:23 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+update-configuration-options February 4, 2025 15:23 Inactive
@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
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+update-configuration-options February 16, 2025 17:33 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+update-configuration-options February 16, 2025 17:34 Inactive
@pyby pyby changed the title Improve remote configuration for Service URL overrides Remote configuration for Data Provider Service URL overrides Feb 16, 2025
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+update-configuration-options February 16, 2025 17:54 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+update-configuration-options February 16, 2025 18:12 Inactive
@defagos
Copy link
Member

defagos commented Feb 18, 2025

I still think my comment above might be relevant.

The fact that other products do something similar is not a justification IMHO. The real question is whether such a mechanism is actually relevant in the first place.

Being able to switch the service to another one on-the-fly is based on the assumption that there is a working service fallback. In most catastrophic scenarios it is likely there will be a broad service disruption, meaning there won't be any fallback available. An erroneously DNS configuration deployment is the only case in which I can see value in this mechanism (since the working fallback is just elsewhere, not suffering from failure). But I doubt this should ever happen again, and I think adding a whole new mechanism to address this use case is unjustified.

Feel free to ignore my comment, but if you still decide to merge this PR I would expect the same to be available for Android as well (I don't see any corresponding PR yet).

@pyby pyby requested a review from defagos February 22, 2025 13:03
@rts-devops rts-devops temporarily deployed to playsrg-ios-nightly+update-configuration-options February 22, 2025 13:05 Inactive
@rts-devops rts-devops temporarily deployed to playsrg-tvos-nightly+update-configuration-options February 22, 2025 13:06 Inactive
@pyby pyby added this pull request to the merge queue Feb 27, 2025
Merged via the queue into main with commit cfa0302 Feb 27, 2025
4 checks passed
@pyby pyby deleted the update-configuration-options branch February 27, 2025 14:04
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