Skip to content

refactor: to use octokit throttling plugin #378

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

Closed
wants to merge 7 commits into from

Conversation

moltar
Copy link

@moltar moltar commented Jun 1, 2021

This is still WIP, but I need help.

  1. Some of tests are failing and I cannot explain.
  2. A set of tests in get-client is failing, but I am not sure if they still make sense, given that we are switching to the plugin, which handles all of the logic. See below. Thoughts?
  3. Tests started to run much slower after adding the plugins, which makes me think that maybe they are bypassing nock and hitting live endpoints? Or maybe it is just the throttling/retry that does that.
✖ get-client › Use the global throttler for all endpoints Rejected promise returned by test
✖ get-client › Use the same throttler for endpoints in the same rate limit group Rejected promise returned by test
✖ get-client › Use different throttler for read and write endpoints Rejected promise returned by test
✖ get-client › Use the same throttler when retrying Rejected promise returned by test

Closes #377

@gr2m
Copy link
Member

gr2m commented Jun 1, 2021

you can remove all tests for throttling as we don't need to test internals of the throttle plugin, it's well tested already

@moltar
Copy link
Author

moltar commented Jun 2, 2021

Removed the throttling tests.

Still a few failures left.

One peculiar one is in test/verify.test.js.

Looks like something with nock, but can't yet figure it out.

> nyc ava -v "test/verify.test.js" "-T" "100s"


  ✖ Throw SemanticReleaseError for invalid token Rejected promise returned by test
  ─

  Throw SemanticReleaseError for invalid token

  Rejected promise returned by test. Reason:

  HttpError (RequestError) {
    headers: {},
    request: {
      headers: {
        accept: 'application/vnd.github.v3+json',
        authorization: 'token [REDACTED]',
        'user-agent': 'octokit-rest.js/18.5.3 octokit-core.js/3.4.0 Node.js/14.15.5 (darwin; x64)',
      },
      method: 'GET',
      request: {
        agent: undefined,
        hook: Function bound bound register {},
        retries: 3,
        retryAfter: 16,
        retryCount: 3,
      },
      url: 'https://api.github.com/repos/test_user/test_repo',
    },
    status: 500,
    message: `request to https://api.github.com/repos/test_user/test_repo failed, reason: Nock: No match for request {␊
      "method": "GET",␊
      "url": "https://api.github.com/repos/test_user/test_repo",␊
      "headers": {␊
        "accept": [␊
          "application/vnd.github.v3+json"␊
        ],␊
        "user-agent": [␊
          "octokit-rest.js/18.5.3 octokit-core.js/3.4.0 Node.js/14.15.5 (darwin; x64)"␊
        ],␊
        "authorization": [␊
          "token github_token"␊
        ],␊
        "accept-encoding": [␊
          "gzip,deflate"␊
        ],␊
        "connection": [␊
          "close"␊
        ]␊
      }␊
    }`,
  }

  › }
  › node_modules/@octokit/request/dist-src/fetch-wrapper.js:91:15
  › Job.doExecute (node_modules/bottleneck/light.js:405:18)

Putting this on pause for a bit. If anyone wants to jump in, feel free.

@gr2m
Copy link
Member

gr2m commented Jun 2, 2021

Still a few failures left.

One peculiar one is in test/verify.test.js.

Looks like something with nock, but can't yet figure it out.

I'll have a look

@gr2m gr2m self-assigned this Jun 3, 2021
auth: `token ${githubToken}`,
baseUrl,
request: {
retries: RETRY_CONF.retries,
Copy link
Member

Choose a reason for hiding this comment

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

why did you set that? I don't think that is right, it's set dynamically by the throttling plugin.

Removing it fixes the "Throw SemanticReleaseError for invalid token" error

Copy link
Author

Choose a reason for hiding this comment

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

I think other tests were failing when I did not set that. Tbh, it was through denial and error.

@gr2m gr2m removed their assignment Jun 3, 2021
@gr2m
Copy link
Member

gr2m commented Jun 3, 2021

I've pushed a few changes, I resolved two failing tests but gotta go now. Can you check if you can resolve the rest?

@moltar
Copy link
Author

moltar commented Jun 3, 2021

I've pushed a few changes, I resolved two failing tests but gotta go now. Can you check if you can resolve the rest?

Awesome! Good teamwork 😁

Any idea why nock debugging is not working?

DEBUG=nock:* npm t -- test/fail.test.js

I get no debug output at all from nock.

@moltar
Copy link
Author

moltar commented Jun 3, 2021

Ugh, never mind, figured it out.

nock seems to deviate from the node convetion of using : as a separator.

The right way is to use a dot . -- DEBUG=nock.*.

@mikehardy
Copy link

mikehardy commented Jan 17, 2022

Apologies if this is perceived as spam, but just wanted to drop a note of thanks for starting this work, and hopefully it can merge at some point - I just had a busted release from github's secondary rate limits on a project 😭 - the release itself went out, but the issue / PR comments didn't go, so it wasn't tragic, but it wasn't as beautiful as the releases normally are :-). Cheers

achingbrain added a commit to achingbrain/github that referenced this pull request Apr 8, 2022
Refactors the client to use [@octokit/plugin-retry](https://www.npmjs.com/package/@octokit/plugin-retry)
and [@octokit/plugin-throttling](https://www.npmjs.com/package/@octokit/plugin-throttling)
as GitHub occasionally changes it's API and these plugins can abstract
those changes away from this module.

- Removes `lib/definitions/rate-limit.js`
- Adds `lib/definitions/retry.js` and `lib/definitions/throttle.js` to handle retry/throttle settings
- Updates tests to be more like GitHub (returing the correct rate limit response headers, etc)

Fixes semantic-release#299 and semantic-release/semantic-release#2204
See also semantic-release#480 and semantic-release#378
@gr2m
Copy link
Member

gr2m commented May 27, 2023

@gr2m gr2m closed this May 27, 2023
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.

Hitting rate limits
3 participants