-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Use GITHUB_TOKEN
to make authorized user requests against GitHub (CI)
#22118
Conversation
I modified the test workflows for macOS, Linux, and Windows to report the rate limits. Following are excerpts from a failed Linux workflow: These show that before and after the tests are run, rate limits have not been exceeded. Nevertheless, the test fails due to an exceeded rate limit. I conclude from this that the rate limit exceeded in the test is a "secondary" rate limit which is not reported in the rate limit queries. |
Limits are shown in Warnings summary whether or not the test succeeds. Limits are also shown in a log message if the test fails. Update all test workflows Syntax error? Test getting rate limits during workflow
So I've discovered a few things
But I think that the changes in this PR will fix the problem. One thing I'm curious about is whether unauthenticated requests are counted against individual IP addresses or against the repository. If the repository, then we could have an issue if more Spyder instances in the wild are sending unauthenticated requests. |
Oh yep, "Unauthenticated requests are associated with the originating IP address". Okay, I think we're good. |
I think we were just hitting the 60 count limit by having multiple (~5) PRs running tests in close proximity. |
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.
Thanks @mrclary for your help to solve this!
logger.info(f"Checking for updates from {url}") | ||
try: | ||
page = requests.get(url) | ||
page = requests.get(url, headers=headers) | ||
_rate_limits(page) |
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.
Is this really necessary for production code? Or only for testing?
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 motivation was for investigating the issue in our test suite. However, when our installers become mainstream, it may be prudent to have a debug log message in production in case users encounter a rate limit error. In production, headers={}
, then requests.get(url)
and requests.get(url, headers={})
are equivalent.
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.
Clarification: currently, only macOS and Windows 5.x standalone installers check Github for updates. When 6.0 is released, we could (hopefully) see much more widespread adoption of our installers, resulting in increased unauthorized requests to Github.
Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>
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.
Looks good to me now, thanks @mrclary!
GITHUB_TOKEN
to make authorized user requests against GitHub (CI)
Some unit tests intermittently fail due to
403 Client Error: rate limit exceeded for url: https://api.github.com/repos/spyder-ide/spyder/releases
.Rate Limit Failure
What is the root cause of this and how do we resolve it?