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

Fix crash due to missing access token; delete CredentialsManager.kt #365

Merged
merged 13 commits into from
May 28, 2021

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented May 24, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>Fix crash due to missing access token; delete CredentialsManager.kt</changelog>.

Summary of changes

While users only set token in attr of MapView, a MapboxConfigurationException will be thrown as we can't find token from resource file and default value. This pr applies the token from attr to fix it.

User impact (optional)

@Chaoba Chaoba self-assigned this May 24, 2021
@Chaoba Chaoba requested a review from a team May 24, 2021 13:49
@Chaoba Chaoba added the bug 🪲 Something isn't working label May 24, 2021
@pengdev
Copy link
Member

pengdev commented May 24, 2021

Also thinking the ResourceOptionsManager stores the static default ResourceOptions, and access token is part of the ResourceOptions, so feels like CredentialsManager is redundant, shall we discuss whether we should remove the CredentialsManager?

@Chaoba Chaoba changed the title Fix crash due to missing access token Fix crash due to missing access token; delete CredentialsManager.kt May 25, 2021
@Chaoba Chaoba requested a review from pengdev May 26, 2021 10:50
Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

I'll drop 🚢 here as changes look ok to me and they fix a concrete bug. However perhaps we want to consider some alignment with iOS beforehand.

@Chaoba Chaoba force-pushed the kl-token branch 2 times, most recently from 8ecf63e to 8385455 Compare May 28, 2021 00:57
Co-authored-by: Peng Liu <peng.liu@mapbox.com>
Copy link
Member

@pengdev pengdev left a comment

Choose a reason for hiding this comment

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

LGTM with nits

Kevin Li and others added 2 commits May 28, 2021 20:35
@Chaoba Chaoba merged commit 7c46fbc into main May 28, 2021
@Chaoba Chaoba deleted the kl-token branch May 28, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants