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

Switch Sync environment to production #503

Merged
merged 10 commits into from
Sep 14, 2023
Merged

Switch Sync environment to production #503

merged 10 commits into from
Sep 14, 2023

Conversation

ayoy
Copy link
Contributor

@ayoy ayoy commented Sep 14, 2023

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/0/1205489036222324/f
iOS PR: duckduckgo/iOS#2009
macOS PR: duckduckgo/macos-browser#1622
What kind of version bump will this require?: Major

Description:
This patch allows to control Sync server environment from client apps.

  • ServerEnvironment enum is added
  • DDGSync dependencies is now mutable
  • Endpoints is now a class (it's shared between AccountManager and SyncQueue so it's handy
    to have it passed around as a reference), can be initialized with ServerEnvironment and can have
    baseURL updated after creation
  • DDGSyncingDebuggingSupport and SyncDependenciesDebuggingSupport protocols
    were added to allow overriding server environment

Steps to test this PR:
Refer to platform PRs for testing steps.

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12

Internal references:

Software Engineering Expectations
Technical Design Template

@ayoy ayoy self-assigned this Sep 14, 2023
@ayoy ayoy marked this pull request as ready for review September 14, 2023 07:30
try? updateAccount(nil)
dependencies.updateServerEnvironment(serverEnvironment)
authState = .initializing
initializeIfNeeded(isInternalUser: false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it could be a good time to get rid of isInternalUser parameter and that old migration, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, good idea. Let me do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the parameter but decided to keep SyncError.failedToMigrate because I'm afraid that if we remove it, error codes will get shifted by 1 (and skew our pixels). I have a task on improving Sync Error reporting in pixels so when I get around to working on that, I'll set error codes manually and will remove this enum case.

Copy link
Collaborator

@bwaresiak bwaresiak left a comment

Choose a reason for hiding this comment

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

LGTM!

@ayoy ayoy merged commit 2a3dc29 into main Sep 14, 2023
@ayoy ayoy deleted the dominik/sync-env-switcher branch September 14, 2023 09:37
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.

2 participants