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

Extend docu regarding rate limit issues. #510

Merged
merged 7 commits into from Oct 31, 2022
Merged

Conversation

kasuteru
Copy link
Contributor

Description:
More precise documentation regarding rate limits.

Related issue:
#229

Check list:
Nothing required, only documentation changed.

I added subsections and also the full error message to make it easier to find, since I expect most people (like me) will stumble upon this after running into the rate limit message and might not even realise that it usually happens on GHES.

Once the change is in a release version, the full hash ID of the commit should be replace with the release version id.
Minor style issue.
@kasuteru kasuteru requested a review from a team September 27, 2022 15:39
docs/advanced-usage.md Outdated Show resolved Hide resolved

To get a higher rate limit, you can [generate a personal access token on github.com](https://github.com/settings/tokens/new) and pass it as the `token` input for the action:
`setup-python` comes pre-installed on the appliance with GHES if Actions is enabled. When dynamically downloading Python distributions, `setup-python` downloads distributions from [`actions/python-versions`](https://github.com/actions/python-versions) on github.com (outside of the appliance). These calls to `actions/python-versions` are by default made via unauthenticated requests, which are limited to [60 requests per hour per IP](https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting). If more requests are made within the time frame, then you will start to see rate-limit errors during downloading that looks like: `##[error]API rate limit exceeded for YOUR_IP. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)`.
Copy link
Contributor

Choose a reason for hiding this comment

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

##[error]API rate limit exceeded for YOUR_IP. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)

This code snippet is probably long enough to go in a block quote

docs/advanced-usage.md Outdated Show resolved Hide resolved
token: ${{ secrets.GH_GITHUB_COM_TOKEN }}
```

Requests should now be authenticated. To actually check this is however difficult, since caching as well as the hourly rate reset make it hard to know. Here is how you can check success:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just replace this section with a reference to the rate limit API. People can use this to confirm they're getting a higher rate limit: https://docs.github.com/en/rest/rate-limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem for us was that you have to check rate limit on the runner itself, and in our company, only admins can access the runners. So developers cannot check it. That's why I put the other way of enabling debugging, I imagine it is the some for other companies...

I will shorten the section a bit but would leave the section about debugging it without runner access in for now. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I was testing the token changes, I hit the API from the runner like this:

name: CI

on: [workflow_dispatch]

jobs:
  sample_build:
    runs-on: [ self-hosted ]
    steps:
      - name: "rate limit"
        run: |
          curl \
            -H "Accept: application/vnd.github.v3+json" \
            -H "Authorization: token $TOKEN" \
            https://api.github.com/rate_limit
        env:
          TOKEN: ${{ secrets.GH_DOTCOM_TOKEN }}

      - name: Clear the tool cache
        run: rm -rf /Users/runner/hostedtoolcache/Python/

      - name: Set up Python
        uses: brcrista/setup-python@patch-1
        with:
          token: ${{ secrets.GH_DOTCOM_TOKEN }}
          python-version: 3.9.12

      - name: "rate limit"
        run: |
          curl \
            -H "Accept: application/vnd.github.v3+json" \
            -H "Authorization: token $TOKEN" \
            https://api.github.com/rate_limit
        env:
          TOKEN: ${{ secrets.GH_DOTCOM_TOKEN }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't even think of that - of course, runner can simply be accessed inside an action...

Should I then reformulate it to use your way? Or take it out entirely? It is not strictly necessary I guess but I suspect there is others like me who aren't that experienced with github actions and might have a hard time figuring out a way to test it on their own... Could also just hotlink to your post.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not opposed to documenting it and think that this is the most straightforward way to test the rate limit. Maybe we can just say something like:

To verify that you are getting the higher rate limit, you can call GitHub's [rate limit API](https://docs.github.com/en/rest/rate-limit) from within your workflow ((example)[https://github.com/actions/setup-python/pull/443#issuecomment-1206776401]).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, and also adapted to official rollout of this feature - I think this is now ready to merge?

kasuteru and others added 2 commits September 28, 2022 09:28
Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
- Move debug text to own line
- Shorten section about checking auth
@kasuteru
Copy link
Contributor Author

@brcrista I have integrated your feedback and updated the PR.

Comment on lines 486 to 494
3. Since this functionality is not yet merged into any release version, for now, use the action with the hash below. Once this is merged into main, use the "normal" action like `@v4`. Also, change _python-version_ as needed.

```yml
uses: actions/setup-python@v4
with:
token: ${{ secrets.GH_DOTCOM_TOKEN }}
python-version: 3.11
- name: Set up Python
uses: actions/setup-python@98c991d13f3149457a7c1ac4083885d0d9db98e1
with:
python-version: 3.8
token: ${{ secrets.GH_GITHUB_COM_TOKEN }}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please change these lines because the major tag was updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is 4.3 correct, or was it available in earlier versions as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's correct. It became available at 4.3: https://github.com/actions/setup-python/releases/tag/v4.3.0

Adapt to reviewer input
Further adaptions to version change & typos.
docs/advanced-usage.md Outdated Show resolved Hide resolved
Copy link
Contributor

@brcrista brcrista left a comment

Choose a reason for hiding this comment

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

one small comment, otherwise looks good to me! Thanks @kasuteru

Edit: saw you enabled maintainers commit so I just applied my suggestion

@brcrista
Copy link
Contributor

@dmitry-shibanov once you approve, I think we're good to merge!

@dmitry-shibanov dmitry-shibanov merged commit af57b64 into actions:main Oct 31, 2022
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.

None yet

6 participants