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

fetch-configlet: make authenticated requests #107

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

ee7
Copy link
Member

@ee7 ee7 commented Jan 6, 2023

(Edited after discussion below):

The configlet CI check in a track repo would sometimes fail due to a rate limiting error. That happened because the check runs this repo's fetch-configlet script, which was making unauthenticated requests (for which the rate limit is low).

Make curl perform authenticated requests, which have a higher rate limit of 1000 requests per hour per repository. The GITHUB_TOKEN environment variable is not set when the script is run, because we're in a composite action. So this commit sets GH_TOKEN, as advised by an error that occurs when using gh in the same context without setting a token:

    gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable. Example:
      env:
        GH_TOKEN: ${{ github.token }}

Note that the fetch-configlet script present in every track repo (and synced via exercism/org-wide-files) is designed for local use, but already uses GITHUB_TOKEN when available. For robust CI, we want to keep using a separate version in a non-track repo for now.

Closes: #101


I think this works. I've tested with:

But if we're happy to merge, we should immediately trigger a configlet job manually on a track, and revert the change in this repo if it fails.

@@ -26,6 +26,8 @@ jobs:

- name: Fetch configlet
uses: exercism/github-actions/configlet-ci@main
env:
GH_TOKEN: ${{ github.token }}
Copy link
Contributor

@SaschaMann SaschaMann Jan 7, 2023

Choose a reason for hiding this comment

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

Docs tell me this should be GITHUB_TOKEN: https://docs.github.com/en/actions/using-workflows/using-github-cli-in-workflows but I was also under the impression that GITHUB_TOKEN is already set by default and is only needed if one wants to use a non-default token, so we might not need to set it at all.

Since you tested it, I assume it works, just pointing this out so we can be sure. Feel free to ignore my comment if you've already checked it.

We could test if it works as intended by temporarily adding a call to the rate limit endpoint for debugging:

gh api \
  -H "Accept: application/vnd.github+json" \
  /rate_limit

which should return something just under 1000 requests remaining if the authentication worked properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Docs tell me this should be GITHUB_TOKEN: https://docs.github.com/en/actions/using-workflows/using-github-cli-in-workflows but I was also under the impression that GITHUB_TOKEN is already set by default and is only needed if one wants to use a non-default token, so we might not need to set it at all.

That's what I thought too. But when I tested the second commit in this PR, CI failed with:

gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable. Example:
  env:
    GH_TOKEN: ${{ github.token }}

The change to advise using github.token rather than secrets.GITHUB_TOKEN is recent.

That commit closed cli/cli#6534, which mentions:

There is absolutely a secrets.GITHUB_TOKEN https://docs.github.com/en/actions/security-guides/automatic-token-authentication#using-the-github_token-in-a-workflow

Though, I would guess, it's only available to top-level workflows and not deep within composite actions. If github.token is more versatile, I agree with your proposed change to swap the former with the latter, because when they both exist, they should have identical values.

So it's tricky. It looks like GITHUB_TOKEN isn't set at the time we call the script.

We could test if it works as intended by temporarily adding a call to the rate limit endpoint for debugging

Nice idea. I'll try it later. But I'm led to believe that gh release download never makes an unauthenticated request, and fails immediately otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I thought too. But when I tested the second commit in this PR, CI failed with:

Love their consistency 🥴

Anyway, in that case consider my comment moot

Copy link
Member

Choose a reason for hiding this comment

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

I noticed the github.token reference too.

Nice idea. I'll try it later. But I'm led to believe that gh release download never makes an unauthenticated request, and fails immediately otherwise.

Interesting. One way in which we could check that would be to check the rate limit before and after downloading the release. This can be done via gh api /rate_limit. @ee7 Could you try that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you try that?

Done: https://github.com/ee7/exercism-nim/actions/runs/3883336081/jobs/6624523492#step:3:17

Immediately before running the script:

{
  "resources": {
    "core": {
      "limit": 1000,
      "used": 0,
      "remaining": 1000,
      "reset": 1673357434
    },
  ...
}

Immediately after:

{
  "resources": {
    "core": {
      "limit": 1000,
      "used": 1,
      "remaining": 999,
      "reset": 1673357434
    },
  ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Perfect!

@ee7 ee7 changed the title fetch-configlet: use gh, not curl fetch-configlet: make authenticated requests Jan 10, 2023
The configlet CI check in a track repo would sometimes fail due to a
rate limiting error. That happened because the check runs this repo's
fetch-configlet script, which was making unauthenticated requests (for
which the rate limit is low).

Make curl perform authenticated requests, which have a higher rate limit
of 1000 requests per hour per repository [1]. The GITHUB_TOKEN
environment variable is not set when the script is run, because we're in
a composite action [2]. So this commit sets GH_TOKEN, as advised by an
error that occurs when using `gh` in the same context without setting a
token [3]:

    gh: To use GitHub CLI in a GitHub Actions workflow, set the GH_TOKEN environment variable. Example:
      env:
        GH_TOKEN: ${{ github.token }}

Note that the fetch-configlet script present in every track repo (and
synced via exercism/org-wide-files) is designed for local use, but
already uses GITHUB_TOKEN when available. For robust CI, we want to keep
using a separate version in a non-track repo for now.

[1] https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limits-for-requests-from-github-actions
[2] https://www.github.com/cli/cli/issues/6534
[3] https://www.github.com/cli/cli/commit/fe485c14cc1b

Closes: #101
@ee7
Copy link
Member Author

ee7 commented Jan 10, 2023

I decided to split this PR.

Follow-ups:

The end state after the follow-ups is the same as what was successfully tested in the past.

I've tested this PR again, so this should be good to merge with the cavaet from the top post:

if we're happy to merge, we should immediately trigger a configlet job manually on a track, and revert the change in this repo if it fails.

@ErikSchierboom ErikSchierboom merged commit d1779e2 into main Jan 10, 2023
@ErikSchierboom ErikSchierboom deleted the fetch-configlet-gh branch January 10, 2023 12:53
@ErikSchierboom
Copy link
Member

fapdash added a commit to fapdash/exercism--emacs-lisp that referenced this pull request Feb 1, 2023
The configlet-ci action switched to using the gh cli command
and needs to have ~GH_TOKEN~ set to work now.

This sets the token in the same way as upstream does it at
https://github.com/exercism/github-actions/blob/a3f83169a9218d28cae3c04d70522f0519d92d46/.github/workflows/configlet.yml#L29-L33

Previous error was:

> The GH_TOKEN environment variable is not set

See exercism/github-actions#107 for more
context
fapdash added a commit to fapdash/exercism--emacs-lisp that referenced this pull request Feb 1, 2023
The configlet-ci action switched to using the gh cli command
and needs to have `GH_TOKEN` set to work now.

This sets the token in the same way as upstream does it at
https://github.com/exercism/github-actions/blob/a3f83169a9218d28cae3c04d70522f0519d92d46/.github/workflows/configlet.yml#L29-L33

Previous error was:

> The GH_TOKEN environment variable is not set

See exercism/github-actions#107 for more
context
fapdash added a commit to fapdash/exercism--github-actions that referenced this pull request Feb 1, 2023
The configlet-ci action is using authenticated requests now.
This change adds the necessary environment variable definition
to the example in the README.

See
- exercism#109 and
- exercism#107
for more context.
fapdash added a commit to fapdash/exercism--github-actions that referenced this pull request Feb 1, 2023
The configlet-ci action is using authenticated requests now.
This change adds the necessary environment variable definition
to the example in the README.

See
- exercism#109 and
- exercism#107

for more context.
fapdash added a commit to fapdash/exercism--emacs-lisp that referenced this pull request Feb 1, 2023
The configlet-ci action switched to using the gh cli command and needs
to have `GH_TOKEN` set to work now.

This sets the token in the same way as upstream does it at
https://github.com/exercism/github-actions/blob/a3f83169a9218d28cae3c04d70522f0519d92d46/.github/workflows/configlet.yml#L29-L33

Previous error was:

> The GH_TOKEN environment variable is not set

See exercism/github-actions#107 for more context
fapdash added a commit to fapdash/exercism--common-lisp that referenced this pull request Feb 1, 2023
The configlet-ci action switched to using the gh cli command and needs
to have `GH_TOKEN` set to work now.

This sets the token in the same way as upstream does it at
https://github.com/exercism/github-actions/blob/a3f83169a9218d28cae3c04d70522f0519d92d46/.github/workflows/configlet.yml#L29-L33

Previous error was:

> The GH_TOKEN environment variable is not set

See exercism/github-actions#107 for more context
verdammelt pushed a commit to exercism/common-lisp that referenced this pull request Feb 1, 2023
The configlet-ci action switched to using the gh cli command and needs
to have `GH_TOKEN` set to work now.

This sets the token in the same way as upstream does it at
https://github.com/exercism/github-actions/blob/a3f83169a9218d28cae3c04d70522f0519d92d46/.github/workflows/configlet.yml#L29-L33

Previous error was:

> The GH_TOKEN environment variable is not set

See exercism/github-actions#107 for more context
ErikSchierboom pushed a commit that referenced this pull request Feb 2, 2023
The configlet-ci action is using authenticated requests now.
This change adds the necessary environment variable definition
to the example in the README.

See
- #109 and
- #107

for more context.
fapdash added a commit to exercism/emacs-lisp that referenced this pull request Feb 2, 2023
The configlet-ci action switched to using the gh cli command
and needs to have `GH_TOKEN` set to work now.

This sets the token in the same way as upstream does it at
https://github.com/exercism/github-actions/blob/a3f83169a9218d28cae3c04d70522f0519d92d46/.github/workflows/configlet.yml#L29-L33

Previous error was:

> The GH_TOKEN environment variable is not set

See exercism/github-actions#107 for more
context
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.

fetch-configlet: use a token
3 participants