From b63d6139029b08c1ac6dfce6f775ac273fa1eb87 Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 15:48:57 +0200 Subject: [PATCH 1/7] Cache issues to reduce API calls --- src/reporter/github.js | 102 +++++++++++---- src/reporter/github.test.js | 254 ++++++++++++++++++------------------ src/reporter/index.js | 4 + 3 files changed, 207 insertions(+), 153 deletions(-) diff --git a/src/reporter/github.js b/src/reporter/github.js index 3da9b0400..510e6f2d3 100644 --- a/src/reporter/github.js +++ b/src/reporter/github.js @@ -28,6 +28,9 @@ export default class GitHub { const [ owner, repo ] = repository.split('/'); this.commonParams = { owner, repo }; + + this.issuesCache = new Map(); + this.cacheLoadedPromise = null; } async initialize() { @@ -53,6 +56,49 @@ 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 cache the oldest issue if there are duplicates + this.issuesCache.set(issue.title, issue); + } + }); + + logger.info(`Cached ${onlyIssues.length} issues from the repository`); + } catch (error) { + logger.error(`Failed to load issues: ${error.message}`); + } + } + + async refreshIssuesCache() { + try { + logger.info('Refreshing issues cache from GitHub…'); + this.issuesCache.clear(); + this.cacheLoadedPromise = this.loadAllIssues(); + await this.cacheLoadedPromise; + logger.info('Issues cache refreshed successfully'); + } catch (error) { + logger.error(`Failed to refresh issues cache: ${error.message}`); + } + } + + async ensureCacheLoaded() { + if (!this.cacheLoadedPromise) { + this.cacheLoadedPromise = this.loadAllIssues(); + } + await this.cacheLoadedPromise; + } + async getRepositoryLabels() { const { data: labels } = await this.octokit.request('GET /repos/{owner}/{repo}/labels', { ...this.commonParams }); @@ -69,6 +115,8 @@ export default class GitHub { } async createIssue({ title, description: body, labels }) { + await this.ensureCacheLoaded(); + const { data: issue } = await this.octokit.request('POST /repos/{owner}/{repo}/issues', { ...this.commonParams, title, @@ -76,10 +124,14 @@ export default class GitHub { labels, }); + this.issuesCache.set(issue.title, issue); + return issue; } async updateIssue(issue, { state, labels }) { + await this.ensureCacheLoaded(); + const { data: updatedIssue } = await this.octokit.request('PATCH /repos/{owner}/{repo}/issues/{issue_number}', { ...this.commonParams, issue_number: issue.number, @@ -87,22 +139,18 @@ export default class GitHub { labels, }); + this.issuesCache.set(updatedIssue.title, updatedIssue); + 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 - - return issue; + getIssue(title) { + return this.issuesCache.get(title) || null; } async addCommentToIssue({ issue, comment: body }) { + await this.ensureCacheLoaded(); + const { data: comment } = await this.octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', { ...this.commonParams, issue_number: issue.number, @@ -114,25 +162,30 @@ export default class GitHub { async closeIssueWithCommentIfExists({ title, comment }) { try { - const openedIssue = await this.getIssue({ title, state: GitHub.ISSUE_STATE_OPEN }); + await this.ensureCacheLoaded(); - if (!openedIssue) { + const issue = this.getIssue(title); + + 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}`); + this.issuesCache.set(updatedIssue.title, updatedIssue); + 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 }); + await this.ensureCacheLoaded(); + + const issue = this.getIssue(title); if (!issue) { const createdIssue = await this.createIssue({ title, description, labels: [label] }); @@ -141,6 +194,7 @@ export default class GitHub { } const managedLabelsNames = this.MANAGED_LABELS.map(label => label.name); + const labelsNotManagedToKeep = issue.labels.map(label => label.name).filter(label => !managedLabelsNames.includes(label)); const [managedLabel] = issue.labels.filter(label => managedLabelsNames.includes(label.name)); // It is assumed that only one specific reason for failure is possible at a time, making managed labels mutually exclusive @@ -148,16 +202,14 @@ 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 ].filter(label => label) }); + await this.addCommentToIssue({ issue, comment: description }); - logger.info(`Added comment to issue #${issue.number}: ${issue.html_url}`); + this.issuesCache.set(updatedIssue.title, updatedIssue); + 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 6ec78519a..728938da1 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.refreshIssuesCache(); }); describe('#initialize', () => { @@ -71,77 +78,81 @@ 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 error when creating 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; - - const ISSUE = { number: 123, title: 'Test Issue' }; - const ANOTHER_ISSUE = { number: 124, title: 'Test Issue 2' }; - before(async () => { - scope = nock('https://api.github.com') - .get('/repos/owner/repo/issues') - .query(true) - .reply(200, [ ISSUE, ANOTHER_ISSUE ]); + it('throws error when creating issue fails', async () => { + nock('https://api.github.com') + .post('/repos/owner/repo/issues') + .reply(400, { message: 'Bad Request' }); - result = await github.getIssue({ title: ISSUE.title }); + await expect(github.createIssue(ISSUE)).to.be.rejected; }); + }); - after(nock.cleanAll); - - it('searches for the issue', () => { - expect(scope.isDone()).to.be.true; + describe('#getIssue', () => { + context('when the issue exists in the cache', () => { + it('returns the cached issue', () => { + expect(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 null', () => { + expect(github.getIssue('Non-existent Issue')).to.be.null; + }); }); }); @@ -163,35 +174,33 @@ describe('GitHub', function () { it('adds the comment to the issue', () => { expect(scope.isDone()).to.be.true; }); + + it('throws error when adding 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 +213,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 +242,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); @@ -271,11 +265,7 @@ describe('GitHub', function () { describe('#createOrUpdateIssue', () => { before(async () => { - nock('https://api.github.com') - .get('/repos/owner/repo/labels') - .reply(200, MANAGED_LABELS); - - await github.initialize(); + github.MANAGED_LABELS = require('./labels.json'); }); context('when the issue does not exist', () => { @@ -287,11 +277,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 +288,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' }); }); - it('reopens the issue and its labels', () => { + after(() => { + github.issuesCache.delete(EXISTING_CLOSED_ISSUE); + github.issuesCache.set(EXISTING_CLOSED_ISSUE.title, EXISTING_CLOSED_ISSUE); + }); + + it('reopens the issue and updates its labels', () => { expect(updateIssueScope.isDone()).to.be.true; }); @@ -354,40 +329,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 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 unchanged', () => { + 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 comment to the issue', () => { + expect(addCommentScope.isDone()).to.be.false; + }); }); }); }); diff --git a/src/reporter/index.js b/src/reporter/index.js index d55576e8a..9171894a1 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.refreshIssuesCache(); + } + async onVersionRecorded(version) { await this.github.closeIssueWithCommentIfExists({ title: Reporter.generateTitleID(version.serviceId, version.termsType), From bddce587c7514140e89a71702a08c213d7838edd Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 16:48:07 +0200 Subject: [PATCH 2/7] Improve phrasing Co-authored-by: Matti Schneider --- src/reporter/github.js | 2 +- src/reporter/github.test.js | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/reporter/github.js b/src/reporter/github.js index 510e6f2d3..ed2a5f3ef 100644 --- a/src/reporter/github.js +++ b/src/reporter/github.js @@ -69,7 +69,7 @@ export default class GitHub { onlyIssues.forEach(issue => { const cachedIssue = this.issuesCache.get(issue.title); - if (!cachedIssue || new Date(issue.created_at) < new Date(cachedIssue.created_at)) { // Only cache the oldest issue if there are duplicates + 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); } }); diff --git a/src/reporter/github.test.js b/src/reporter/github.test.js index 728938da1..f384fd5a5 100644 --- a/src/reporter/github.test.js +++ b/src/reporter/github.test.js @@ -91,7 +91,7 @@ describe('GitHub', function () { expect(scope.isDone()).to.be.true; }); - it('throws error when creating label fails', async () => { + 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' }); @@ -175,7 +175,7 @@ describe('GitHub', function () { expect(scope.isDone()).to.be.true; }); - it('throws error when adding comment fails', async () => { + 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' }); @@ -354,11 +354,11 @@ describe('GitHub', function () { expect(updateIssueScope.isDone()).to.be.true; }); - it('adds comment to the issue', () => { + it('adds a comment to the issue', () => { expect(addCommentScope.isDone()).to.be.true; }); }); - context('when the reason unchanged', () => { + context('when the reason did not change', () => { let addCommentScope; let updateIssueScope; @@ -379,11 +379,11 @@ describe('GitHub', function () { github.issuesCache.set(EXISTING_OPEN_ISSUE.title, EXISTING_OPEN_ISSUE); }); - it("does not attempt to updates the issue's labels", () => { + it('does not attempt to updates the issue’s labels', () => { expect(updateIssueScope.isDone()).to.be.false; }); - it('does not attempt to add comment to the issue', () => { + it('does not attempt to add any comment to the issue', () => { expect(addCommentScope.isDone()).to.be.false; }); }); From c2283d6da3cf41a347602a6130a115ccef5e99c7 Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 17:29:00 +0200 Subject: [PATCH 3/7] Refactor caching system --- src/reporter/github.js | 64 +++++++++++++++---------------------- src/reporter/github.test.js | 21 ++++++++---- src/reporter/index.js | 2 +- 3 files changed, 41 insertions(+), 46 deletions(-) diff --git a/src/reporter/github.js b/src/reporter/github.js index ed2a5f3ef..49393153f 100644 --- a/src/reporter/github.js +++ b/src/reporter/github.js @@ -30,7 +30,22 @@ export default class GitHub { this.commonParams = { owner, repo }; this.issuesCache = new Map(); - this.cacheLoadedPromise = null; + 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() { @@ -74,29 +89,13 @@ export default class GitHub { } }); - logger.info(`Cached ${onlyIssues.length} issues from the repository`); - } catch (error) { - logger.error(`Failed to load issues: ${error.message}`); - } - } + logger.info(`Cached ${onlyIssues.length} issues from the GitHub repository`); - async refreshIssuesCache() { - try { - logger.info('Refreshing issues cache from GitHub…'); - this.issuesCache.clear(); - this.cacheLoadedPromise = this.loadAllIssues(); - await this.cacheLoadedPromise; - logger.info('Issues cache refreshed successfully'); + return this.issuesCache; } catch (error) { - logger.error(`Failed to refresh issues cache: ${error.message}`); - } - } - - async ensureCacheLoaded() { - if (!this.cacheLoadedPromise) { - this.cacheLoadedPromise = this.loadAllIssues(); + logger.error(`Failed to load issues: ${error.message}`); + throw error; } - await this.cacheLoadedPromise; } async getRepositoryLabels() { @@ -114,9 +113,11 @@ export default class GitHub { }); } - async createIssue({ title, description: body, labels }) { - await this.ensureCacheLoaded(); + 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, title, @@ -130,8 +131,6 @@ export default class GitHub { } async updateIssue(issue, { state, labels }) { - await this.ensureCacheLoaded(); - const { data: updatedIssue } = await this.octokit.request('PATCH /repos/{owner}/{repo}/issues/{issue_number}', { ...this.commonParams, issue_number: issue.number, @@ -144,13 +143,7 @@ export default class GitHub { return updatedIssue; } - getIssue(title) { - return this.issuesCache.get(title) || null; - } - async addCommentToIssue({ issue, comment: body }) { - await this.ensureCacheLoaded(); - const { data: comment } = await this.octokit.request('POST /repos/{owner}/{repo}/issues/{issue_number}/comments', { ...this.commonParams, issue_number: issue.number, @@ -162,9 +155,7 @@ export default class GitHub { async closeIssueWithCommentIfExists({ title, comment }) { try { - await this.ensureCacheLoaded(); - - const issue = this.getIssue(title); + const issue = await this.getIssue(title); if (!issue || issue.state == GitHub.ISSUE_STATE_CLOSED) { return; @@ -183,9 +174,7 @@ export default class GitHub { async createOrUpdateIssue({ title, description, label }) { try { - await this.ensureCacheLoaded(); - - const issue = this.getIssue(title); + const issue = await this.getIssue(title); if (!issue) { const createdIssue = await this.createIssue({ title, description, labels: [label] }); @@ -194,7 +183,6 @@ export default class GitHub { } const managedLabelsNames = this.MANAGED_LABELS.map(label => label.name); - const labelsNotManagedToKeep = issue.labels.map(label => label.name).filter(label => !managedLabelsNames.includes(label)); const [managedLabel] = issue.labels.filter(label => managedLabelsNames.includes(label.name)); // It is assumed that only one specific reason for failure is possible at a time, making managed labels mutually exclusive diff --git a/src/reporter/github.test.js b/src/reporter/github.test.js index f384fd5a5..5aa21f18e 100644 --- a/src/reporter/github.test.js +++ b/src/reporter/github.test.js @@ -22,7 +22,7 @@ describe('GitHub', function () { .get('/repos/owner/repo/issues') .query(true) .reply(200, [ EXISTING_OPEN_ISSUE, EXISTING_CLOSED_ISSUE ]); - await github.refreshIssuesCache(); + await github.clearCache(); }); describe('#initialize', () => { @@ -143,15 +143,22 @@ describe('GitHub', function () { }); describe('#getIssue', () => { + before(() => { + nock('https://api.github.com') + .get('/repos/owner/repo/issues') + .query(true) + .reply(200, [ EXISTING_OPEN_ISSUE, EXISTING_CLOSED_ISSUE ]); + }); + context('when the issue exists in the cache', () => { - it('returns the cached issue', () => { - expect(github.getIssue(EXISTING_OPEN_ISSUE.title)).to.deep.equal(EXISTING_OPEN_ISSUE); + it('returns the cached issue', async () => { + expect(await github.getIssue(EXISTING_OPEN_ISSUE.title)).to.deep.equal(EXISTING_OPEN_ISSUE); }); }); context('when the issue does not exist in the cache', () => { - it('returns null', () => { - expect(github.getIssue('Non-existent Issue')).to.be.null; + it('returns undefined', async () => { + expect(await github.getIssue('Non-existent Issue')).to.be.undefined; }); }); }); @@ -264,7 +271,7 @@ describe('GitHub', function () { }); describe('#createOrUpdateIssue', () => { - before(async () => { + before(() => { github.MANAGED_LABELS = require('./labels.json'); }); @@ -315,7 +322,7 @@ describe('GitHub', function () { }); after(() => { - github.issuesCache.delete(EXISTING_CLOSED_ISSUE); + github.issuesCache.delete(EXISTING_CLOSED_ISSUE.title); github.issuesCache.set(EXISTING_CLOSED_ISSUE.title, EXISTING_CLOSED_ISSUE); }); diff --git a/src/reporter/index.js b/src/reporter/index.js index 9171894a1..23b678de8 100644 --- a/src/reporter/index.js +++ b/src/reporter/index.js @@ -50,7 +50,7 @@ export default class Reporter { } onTrackingStarted() { - return this.github.refreshIssuesCache(); + return this.github.clearCache(); } async onVersionRecorded(version) { From fd4fff0998bb457437343cc8570dfe7f4b545ec3 Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 17:29:23 +0200 Subject: [PATCH 4/7] Remove obsolete code --- src/reporter/github.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/reporter/github.js b/src/reporter/github.js index 49393153f..be57e51c8 100644 --- a/src/reporter/github.js +++ b/src/reporter/github.js @@ -165,7 +165,6 @@ export default class GitHub { const updatedIssue = await this.updateIssue(issue, { state: GitHub.ISSUE_STATE_CLOSED }); - this.issuesCache.set(updatedIssue.title, updatedIssue); logger.info(`Closed issue with comment #${updatedIssue.number}: ${updatedIssue.html_url}`); } catch (error) { logger.error(`Failed to close issue with comment "${title}": ${error.stack}`); @@ -194,7 +193,6 @@ export default class GitHub { await this.addCommentToIssue({ issue, comment: description }); - this.issuesCache.set(updatedIssue.title, updatedIssue); logger.info(`Updated issue with comment #${updatedIssue.number}: ${updatedIssue.html_url}`); } catch (error) { logger.error(`Failed to update issue "${title}": ${error.stack}`); From 5edd63b22e9296589707fe0c321e368ab31c72a2 Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 17:33:53 +0200 Subject: [PATCH 5/7] Remove obsolete code --- src/reporter/github.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/reporter/github.js b/src/reporter/github.js index be57e51c8..535d008a3 100644 --- a/src/reporter/github.js +++ b/src/reporter/github.js @@ -189,7 +189,7 @@ export default class GitHub { return; } - const updatedIssue = await this.updateIssue(issue, { state: GitHub.ISSUE_STATE_OPEN, labels: [ label, ...labelsNotManagedToKeep ].filter(label => label) }); + const updatedIssue = await this.updateIssue(issue, { state: GitHub.ISSUE_STATE_OPEN, labels: [ label, ...labelsNotManagedToKeep ] }); await this.addCommentToIssue({ issue, comment: description }); From 4c6fd8443460b3af2ed5aaa42ad87d579da95c46 Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 17:38:13 +0200 Subject: [PATCH 6/7] Add changelog entry --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2397caf6..2a0561de5 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)._ From 06e8664b1b015cf4627b56814972fa0d4968352b Mon Sep 17 00:00:00 2001 From: Nicolas Dupont Date: Wed, 23 Oct 2024 17:49:22 +0200 Subject: [PATCH 7/7] Fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a0561de5..a0f4ae5a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All changes that impact users of this module are documented in this file, in the > 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 +### Fixed - Reduce GitHub API calls in issues reporter