-
-
Notifications
You must be signed in to change notification settings - Fork 100
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(rest)!: Fix Basic OAuth2 queues and add method for updating the queues and ratelimit paths after a refresh #3185
base: main
Are you sure you want to change the base?
Conversation
BenchmarkDetail results of benchmarks
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3185 +/- ##
==========================================
- Coverage 76.72% 76.30% -0.43%
==========================================
Files 193 193
Lines 23384 23627 +243
Branches 763 747 -16
==========================================
+ Hits 17941 18028 +87
- Misses 5434 5588 +154
- Partials 9 11 +2
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good, please have this tested properly before merging but the general idea looks good to me
I'm currently running this pr in production, atm i do not have something that uses OAuth ready to test this on (but the test scripts i made while making the code changes are working fine). I'm going to disable the auto-merge so this can be tested a bit more before merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! You mentioned that you disabled auto-merge so this can be tested a bit more before merge, it's been a month so what do you think?
Didn't had the time to setup the stuff i need for the OAuth2 testing, but the bot auth is working on my prod bot. If i can today i will test it and enable auto merge accordingly |
Re enabling the auto-merge as from my testing I did today it seems to be fine. It does carry over the ratelimit from the previous queue and it does change the auth header preventing the 401 |
This pull request addresses 3 issues:
updateTokenQueues
method to be called by the user-code to update the queues with the new token