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

Dev: Avoid GitHub API rate limits with basic auth #111

Closed
rads opened this issue Aug 16, 2022 · 8 comments
Closed

Dev: Avoid GitHub API rate limits with basic auth #111

rads opened this issue Aug 16, 2022 · 8 comments

Comments

@rads
Copy link
Collaborator

rads commented Aug 16, 2022

Quick example for dev:

;; babashka.neil.curl

(def dev-github-user (System/getenv "BABASHKA_NEIL_DEV_GITHUB_USER"))
(def dev-github-token (System/getenv "BABASHKA_NEIL_DEV_GITHUB_TOKEN"))

(def curl-opts
  (merge {:throw false
          :compressed (not (fs/windows?))}
         (when (and dev-github-user dev-github-token)
           {:basic-auth [dev-github-user dev-github-token]})))
$ bb -e "(-> ((requiring-resolve 'babashka.neil.curl/curl-get-json) \"https://api.github.com/rate_limit\") :rate)"
{:limit 60, :used 8, :remaining 52, :reset 1660690276}

$ export BABASHKA_NEIL_DEV_GITHUB_USER=rads
$ export BABASHKA_NEIL_DEV_GITHUB_TOKEN=op://Private/Github/access-token
$ op run -- bb -e "(-> ((requiring-resolve 'babashka.neil.curl/curl-get-json) \"https://api.github.com/rate_limit\") :rate)"
{:limit 5000, :used 24, :remaining 4976, :reset 1660688942}

This would be the simplest solution to avoid the GitHub rate limits when running bb tests locally. Using basic auth raises the limit from 60 requests per hour to 5000.

The rate limit seems to be a common pitfall for new contributors to this project so we may want to add something to the README as well. Maybe some better error handling for the rate-limiting HTTP response too.

If this is useful outside of dev we could add it as an command line argument, but I think the env variable would solve the immediate problem since hitting the rate limit is usually due to running bb tests. I don't think we should support passwords, only Personal Access Tokens.

@rads rads mentioned this issue Aug 16, 2022
2 tasks
@teodorlu
Copy link
Contributor

The rate limit seems to be a common pitfall for new contributors to this project so we may want to add something to the README as well. Maybe some better error handling for the rate-limiting HTTP response too.

Is it possible to check whether we've hit the rate limit directly in the tests, and stop with a good error message?

@borkdude
Copy link
Contributor

This is how I use GITHUB_TOKEN in gh-release-artifact: https://github.com/borkdude/gh-release-artifact/blob/cf082df46a648178d1904e9cbcb787d8136a35c6/src/borkdude/gh_release_artifact.clj#L19, if anyone wants to put together a PR.

@rads
Copy link
Collaborator Author

rads commented Aug 17, 2022

@borkdude: It seems like it could lead to confusion if we just start using whatever GITHUB_TOKEN is defined without documenting that as part of the public neil behavior. I expect using a GitHub token would be gated by a --dev mode or simple prefixing like I did in my example (BABASHKA_NEIL_DEV_GITHUB_TOKEN).

Otherwise if we do want people to be able to use their own GITHUB_TOKEN for non-dev neil usage, we can document that. I just want to make sure we aren't doing this implicitly.

What do you think?

@borkdude
Copy link
Contributor

Sounds good!

@teodorlu
Copy link
Contributor

teodorlu commented Oct 14, 2022

We merged @rads' proposed solution in #110. BABASHKA_NEIL_DEV_GITHUB_TOKEN is now used if set.

Though I don't think this behavior is documented anywhere.

Cc @russmatney

@russmatney
Copy link
Contributor

Indeed, it was helpful immediately to unblock my local testing!

I added some thoughts on caching requests as a potential solution for local dev/testing (and faster neil performance) here: #127 (comment) i can port this over to a new discussion/issue if there's interest, feel free to ping me!

@rads
Copy link
Collaborator Author

rads commented Oct 14, 2022

Thanks for taking care of this! Since this issue is specific to the dev experience, this is now resolved from my point of view.

If we want to expand the scope here to improve the experience for end users too, let’s create a new issue for that.

@teodorlu
Copy link
Contributor

Sounds good 👍

I'm happy with the current solution as-is.

I suspect that for end users, many won't appreciate extra mandatory setup.

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

No branches or pull requests

4 participants