Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Cache token scope check failures #1871
Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.
Description of the Change
I did not cache tokens that fail the scope-check because the
We may also be able to address this with careful handling of how
We will no longer lock users out with rate-limit errors when the token stored in your OS keychain is revoked or older than the last time that we've changed the required OAuth scopes.
The Map of checked tokens could, theoretically, grow without bound. For it to be an issue you'd need to revoke and regenerate tokens millions of times. I'm guessing that the GitHub API would become unhappy with you long before your RAM started to noticeably bloat.
I've added unit tests to
To test it manually, I'll:
User Experience Research (Optional)
@@ Coverage Diff @@ ## master #1871 +/- ## ========================================== + Coverage 91.14% 91.21% +0.06% ========================================== Files 196 196 Lines 10745 10745 Branches 1575 1574 -1 ========================================== + Hits 9794 9801 +7 + Misses 951 944 -7
From manual testing - here's the call log from master:
And here's the call log from this branch:
We still have two duplicated HEAD requests; that's because we have two calls to
added this to In Progress 🔧
in Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0
Jan 2, 2019
Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0
moved this from In Progress 🔧
to QA Review 🔬
Jan 3, 2019
left a comment
@annthurium and I discussed to implement a mentionable user fallback for an insufficient token scope by loading the authors from last n commits instead of just showing nothing. But I would be happy to address this in another PR so we don't hold up release.
I left a question, but this otherwise LGTM
Yeah, this is kind of what I was thinking