Skip to content

chore: update @octokit/rest from v16 to v17 #371

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.

Sign up for GitHub

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

Merged
Trott merged 2 commits into from
Mar 2, 2023
Merged

chore: update @octokit/rest from v16 to v17 #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f9f509
chore: replace deprecated .hasLastPage() with pagination
Trott Feb 27, 2023
ea8a73b
chore: update @octokit/rest from v16 to v17
Trott Feb 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 6 additions & 14 deletions lib/push-jenkins-update.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,16 @@ function findPrInRef (gitRef) {
return parseInt(gitRef.split('/')[2], 10)
}

async function findLatestCommitInPr (options, pageNumber = 1) {
const res = await githubClient.pulls.listCommits({
async function findLatestCommitInPr (options) {
const { Octokit } = require('@octokit/rest')
const githubClient = new Octokit({})
Copy link
Copy Markdown
Member

@phillipj phillipj Mar 5, 2023

Choose a reason for hiding this comment

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

Was it deliberate to not use githubClient from line 3?

Copy link
Copy Markdown
Member Author

@Trott Trott Mar 5, 2023

Choose a reason for hiding this comment

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

That was probably a blunder. Sorry about that!

Copy link
Copy Markdown
Member

@richardlau richardlau Mar 6, 2023

Choose a reason for hiding this comment

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

Is this why we're hitting rate limits? The github-bot logs now have lots of "API rate limit exceeded for 23.253.100.79. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)" errors, e.g.

{"name":"bot","hostname":"infra-rackspace-debian8-x64-1","pid":15899,"req_id":"e62c49d0-bbaa-11ed-9410-77b81a7c3334","identifier":"node-test-linux-ubuntu1804-64","event":"end","level":50,"err":{"message":"API rate limit exceeded for 23.253.100.79. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","name":"HttpError","stack":"HttpError: API rate limit exceeded for 23.253.100.79. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)\n    at /home/iojs/github-bot/node_modules/@octokit/request/dist-node/index.js:86:21\n    at runMicrotasks (<anonymous>)\n    at processTicksAndRejections (internal/process/task_queues.js:93:5)\n    at async Object.next (/home/iojs/github-bot/node_modules/@octokit/plugin-paginate-rest/dist-node/index.js:112:28)\n    at async findLatestCommitInPr (/home/iojs/github-bot/lib/push-jenkins-update.js:88:23)","code":403},"msg":"Error while handling Jenkins end event","time":"2023-03-05T23:10:20.025Z","v":0}

Copy link
Copy Markdown
Member Author

@Trott Trott Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ugh, sending a PR momentarily. I wouldn't have realized this would caused this kind of problem. Sorry about that.

Copy link
Copy Markdown
Member

@phillipj phillipj Mar 6, 2023

Choose a reason for hiding this comment

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

Thanks for opportunity to learn why Github requests better be authenticated 👍🏻

const paginateOptions = githubClient.pulls.listCommits.endpoint.merge({
owner: options.owner,
repo: options.repo,
pull_number: options.pr,
page: pageNumber,
per_page: 100
pull_number: options.pr
})
const commitMetas = await githubClient.paginate(paginateOptions)

const commitMetas = res.data || []
const lastPageURL = githubClient.hasLastPage(res)
if (lastPageURL) {
return findLatestCommitInPr(options, pageNumberFromURL(lastPageURL))
}
const lastCommitMeta = commitMetas.pop()
const lastCommit = {
sha: lastCommitMeta.sha,
Expand Down Expand Up @@ -123,10 +119,6 @@ function validate (payload) {
return ['identifier', 'status', 'message', 'commit', 'url'].every(isString)
}

function pageNumberFromURL (githubUrl) {
return new URL(githubUrl).searchParams.get('page')
}

exports.validate = validate
exports.pushStarted = pushStarted
exports.pushEnded = pushEnded
Expand Down
315 changes: 129 additions & 186 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"private": true,
"license": "MIT",
"dependencies": {
"@octokit/rest": "^16.43.2",
"@octokit/rest": "^17.11.2",
"aigle": "^1.14.1",
"async": "3.2.4",
"basic-auth": "^2.0.1",
Expand Down
2,335 changes: 2,335 additions & 0 deletions test/_fixtures/pull-request-commits-page-1.json
View file
Open in desktop

Large diffs are not rendered by default.

2,373 changes: 2,373 additions & 0 deletions test/_fixtures/pull-request-commits-page-2.json
View file
Open in desktop

Large diffs are not rendered by default.

2,354 changes: 2,354 additions & 0 deletions test/_fixtures/pull-request-commits-page-3.json
View file
Open in desktop

Large diffs are not rendered by default.

1,109 changes: 1,109 additions & 0 deletions test/_fixtures/pull-request-commits-page-4.json
View file
Open in desktop

Large diffs are not rendered by default.

78 changes: 0 additions & 78 deletions test/_fixtures/pull-requests-commits-page-1.json
View file
Open in desktop

This file was deleted.

78 changes: 0 additions & 78 deletions test/_fixtures/pull-requests-commits-page-104.json
View file
Open in desktop

This file was deleted.

35 changes: 22 additions & 13 deletions test/unit/push-jenkins-update.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,41 @@ const { findLatestCommitInPr } = require('../../lib/push-jenkins-update')

const nock = require('nock')
const readFixture = require('../read-fixture')
const { ignoreQueryParams } = require('../common')

tap.test('findLatestCommitInPr: paginates results when more than 100 commits in a PR', async (t) => {
const commitsFixturePage1 = readFixture('pull-requests-commits-page-1.json')
const commitsFixturePage104 = readFixture('pull-requests-commits-page-104.json')
const commitsFixturePage1 = readFixture('pull-request-commits-page-1.json')
const commitsFixturePage2 = readFixture('pull-request-commits-page-2.json')
const commitsFixturePage3 = readFixture('pull-request-commits-page-3.json')
const commitsFixturePage4 = readFixture('pull-request-commits-page-4.json')
Copy link
Copy Markdown
Member

@phillipj phillipj Mar 5, 2023

Choose a reason for hiding this comment

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

Hooray for tests being maintained and adjusted accordingly

const owner = 'nodejs'
const repo = 'node'
const pr = 9745

const headers = {
Link: '<https://api.github.com/repos/nodejs/node/pulls/9745/commits?page=104>; rel="last"'
}
const firstPageScope = nock('https://api.github.com')
.filteringPath(ignoreQueryParams)
.get(`/repos/${owner}/${repo}/pulls/${pr}/commits`)
.reply(200, commitsFixturePage1, headers)
.reply(200, commitsFixturePage1, { link: '<https://api.github.com/repositories/27193779/pulls/9745/commits?page=2>; rel="next", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=4>; rel="last"' })

const lastPageScope = nock('https://api.github.com')
.get(`/repos/${owner}/${repo}/pulls/${pr}/commits`)
.query({ page: 104, per_page: 100 })
.reply(200, commitsFixturePage104)
const secondPageScope = nock('https://api.github.com')
.get(`/repositories/27193779/pulls/${pr}/commits`)
.query({ page: 2 })
.reply(200, commitsFixturePage2, { link: '<https://api.github.com/repositories/27193779/pulls/9745/commits?page=1>; rel="prev", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=3>; rel="next", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=4>; rel="last", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=1>; rel="first"' })

const thirdPageScope = nock('https://api.github.com')
.get(`/repositories/27193779/pulls/${pr}/commits`)
.query({ page: 3 })
.reply(200, commitsFixturePage3, { link: '<https://api.github.com/repositories/27193779/pulls/9745/commits?page=2>; rel="prev", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=4>; rel="next", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=4>; rel="last", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=1>; rel="first"' })

const fourthPageScope = nock('https://api.github.com')
.get(`/repositories/27193779/pulls/${pr}/commits`)
.query({ page: 4 })
.reply(200, commitsFixturePage4, { link: '<https://api.github.com/repositories/27193779/pulls/9745/commits?page=3>; rel="prev", <https://api.github.com/repositories/27193779/pulls/9745/commits?page=1>; rel="first"' })

t.plan(1)

const commit = await findLatestCommitInPr({ owner, repo, pr })
t.equal(commit.sha, 'c1aa949064892dbe693750686c06f4ad5673e577')
firstPageScope.done()
lastPageScope.done()
secondPageScope.done()
thirdPageScope.done()
fourthPageScope.done()
})