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

RCORE-2010 Update default base URL to new domain #7534

Merged
merged 6 commits into from
Apr 2, 2024

Conversation

michael-wb
Copy link
Contributor

@michael-wb michael-wb commented Mar 28, 2024

What, How & Why?

Updated default base url to https://services.cloud.mongodb.com.
The default base url string value can be retrieved using App::default_base_url() static accessor or the realm_app_get_default_base_url() C_API function.

Fixes #7449

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • [ ] C-API, if public C++ API changed
  • [ ] bindgen/spec.yml, if public C++ API changed

@michael-wb michael-wb self-assigned this Mar 28, 2024
@cla-bot cla-bot bot added the cla: yes label Mar 28, 2024
Copy link

coveralls-official bot commented Mar 28, 2024

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1026

Details

  • 36 of 36 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 92.147%

Totals Coverage Status
Change from base Build 2184: 0.04%
Covered Lines: 245396
Relevant Lines: 266309

💛 - Coveralls

bindgen/spec.yml Outdated
@@ -1244,6 +1244,7 @@ classes:
all_users: std::vector<SharedSyncUser>
sync_manager: SharedSyncManager
subscribers_count: count_t
default_base_url: std::string_view
Copy link
Member

Choose a reason for hiding this comment

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

The properties key in the spec declare instance properties (implemented through instance methods on the C++ class), not static properties of the App: We currently don't have a way to expose static methods from C++ as static properties on the class, nor static properties directly.

Do you intent for SDKs to be able to set this value? I don't expect that to be the case, since it's a string_view. In fact, is there an SDK need to expose it at all?

If you want this to be exposed as a read-only static member of App, you'd need to conform to the style of the existing C++ code and express this as a static default_base_url function on App.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @kraenhansen - I have updated App::default_base_url to be a static accessor function (e.g. App::default_base_url()) - the purpose of this is to (hopefully) provide the default value to the SDKs so they don't have to define their own default base URL value.

Copy link
Member

@kraenhansen kraenhansen Apr 2, 2024

Choose a reason for hiding this comment

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

Until an SDK is actually expecting to consume this, I would suggest simply leaving it out of the spec.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - I will remove the reference until it is needed.

@jbreams jbreams requested a review from mpobrien March 28, 2024 14:56
Copy link

@mpobrien mpobrien left a comment

Choose a reason for hiding this comment

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

LGTM.
Since this is a "breaking" change, will it be a new major version of core? And if so, does that require that SDK releases which pull in that core version are published as new major versions?
In any case we'll need to communicate to any customers that might rely on domain whitelisting that by upgrading to an SDK that includes this change, they will need to either add the new set of domains to their whitelist, or explicitly set
the old https//realm.mongodb.com as their baseURL in the meantime.

@danieltabacaru
Copy link
Collaborator

In any case we'll need to communicate to any customers that might rely on domain whitelisting that by upgrading to an SDK that includes this change, they will need to either add the new set of domains to their whitelist, or explicitly set
the old https//realm.mongodb.com as their baseURL in the meantime.

As an alternative we could keep the default to https//realm.mongodb.com and the users have to explicitly set the new domain.

@melodeeli98
Copy link

melodeeli98 commented Apr 1, 2024

As an alternative we could keep the default to https//realm.mongodb.com and the users have to explicitly set the new domain.

The App Services team requested this change so that migration to the new domains is as seamless as possible, especially with regards to users migrating to the new Private Endpoints. We decided that telling customers to upgrade their SDK versions would be easiest. The assumption was that any customers that experience issues with the new domain can choose to either stay in an older SDK version or override the base url themselves until they resolve their issues.

@michael-wb
Copy link
Contributor Author

Thank you for the reply @melodeeli98!

@michael-wb michael-wb merged commit ce2c0fe into master Apr 2, 2024
35 of 38 checks passed
@michael-wb michael-wb deleted the mwb/update-default-base-url branch April 2, 2024 14:19
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update default base URL to new domain
6 participants