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

Allow passing in GH token #31

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

axmmisaka
Copy link

GH API might rate-limit us which is a concern when running CI that fires a bunch of curl requests unless we provide a GH token. This PR allows us to do so.

@lhstrh
Copy link
Member

lhstrh commented Feb 23, 2025

Shouldn't the CI script then also pass in the auth token?

@axmmisaka
Copy link
Author

Shouldn't the CI script then also pass in the auth token?

I have a diff here: axmmisaka/action-setup-lf@7f60b1e
But since GitHub Actions is a bit unpredictable I think we might need some more stress testing to make the PR...

@lhstrh
Copy link
Member

lhstrh commented Feb 24, 2025

How did you even figure out that this was a problem? 🤣 Note that this fix will only work in CI, not in actual use of the installer. If we don't want to be rate limited, we should probably just serve this differently. Could we just serve it via Pages? I imagine that wouldn't be rate limited as aggressively.

@axmmisaka
Copy link
Author

axmmisaka commented Feb 25, 2025

How did you even figure out that this was a problem?

I tweaked the script to have set -euo pipefail and it shows it fails at resp=$(curl ...) part; I got rid of the -s in curl and it appears that we got 403/429, main reason being https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28

https://github.com/mxschmitt/action-tmate this is the ugliest form of debugging but works nicely

Note that this fix will only work in CI, not in actual use of the installer.

One use case I could think of is if people want to install LF onto a cluster of machines with this script (they shouldn't), they can supply their own token.

Could we just serve it via Pages? I imagine that wouldn't be rate limited as aggressively.

To be honest I'm not 100% sure...

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