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

GitHub Actions Checks: Submit the checks in batches #7

Merged
merged 1 commit into from
Feb 28, 2022

Conversation

vfonic
Copy link
Contributor

@vfonic vfonic commented Feb 26, 2022

This is needed because GitHub Actions Checks API limits the number of annotations to a maximum of 50 per API request. To create more than 50 annotations, you have to make multiple requests to the Update a check run endpoint.
https://docs.github.com/en/developers/apps/guides/creating-ci-tests-with-the-checks-api#step-24-collecting-rubocop-errors

Closes #6
Action fails with: Error: Invalid request. No more than 50 items are allowed; 404 were supplied.

More people having this issue (just 2 random repos, but there are more):

Their fixes are similar to mine:

Test steps:

  1. In any repo where theme-check-action is used (preferably repo with more than 50 theme-check issues), switch to vfonic fork:
name: Theme Check
on: [push]
jobs:
  theme-check:
    name: Theme Check
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - name: Theme Check
-        uses: shopify/theme-check-action@v1
+        uses: vfonic/theme-check-action@batch-octokit-annotations
        with:
          theme_root: '.'
          token: ${{ github.token }}
  1. Push the change to some branch and check how the Action runs

I believe, but I didn't properly test to confirm, that I'm making these Checks update requests in parallel. This means that, if there are 2000 errors, this will make 2000 / 50 = 40 requests at the same time.

I tested this if it will timeout or get rate limited, and it doesn't. It does slow down as it needs to make all those requests. It slows down by approx 1s per 100 annotations (it seems to be able to make 2 requests per second(?), but I'm not sure as I'm supposedly making all of these requests at once?).
I ran this action on a repo with 2207 theme-check issues.

Here are my findings:
There's an undocumented limit of 1000 annotations. I searched GH Actions documentation and couldn't find anything about the upper limit, only 50 annotations limit per request.

I tested with updating Checks in parallel and consecutively.

Running in parallel:

await Promise.all(
    annotationsChunks.map(async (annotations) =>
      octokit.checks.update({
        // ...

Running consecutively:

annotationsChunks.forEach(
    async (annotations) =>
      await octokit.checks.update({
        // ...
Results
Parallel Consecutively
Errors reported Screen Shot 2022-02-26 at 12 40 07 Errors reported Screen Shot 2022-02-26 at 12 36 57
Action run time and number of annotations Screen Shot 2022-02-26 at 12 40 25 Action run time and number of annotations Screen Shot 2022-02-26 at 12 37 15

There seems to be a race condition when doing Checks update. It always reports 1000 annotations. However, it always reports 1000 different annotations.
I tested this for both "parallel job" and "consecutive job" (code posted above).

It only seems that "parallel job" runs some ~10 seconds faster so I kept that code here in the PR.

In my defense for having a theme with 2000+ errors...YOU'RE WELCOME! (for the great testing material! 😅 )

This is needed because GitHub Actions Checks API limits the number of annotations to a maximum of 50 per API request. To create more than 50 annotations, you have to make multiple requests to the Update a check run endpoint.
https://docs.github.com/en/developers/apps/guides/creating-ci-tests-with-the-checks-api#step-24-collecting-rubocop-errors
});
await Promise.all(
annotationsChunks.map(async (annotations) =>
octokit.checks.update({
Copy link
Contributor Author

@vfonic vfonic Feb 26, 2022

Choose a reason for hiding this comment

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

This used to be await octokit.checks.update({, but now I'm immediately returning the promise and awaiting for all promises to finish with Promise.all.

The above async is needed for the below await exec('theme-check --print....

@vfonic vfonic force-pushed the batch-octokit-annotations branch from 84d427b to 5de2392 Compare February 26, 2022 06:07
@charlespwd
Copy link
Contributor

In my defense for having a theme with 2000+ errors...YOU'RE WELCOME! (for the great testing material! 😅)

😂😂😂 THANK YOU! I'll push that and try to look into the limits.

I tested this if it will timeout or get rate limited, and it doesn't. It does slow down as it needs to make all those requests. It slows down by approx 1s per 100 annotations (it seems to be able to make 2 requests per second(?), but I'm not sure as I'm supposedly making all of these requests at once?).

I would hope their SDK handles this internally. Looks like they have a plugin for that, but it doesn't show up in yarn.lock or node_modules.

@charlespwd
Copy link
Contributor

Looks like you are absolutely right about everything.

Added the plugin, didn't hit the limits with 2000 errors. Looks like they have a pretty large window for the limit. Something like X calls / hour.

I'll push it anyway, just in case someone has a lower limit and ends up hitting them. Thanks again for the contribution :D

@charlespwd charlespwd merged commit 378528d into Shopify:main Feb 28, 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.

Action fails with: Error: Invalid request. No more than 50 items are allowed; 404 were supplied.
2 participants