diff --git a/CHANGELOG.md b/CHANGELOG.md index a2397caf..a0f4ae5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ All changes that impact users of this module are documented in this file, in the [Common Changelog](https://common-changelog.org) format with some additional specifications defined in the CONTRIBUTING file. This codebase adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased [patch] + +> Development of this release was supported by the [French Ministry for Foreign Affairs](https://www.diplomatie.gouv.fr/fr/politique-etrangere-de-la-france/diplomatie-numerique/) through its ministerial [State Startups incubator](https://beta.gouv.fr/startups/open-terms-archive.html) under the aegis of the Ambassador for Digital Affairs. + +### Fixed + +- Reduce GitHub API calls in issues reporter + ## 2.3.2 - 2024-10-23 _Full changeset and discussions: [#1112](https://github.com/OpenTermsArchive/engine/pull/1112)._ diff --git a/src/reporter/github.js b/src/reporter/github.js index 3da9b040..535d008a 100644 --- a/src/reporter/github.js +++ b/src/reporter/github.js @@ -28,6 +28,24 @@ export default class GitHub { const [ owner, repo ] = repository.split('/'); this.commonParams = { owner, repo }; + + this.issuesCache = new Map(); + this._issuesPromise = null; + } + + get issues() { + if (!this._issuesPromise) { + logger.info('Loading issues from GitHub…'); + this._issuesPromise = this.loadAllIssues(); + } + + return this._issuesPromise; + } + + clearCache() { + this.issuesCache.clear(); + this._issuesPromise = null; + logger.info('Issues cache cleared'); } async initialize() { @@ -53,6 +71,33 @@ export default class GitHub { } } + async loadAllIssues() { + try { + const issues = await this.octokit.paginate('GET /repos/{owner}/{repo}/issues', { + ...this.commonParams, + state: GitHub.ISSUE_STATE_ALL, + per_page: 100, + }); + + const onlyIssues = issues.filter(issue => !issue.pull_request); // Filter out pull requests since GitHub treats them as a special type of issue + + onlyIssues.forEach(issue => { + const cachedIssue = this.issuesCache.get(issue.title); + + if (!cachedIssue || new Date(issue.created_at) < new Date(cachedIssue.created_at)) { // Only work on the oldest issue if there are duplicates, in order to consolidate the longest history possible + this.issuesCache.set(issue.title, issue); + } + }); + + logger.info(`Cached ${onlyIssues.length} issues from the GitHub repository`); + + return this.issuesCache; + } catch (error) { + logger.error(`Failed to load issues: ${error.message}`); + throw error; + } + } + async getRepositoryLabels() { const { data: labels } = await this.octokit.request('GET /repos/{owner}/{repo}/labels', { ...this.commonParams }); @@ -68,6 +113,10 @@ export default class GitHub { }); } + async getIssue(title) { + return (await this.issues).get(title); + } + async createIssue({ title, description: body, labels }) { const { data: issue } = await this.octokit.request('POST /repos/{owner}/{repo}/issues', { ...this.commonParams, @@ -76,6 +125,8 @@ export default class GitHub { labels, }); + this.issuesCache.set(issue.title, issue); + return issue; } @@ -87,19 +138,9 @@ export default class GitHub { labels, }); - return updatedIssue; - } - - async getIssue({ title, ...searchParams }) { - const issues = await this.octokit.paginate('GET /repos/{owner}/{repo}/issues', { - ...this.commonParams, - per_page: 100, - ...searchParams, - }, response => response.data); - - const [issue] = issues.filter(item => item.title === title); // Since only one is expected, use the first one + this.issuesCache.set(updatedIssue.title, updatedIssue); - return issue; + return updatedIssue; } async addCommentToIssue({ issue, comment: body }) { @@ -114,25 +155,25 @@ export default class GitHub { async closeIssueWithCommentIfExists({ title, comment }) { try { - const openedIssue = await this.getIssue({ title, state: GitHub.ISSUE_STATE_OPEN }); + const issue = await this.getIssue(title); - if (!openedIssue) { + if (!issue || issue.state == GitHub.ISSUE_STATE_CLOSED) { return; } - await this.addCommentToIssue({ issue: openedIssue, comment }); - logger.info(`Added comment to issue #${openedIssue.number}: ${openedIssue.html_url}`); + await this.addCommentToIssue({ issue, comment }); + + const updatedIssue = await this.updateIssue(issue, { state: GitHub.ISSUE_STATE_CLOSED }); - await this.updateIssue(openedIssue, { state: GitHub.ISSUE_STATE_CLOSED }); - logger.info(`Closed issue #${openedIssue.number}: ${openedIssue.html_url}`); + logger.info(`Closed issue with comment #${updatedIssue.number}: ${updatedIssue.html_url}`); } catch (error) { - logger.error(`Failed to update issue "${title}": ${error.message}`); + logger.error(`Failed to close issue with comment "${title}": ${error.stack}`); } } async createOrUpdateIssue({ title, description, label }) { try { - const issue = await this.getIssue({ title, state: GitHub.ISSUE_STATE_ALL }); + const issue = await this.getIssue(title); if (!issue) { const createdIssue = await this.createIssue({ title, description, labels: [label] }); @@ -148,16 +189,13 @@ export default class GitHub { return; } - await this.updateIssue(issue, { - state: GitHub.ISSUE_STATE_OPEN, - labels: [ label, ...labelsNotManagedToKeep ], - }); - logger.info(`Updated issue #${issue.number}: ${issue.html_url}`); + const updatedIssue = await this.updateIssue(issue, { state: GitHub.ISSUE_STATE_OPEN, labels: [ label, ...labelsNotManagedToKeep ] }); + await this.addCommentToIssue({ issue, comment: description }); - logger.info(`Added comment to issue #${issue.number}: ${issue.html_url}`); + logger.info(`Updated issue with comment #${updatedIssue.number}: ${updatedIssue.html_url}`); } catch (error) { - logger.error(`Failed to update issue "${title}": ${error.message}`); + logger.error(`Failed to update issue "${title}": ${error.stack}`); } } } diff --git a/src/reporter/github.test.js b/src/reporter/github.test.js index 6ec78519..5aa21f18 100644 --- a/src/reporter/github.test.js +++ b/src/reporter/github.test.js @@ -12,10 +12,17 @@ describe('GitHub', function () { let MANAGED_LABELS; let github; + const EXISTING_OPEN_ISSUE = { number: 1, title: 'Opened issue', description: 'Issue description', state: GitHub.ISSUE_STATE_OPEN, labels: [{ name: 'location' }] }; + const EXISTING_CLOSED_ISSUE = { number: 2, title: 'Closed issue', description: 'Issue description', state: GitHub.ISSUE_STATE_CLOSED, labels: [{ name: '403' }] }; - before(() => { + before(async () => { MANAGED_LABELS = require('./labels.json'); github = new GitHub('owner/repo'); + nock('https://api.github.com') + .get('/repos/owner/repo/issues') + .query(true) + .reply(200, [ EXISTING_OPEN_ISSUE, EXISTING_CLOSED_ISSUE ]); + await github.clearCache(); }); describe('#initialize', () => { @@ -71,77 +78,88 @@ describe('GitHub', function () { }); describe('#createLabel', () => { - let scope; const LABEL = { name: 'new_label', color: 'ffffff' }; - before(async () => { - scope = nock('https://api.github.com') + afterEach(nock.cleanAll); + + it('creates the new label successfully', async () => { + const scope = nock('https://api.github.com') .post('/repos/owner/repo/labels', body => body.name === LABEL.name) .reply(200, LABEL); await github.createLabel(LABEL); + expect(scope.isDone()).to.be.true; }); - after(nock.cleanAll); + it('throws an error when creating a label fails', async () => { + nock('https://api.github.com') + .post('/repos/owner/repo/labels') + .reply(400, { message: 'Bad Request' }); - it('creates the new label', () => { - expect(scope.isDone()).to.be.true; + await expect(github.createLabel(LABEL)).to.be.rejected; }); }); describe('#createIssue', () => { let scope; let result; + const ISSUE = { title: 'New Issue', description: 'Description of the new issue', - labels: ['bug'], + labels: [{ name: 'bug' }], }; const CREATED_ISSUE = { number: 123, ...ISSUE }; before(async () => { scope = nock('https://api.github.com') - .post('/repos/owner/repo/issues', request => request.title === ISSUE.title && request.body === ISSUE.description && request.labels[0] === ISSUE.labels[0]) + .post('/repos/owner/repo/issues', request => + request.title === ISSUE.title && request.body === ISSUE.description && request.labels[0].name === ISSUE.labels[0].name) .reply(200, CREATED_ISSUE); result = await github.createIssue(ISSUE); }); - after(nock.cleanAll); + after(() => { + nock.cleanAll(); + github.issuesCache.delete(ISSUE.title); + }); it('creates the new issue', () => { expect(scope.isDone()).to.be.true; }); - it('returns the created issue', () => { + it('returns the newly created issue', () => { expect(result).to.deep.equal(CREATED_ISSUE); }); - }); - describe('#getIssue', () => { - let scope; - let result; + it('throws error when creating issue fails', async () => { + nock('https://api.github.com') + .post('/repos/owner/repo/issues') + .reply(400, { message: 'Bad Request' }); - const ISSUE = { number: 123, title: 'Test Issue' }; - const ANOTHER_ISSUE = { number: 124, title: 'Test Issue 2' }; + await expect(github.createIssue(ISSUE)).to.be.rejected; + }); + }); - before(async () => { - scope = nock('https://api.github.com') + describe('#getIssue', () => { + before(() => { + nock('https://api.github.com') .get('/repos/owner/repo/issues') .query(true) - .reply(200, [ ISSUE, ANOTHER_ISSUE ]); - - result = await github.getIssue({ title: ISSUE.title }); + .reply(200, [ EXISTING_OPEN_ISSUE, EXISTING_CLOSED_ISSUE ]); }); - after(nock.cleanAll); - - it('searches for the issue', () => { - expect(scope.isDone()).to.be.true; + context('when the issue exists in the cache', () => { + it('returns the cached issue', async () => { + expect(await github.getIssue(EXISTING_OPEN_ISSUE.title)).to.deep.equal(EXISTING_OPEN_ISSUE); + }); }); - it('returns the expected issue', () => { - expect(result).to.deep.equal(ISSUE); + context('when the issue does not exist in the cache', () => { + it('returns undefined', async () => { + expect(await github.getIssue('Non-existent Issue')).to.be.undefined; + }); }); }); @@ -163,35 +181,33 @@ describe('GitHub', function () { it('adds the comment to the issue', () => { expect(scope.isDone()).to.be.true; }); + + it('throws an error when adding a comment fails', async () => { + nock('https://api.github.com') + .post(`/repos/owner/repo/issues/${ISSUE_NUMBER}/comments`) + .reply(400, { message: 'Bad Request' }); + + await expect(github.addCommentToIssue({ issue: { number: ISSUE_NUMBER }, comment: COMMENT })).to.be.rejected; + }); }); describe('#closeIssueWithCommentIfExists', () => { after(nock.cleanAll); context('when the issue exists and is open', () => { - const ISSUE = { - number: 123, - title: 'Open Issue', - state: GitHub.ISSUE_STATE_OPEN, - }; let addCommentScope; let closeIssueScope; before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, [ISSUE]); - addCommentScope = nock('https://api.github.com') - .post(`/repos/owner/repo/issues/${ISSUE.number}/comments`) + .post(`/repos/owner/repo/issues/${EXISTING_OPEN_ISSUE.number}/comments`) .reply(200); closeIssueScope = nock('https://api.github.com') - .patch(`/repos/owner/repo/issues/${ISSUE.number}`, { state: GitHub.ISSUE_STATE_CLOSED }) + .patch(`/repos/owner/repo/issues/${EXISTING_OPEN_ISSUE.number}`, { state: GitHub.ISSUE_STATE_CLOSED }) .reply(200); - await github.closeIssueWithCommentIfExists({ title: ISSUE.title, comment: 'Closing comment' }); + await github.closeIssueWithCommentIfExists({ title: EXISTING_OPEN_ISSUE.title, comment: 'Closing comment' }); }); it('adds comment to the issue', () => { @@ -204,29 +220,19 @@ describe('GitHub', function () { }); context('when the issue exists and is closed', () => { - const ISSUE = { - number: 123, - title: 'Closed Issue', - state: GitHub.ISSUE_STATE_CLOSED, - }; let addCommentScope; let closeIssueScope; before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, []); - addCommentScope = nock('https://api.github.com') - .post(`/repos/owner/repo/issues/${ISSUE.number}/comments`) + .post(`/repos/owner/repo/issues/${EXISTING_CLOSED_ISSUE.number}/comments`) .reply(200); closeIssueScope = nock('https://api.github.com') - .patch(`/repos/owner/repo/issues/${ISSUE.number}`, { state: GitHub.ISSUE_STATE_CLOSED }) + .patch(`/repos/owner/repo/issues/${EXISTING_CLOSED_ISSUE.number}`, { state: GitHub.ISSUE_STATE_CLOSED }) .reply(200); - await github.closeIssueWithCommentIfExists({ title: ISSUE.title, comment: 'Closing comment' }); + await github.closeIssueWithCommentIfExists({ title: EXISTING_CLOSED_ISSUE.title, comment: 'Closing comment' }); }); it('does not add comment', () => { @@ -243,11 +249,6 @@ describe('GitHub', function () { let closeIssueScope; before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, []); - addCommentScope = nock('https://api.github.com') .post(/\/repos\/owner\/repo\/issues\/\d+\/comments/) .reply(200); @@ -270,12 +271,8 @@ describe('GitHub', function () { }); describe('#createOrUpdateIssue', () => { - before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/labels') - .reply(200, MANAGED_LABELS); - - await github.initialize(); + before(() => { + github.MANAGED_LABELS = require('./labels.json'); }); context('when the issue does not exist', () => { @@ -287,11 +284,6 @@ describe('GitHub', function () { }; before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, []); // Simulate that there is no issues on the repository - createIssueScope = nock('https://api.github.com') .post('/repos/owner/repo/issues', { title: ISSUE_TO_CREATE.title, @@ -303,48 +295,38 @@ describe('GitHub', function () { await github.createOrUpdateIssue(ISSUE_TO_CREATE); }); + afterEach(() => { + github.issuesCache.delete(ISSUE_TO_CREATE.title); + }); + it('creates the issue', () => { expect(createIssueScope.isDone()).to.be.true; }); }); context('when the issue already exists', () => { - const ISSUE = { - title: 'Existing Issue', - description: 'New comment', - label: 'location', - }; - context('when issue is closed', () => { let updateIssueScope; let addCommentScope; - const GITHUB_RESPONSE_FOR_EXISTING_ISSUE = { - number: 123, - title: ISSUE.title, - description: ISSUE.description, - labels: [{ name: 'selectors' }], - state: GitHub.ISSUE_STATE_CLOSED, - }; - before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, [GITHUB_RESPONSE_FOR_EXISTING_ISSUE]); - updateIssueScope = nock('https://api.github.com') - .patch(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: ['location'] }) + .patch(`/repos/owner/repo/issues/${EXISTING_CLOSED_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: ['location'] }) .reply(200); addCommentScope = nock('https://api.github.com') - .post(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}/comments`, { body: ISSUE.description }) + .post(`/repos/owner/repo/issues/${EXISTING_CLOSED_ISSUE.number}/comments`, { body: EXISTING_CLOSED_ISSUE.description }) .reply(200); - await github.createOrUpdateIssue(ISSUE); + await github.createOrUpdateIssue({ title: EXISTING_CLOSED_ISSUE.title, description: EXISTING_CLOSED_ISSUE.description, label: 'location' }); + }); + + after(() => { + github.issuesCache.delete(EXISTING_CLOSED_ISSUE.title); + github.issuesCache.set(EXISTING_CLOSED_ISSUE.title, EXISTING_CLOSED_ISSUE); }); - it('reopens the issue and its labels', () => { + it('reopens the issue and updates its labels', () => { expect(updateIssueScope.isDone()).to.be.true; }); @@ -354,40 +336,63 @@ describe('GitHub', function () { }); context('when issue is already opened', () => { - let addCommentScope; - let updateIssueScope; - - const GITHUB_RESPONSE_FOR_EXISTING_ISSUE = { - number: 123, - title: ISSUE.title, - description: ISSUE.description, - labels: [{ name: 'selectors' }], - state: GitHub.ISSUE_STATE_OPEN, - }; - - before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, [GITHUB_RESPONSE_FOR_EXISTING_ISSUE]); - - updateIssueScope = nock('https://api.github.com') - .patch(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: ['location'] }) - .reply(200); - - addCommentScope = nock('https://api.github.com') - .post(`/repos/owner/repo/issues/${GITHUB_RESPONSE_FOR_EXISTING_ISSUE.number}/comments`, { body: ISSUE.description }) - .reply(200); - - await github.createOrUpdateIssue(ISSUE); - }); - - it("updates the issue's labels", () => { - expect(updateIssueScope.isDone()).to.be.true; + context('when the reason is new', () => { + let addCommentScope; + let updateIssueScope; + + before(async () => { + updateIssueScope = nock('https://api.github.com') + .patch(`/repos/owner/repo/issues/${EXISTING_OPEN_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: ['404'] }) + .reply(200); + + addCommentScope = nock('https://api.github.com') + .post(`/repos/owner/repo/issues/${EXISTING_OPEN_ISSUE.number}/comments`, { body: EXISTING_OPEN_ISSUE.description }) + .reply(200); + + await github.createOrUpdateIssue({ title: EXISTING_OPEN_ISSUE.title, description: EXISTING_OPEN_ISSUE.description, label: '404' }); + }); + + after(() => { + github.issuesCache.delete(EXISTING_OPEN_ISSUE.title); + github.issuesCache.set(EXISTING_OPEN_ISSUE.title, EXISTING_OPEN_ISSUE); + }); + + it("updates the issue's labels", () => { + expect(updateIssueScope.isDone()).to.be.true; + }); + + it('adds a comment to the issue', () => { + expect(addCommentScope.isDone()).to.be.true; + }); }); - - it('adds comment to the issue', () => { - expect(addCommentScope.isDone()).to.be.true; + context('when the reason did not change', () => { + let addCommentScope; + let updateIssueScope; + + before(async () => { + updateIssueScope = nock('https://api.github.com') + .patch(`/repos/owner/repo/issues/${EXISTING_OPEN_ISSUE.number}`, { state: GitHub.ISSUE_STATE_OPEN, labels: EXISTING_OPEN_ISSUE.labels }) + .reply(200); + + addCommentScope = nock('https://api.github.com') + .post(`/repos/owner/repo/issues/${EXISTING_OPEN_ISSUE.number}/comments`, { body: EXISTING_OPEN_ISSUE.description }) + .reply(200); + + await github.createOrUpdateIssue({ title: EXISTING_OPEN_ISSUE.title, description: EXISTING_OPEN_ISSUE.description, label: EXISTING_OPEN_ISSUE.labels[0].name }); + }); + + after(() => { + github.issuesCache.delete(EXISTING_OPEN_ISSUE.title); + github.issuesCache.set(EXISTING_OPEN_ISSUE.title, EXISTING_OPEN_ISSUE); + }); + + it('does not attempt to updates the issue’s labels', () => { + expect(updateIssueScope.isDone()).to.be.false; + }); + + it('does not attempt to add any comment to the issue', () => { + expect(addCommentScope.isDone()).to.be.false; + }); }); }); }); diff --git a/src/reporter/index.js b/src/reporter/index.js index d55576e8..23b678de 100644 --- a/src/reporter/index.js +++ b/src/reporter/index.js @@ -49,6 +49,10 @@ export default class Reporter { return this.github.initialize(); } + onTrackingStarted() { + return this.github.clearCache(); + } + async onVersionRecorded(version) { await this.github.closeIssueWithCommentIfExists({ title: Reporter.generateTitleID(version.serviceId, version.termsType),