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

Avoid exceeding the GitHub REST API rate limit #150

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

dscho
Copy link
Collaborator

@dscho dscho commented Jan 9, 2023

There was a recent uptick of probems with symptoms like this:

Error: API rate limit exceeded for 172.176.196.48. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)

Let's follow that advice and use authenticated REST API requests.

There was a recent uptick of problems with the `limit-access-to-actor`
feature because it uses an unauthenticated REST API call (and the rate
limiting is based on IP, i.e. hosted GitHub build agents will run into
this _a lot_). The symptom looks like this:

	Error: API rate limit exceeded for 172.176.196.48. (But here's
	the good news: Authenticated requests get a higher rate limit.
	Check out the documentation for more details.)

Let's imitate how `actions/checkout` uses authenticated REST API calls
(see https://github.com/actions/checkout/blob/v3.3.0/action.yml#L24):
add an "input" variable with the correct default (and users are
expected to never _actually_ to provide that variable).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
src/helpers.js Outdated
shell: true,
env: {
...process.env,
HOMEBREW_GITHUB_API_TOKEN: core.getInput('github-token')
Copy link
Owner

@mxschmitt mxschmitt Jan 9, 2023

Choose a reason for hiding this comment

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

Suggested change
HOMEBREW_GITHUB_API_TOKEN: core.getInput('github-token')
...(core.getInput('github-token') ? { HOMEBREW_GITHUB_API_TOKEN: core.getInput('github-token') } : undefined)

was thinking about that maybe. Otherwise it does not get forwarded. So the user can set it on his own as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was under the impression that spawn() ignores env keys with undefined values? Let me try this.

Copy link
Owner

Choose a reason for hiding this comment

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

we are spreading with undefined, which means that nothing gets spreaded inside the existing ...process.env object. aka. a noop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, if the env object contains a key whose value is undefined, it isn't set:

$ node -e "const { spawn } = require('child_process'); const proc = spawn('set -x; env | grep HOMEBREW_GITHUB_API_TOKEN >&2', { shell: true, env: { HOMEBREW_GITHUB_API_TOKEN: undefined } }); proc.stdout.on('data', (data) => { process.stdout.write(data); }); proc.stderr.on('data', (data) => { process.stderr.write(data); });"
+ grep HOMEBREW_GITHUB_API_TOKEN
+ env

However, I guess the problem here is that core.getInput() would maybe always return a string? How about this instead, then?

Suggested change
HOMEBREW_GITHUB_API_TOKEN: core.getInput('github-token')
HOMEBREW_GITHUB_API_TOKEN: core.getInput('github-token') || undefined

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fixes mxschmitt#69

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@mxschmitt mxschmitt merged commit 7d7441a into mxschmitt:master Jan 9, 2023
@dscho dscho deleted the use-github-token branch January 9, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants