Skip to content
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

Reduce GitHub API calls in issues reporter #1113

Merged
merged 7 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
102 changes: 77 additions & 25 deletions src/reporter/github.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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
Ndpnt marked this conversation as resolved.
Show resolved Hide resolved
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() {
Copy link
Member

Choose a reason for hiding this comment

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

I would find it more straightforward if there was some issues getter that memoizes the promise and populates the cache if needed, and a clearCache method that does only that.

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() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply await this.cacheLoadedPromise when we call ensureCacheLoaded()? 🙂 Then we don't need to await it in refreshIssuesCache() and we don't need this method.

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 });

Expand All @@ -69,40 +115,42 @@ 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,
body,
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,
state,
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) {
Copy link
Member

Choose a reason for hiding this comment

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

What are the benefits of wrapping this.issues.get(title) into this.getIssue(title)? 🙂

return this.issuesCache.get(title) || null;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't undefined acceptable as a return value if the issue is not found in the cache? 🙂

Suggested change
return this.issuesCache.get(title) || null;
return this.issuesCache.get(title);

}

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,
Expand All @@ -114,25 +162,30 @@ export default class GitHub {

async closeIssueWithCommentIfExists({ title, comment }) {
try {
MattiSG marked this conversation as resolved.
Show resolved Hide resolved
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);
MattiSG marked this conversation as resolved.
Show resolved Hide resolved
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] });
Expand All @@ -141,23 +194,22 @@ 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

if (issue.state !== GitHub.ISSUE_STATE_CLOSED && managedLabel?.name === label) {
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) });
Copy link
Member

Choose a reason for hiding this comment

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

Why do we add the filter here? Do we have cases where some labels are empty strings?

I found it more readable over multiple lines.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had it in tests but not sure if it can happen in real life. I will double check.


await this.addCommentToIssue({ issue, comment: description });

logger.info(`Added comment to issue #${issue.number}: ${issue.html_url}`);
this.issuesCache.set(updatedIssue.title, updatedIssue);
Copy link
Member

Choose a reason for hiding this comment

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

I feel that the cache update should take place in updateIssue, not in the caller.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, of course. Fatigue...

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}`);
}
}
}
Loading
Loading