Skip to content

Comments

refactor: to use octokit throttling plugin#378

Closed
moltar wants to merge 7 commits intosemantic-release:masterfrom
moltar:refactor/throttling
Closed

refactor: to use octokit throttling plugin#378
moltar wants to merge 7 commits intosemantic-release:masterfrom
moltar:refactor/throttling

Conversation

@moltar
Copy link

@moltar moltar commented Jun 1, 2021 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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
gr2m
gr2m reviewed Jun 3, 2021
View reviewed changes
lib/get-client.js Outdated
auth: `token ${githubToken}`,
baseUrl,
request: {
retries: RETRY_CONF.retries,
Copy link
Member

@gr2m gr2m Jun 3, 2021 •
edited by hearts bot
Loading

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

@moltar moltar Jun 3, 2021 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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 •
edited by hearts bot
Loading

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
@achingbrain
fix: use retry and throttle octokit plugins
fa3e386 
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
@achingbrain achingbrain mentioned this pull request Apr 8, 2022
Merged
@gr2m
Copy link
Member

gr2m commented May 27, 2023 •
edited by hearts bot
Loading

Done via https://github.com/semantic-release/github/releases/tag/v8.0.8

@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

Reviewers

@gr2m gr2m gr2m left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Hitting rate limits

3 participants

@moltar @gr2m @mikehardy