Skip to content

Deprecate RemoteSettingsConfig.server_url in favor of a new, type-safe RemoteSettingsConfig.server#6191

Merged
linabutler merged 2 commits intomainfrom
remote-settings-server
Apr 4, 2024
Merged

Deprecate RemoteSettingsConfig.server_url in favor of a new, type-safe RemoteSettingsConfig.server#6191
linabutler merged 2 commits intomainfrom
remote-settings-server

Conversation

@linabutler
Copy link
Copy Markdown
Contributor

@linabutler linabutler commented Apr 4, 2024

The RemoteSettingsConfig.server_url option has a couple of small pain points for consumers:

  1. It forces them to hard-code the URLs for other environments (Fenix does this for stage here and here, for example). We can make things a bit smoother for these callers by having the Remote Settings component hard-code these URLs instead.
  2. It forces other consumers of Remote Settings, like Suggest, to override all config options—including the bucket and collection name—even if they just wanted to override the server URL. For example, if Fenix wanted to ingest suggestions from the staging server, it would need to also specify collectionName: "quicksuggest", even though the collection name is an implementation detail of the Suggest component.

This PR tries to make that situation better by:

  1. Introducing a new type, RemoteSettingsServer, that allows specifying a well-known (Prod, Stage, Dev) or Custom server.
  2. Adding RemoteSettingsServer.server and SuggestStoreBuilder.remote_settings_server().
  3. Deprecating RemoteSettingsConfig.server_url and SuggestStoreBuilder.remote_settings_config(). These APIs are still available, but can't be used together with the new APIs—consumers must specify one or the other, or neither (to use the default Prod server), but not both.

This is the first step to wiring up Fenix's "use Remote Settings {production / stage} server" switch to Suggest (bug 1889495).

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@linabutler linabutler requested a review from bendk April 4, 2024 02:05
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 4, 2024

Codecov Report

Attention: Patch coverage is 0% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 83.85%. Comparing base (5ab11bb) to head (7999f63).

Files Patch % Lines
components/remote_settings/src/config.rs 0.00% 10 Missing ⚠️
components/remote_settings/src/client.rs 0.00% 9 Missing ⚠️
.../support/nimbus-cli/src/sources/experiment_list.rs 0.00% 3 Missing ⚠️
components/remote_settings/src/error.rs 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6191      +/-   ##
==========================================
- Coverage   83.96%   83.85%   -0.11%     
==========================================
  Files         117      117              
  Lines       15651    15671      +20     
==========================================
  Hits        13141    13141              
- Misses       2510     2530      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@linabutler linabutler force-pushed the remote-settings-server branch 2 times, most recently from 6b9baa9 to 2b160fa Compare April 4, 2024 02:24
Copy link
Copy Markdown
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is great.

What's the deprecation plan for server_url? My preference would be to remove it in the near future if possible.

}),
server_url: None,
bucket_name: None,
collection_name: collection_name.to_string(),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using ..RemoteSettingsConfig::default() at the end of the struct initialization? It could be a nice way to handle server_url being deprecated and eventually being removed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I don't think that'll work, because RemoteSettingsConfig doesn't implement Default. We could have it derive Default, but we don't have a suitable default that we can use for collection_name, since that's the only setting that is required.

I wonder if moving RemoteSettingsConfig to a builder-style API would be a good long-term direction, though?

let b = RemoteSettingsConfig::with_collection_name("quicksuggest".into());
b.server(RemoteSettingsServer::Stage)?.bucket_name("main".into()); // Optional.
let client = b.build();

Copy link
Copy Markdown
Contributor Author

@linabutler linabutler Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could even do that in a backward-compatible way by adding that new API as RemoteSettingsBuilder, deprecating RemoteSettingsConfig, and migrating callers over to RemoteSettingsBuilder.

I'm really liking the builder pattern you added for Suggest!

@linabutler
Copy link
Copy Markdown
Contributor Author

What's the deprecation plan for server_url? My preference would be to remove it in the near future if possible.

Same! I think only Fenix and Desktop use it now, so we can migrate it as soon as this PR lands in a nightly, and then remove server_url in the next two weeks or so.I'm not sure how our breaking change flow works post-Android monorepofication, and this change felt relatively easy to make backward-compatible! 😅

@linabutler linabutler force-pushed the remote-settings-server branch from 2b160fa to 7999f63 Compare April 4, 2024 18:27
@linabutler linabutler force-pushed the remote-settings-server branch from 7999f63 to 5204782 Compare April 4, 2024 19:04
@linabutler linabutler added this pull request to the merge queue Apr 4, 2024
Merged via the queue into main with commit 0152f26 Apr 4, 2024
@linabutler linabutler deleted the remote-settings-server branch April 4, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants