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.

Sign up for GitHub

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

Jump to bottom

Use unified BattleNet server #813

Merged
merged 2 commits into from
Nov 14, 2023
Merged

Use unified BattleNet server #813

merged 2 commits into from
Nov 14, 2023

Conversation

martincostello
Copy link
Member

Implemented based on #812 (comment).

Unless it's because of my office's DNS, I couldn't get oauth.battlenet.com.cn to resolve, so I left the URLs for China as they were.

I don't actually have a BattleNet account so I haven't tested this against the actual server.

Resolves #812.

@martincostello martincostello mentioned this pull request Nov 7, 2023
Closed
@kevinchalet
Copy link
Member

Unless it's because of my office's DNS, I couldn't get oauth.battlenet.com.cn to resolve, so I left the URLs for China as they were.

Yeah, I noticed the same thing when testing the OpenIddict implementation. It just seems this domain name - that has a CNAME entry that points to AWS China - is not resolvable everywhere: https://www.whatsmydns.net/#CNAME/oauth.battlenet.com.cn

@martincostello
Use IPostConfigureOptions
80cd250 
- Use `IPostConfigureOptions<T>` to configure the URLs for BattleNet.
- Add new `Unified` and `Custom` regions.
- Update/add XML documentation.
- Undo obsolete attributes.
@martincostello martincostello marked this pull request as ready for review November 14, 2023 11:33
kevinchalet
kevinchalet approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@kevinchalet kevinchalet left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

(adding a new Unified constants class is fine, but maybe that would have been safer to just update the existing "constants" to use the new server address, in case they'd change the rules in the future and "de-unify" some of the regions?)

@martincostello
Copy link
Member Author

I figured it was better to let people just switch the region so if they de-split it again, then we don't need to change anything in the code at all, people just change their Region value back to one of the original ones. If we change all the field values and they de-unify, then we need to do a new release to change them back again.

I figured this was a good middle ground of back-compat and not preventing the use of the unified regions at all.

@kevinchalet
Copy link
Member

Yeah, it's a reasonable compromise 👍🏻

@martincostello martincostello added this to the 8.0.0 milestone Nov 14, 2023
@martincostello martincostello merged commit 8dfccb3 into aspnet-contrib:dev-v8 Nov 14, 2023
8 checks passed
@martincostello martincostello deleted the battlenet-almost-global branch November 14, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinchalet kevinchalet kevinchalet approved these changes

Assignees
No one assigned
Labels
enhancement
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@martincostello @kevinchalet