diff --git a/lib/context/repoContext.js b/lib/context/repoContext.js index 0d419b1fa..fab51d82c 100644 --- a/lib/context/repoContext.js +++ b/lib/context/repoContext.js @@ -2,6 +2,7 @@ 'use strict'; +const Lock = require('lock').Lock; const teamConfigs = require('../teamconfigs'); const { obtainTeamContext } = require('./teamContext'); const initRepoLabels = require('./initRepoLabels'); @@ -106,9 +107,31 @@ const initRepoContext = async (context, config) => { } }; + const lock = Lock(); + return Object.assign(repoContext, { updateStatusCheckFromLabels, + lockPR: (context, callback) => + new Promise((resolve, reject) => { + const pr = context.payload.pull_request; + console.log('try to lock pr', { id: pr.id }); + lock(pr.id, async (release) => { + console.log('pr lock acquired', { id: pr.id }); + try { + await callback(); + } catch (err) { + console.log('release pr (with error)', { id: pr.id }); + release()(); + reject(err); + return; + } + console.log('release pr', { id: pr.id }); + release()(); + resolve(); + }); + }), + updateReviewStatus: async ( context, reviewGroup, @@ -153,7 +176,7 @@ const initRepoContext = async (context, config) => { newLabels: [...newLabels], }); - if (process.env.DRY_RUN) return; + // if (process.env.DRY_RUN) return; if (toAdd.size || toDelete.size) { await context.github.issues.replaceLabels( diff --git a/lib/index.js b/lib/index.js index 81fff9247..f87e8ca9d 100644 --- a/lib/index.js +++ b/lib/index.js @@ -26,126 +26,134 @@ module.exports = (app) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - await Promise.all([ - autoAssignPRToCreator(repoContext, context), - editOpenedPR(repoContext, context), - lintPR(repoContext, context), - repoContext.updateReviewStatus(context, 'dev', { - add: ['needsReview'], - }), - ]); + return repoContext.lockPR(context, async () => { + await Promise.all([ + autoAssignPRToCreator(repoContext, context), + editOpenedPR(repoContext, context), + lintPR(repoContext, context), + repoContext.updateReviewStatus(context, 'dev', { + add: ['needsReview'], + }), + ]); + }); }); app.on('pull_request.review_requested', async (context) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - const sender = context.payload.sender; - // ignore if sender is self (dismissed review rerequest review) - if (sender.type === 'Bot') return; - - const pr = context.payload.pull_request; - const reviewer = context.payload.requested_reviewer; - - const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); - const shouldWait = false; - // repoContext.reviewShouldWait(reviewerGroup, pr.requested_reviewers, { includesWaitForGroups: true }); + return repoContext.lockPR(context, async () => { + const sender = context.payload.sender; - if (repoContext.config.labels.review[reviewerGroup]) { - const { data: reviews } = await context.github.pullRequests.listReviews( - context.issue({ per_page: 50 }) - ); - const hasChangesRequestedInReviews = reviews.some( - (review) => - repoContext.getReviewerGroup(review.user.login) === reviewerGroup && - review.state === 'REQUEST_CHANGES' && - // In case this is a rerequest for review - review.user.login !== reviewer.login - ); + // ignore if sender is self (dismissed review rerequest review) + if (sender.type === 'Bot') return; - if (!hasChangesRequestedInReviews) { - repoContext.updateReviewStatus(context, reviewerGroup, { - add: ['needsReview', !shouldWait && 'requested'], - remove: ['approved', 'changesRequested'], - }); + const pr = context.payload.pull_request; + const reviewer = context.payload.requested_reviewer; + + const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); + const shouldWait = false; + // repoContext.reviewShouldWait(reviewerGroup, pr.requested_reviewers, { includesWaitForGroups: true }); + + if (repoContext.config.labels.review[reviewerGroup]) { + const { data: reviews } = await context.github.pullRequests.listReviews( + context.issue({ per_page: 50 }) + ); + const hasChangesRequestedInReviews = reviews.some( + (review) => + repoContext.getReviewerGroup(review.user.login) === reviewerGroup && + review.state === 'REQUEST_CHANGES' && + // In case this is a rerequest for review + review.user.login !== reviewer.login + ); + + if (!hasChangesRequestedInReviews) { + repoContext.updateReviewStatus(context, reviewerGroup, { + add: ['needsReview', !shouldWait && 'requested'], + remove: ['approved', 'changesRequested'], + }); + } } - } - if (sender.login === reviewer.login) return; + if (sender.login === reviewer.login) return; - if (!shouldWait) { - repoContext.slack.postMessage( - reviewer.login, - `:eyes: ${repoContext.slack.mention( - sender.login - )} requests your review on ${pr.html_url} !\n> ${pr.title}` - ); - } + if (!shouldWait) { + repoContext.slack.postMessage( + reviewer.login, + `:eyes: ${repoContext.slack.mention( + sender.login + )} requests your review on ${pr.html_url} !\n> ${pr.title}` + ); + } + }); }); app.on('pull_request.review_request_removed', async (context) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - const sender = context.payload.sender; - const pr = context.payload.pull_request; - const reviewer = context.payload.requested_reviewer; - const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); + return repoContext.lockPR(context, async () => { + const sender = context.payload.sender; + const pr = context.payload.pull_request; + const reviewer = context.payload.requested_reviewer; - const hasRequestedReviewsForGroup = repoContext.reviewShouldWait( - reviewerGroup, - pr.requested_reviewers, - { - includesReviewerGroup: true, - } - ); + const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); - if (repoContext.config.labels.review[reviewerGroup]) { - const { data: reviews } = await context.github.pullRequests.listReviews( - context.issue({ per_page: 50 }) + const hasRequestedReviewsForGroup = repoContext.reviewShouldWait( + reviewerGroup, + pr.requested_reviewers, + { + includesReviewerGroup: true, + } ); - const hasChangesRequestedInReviews = reviews.some( - (review) => - repoContext.getReviewerGroup(review.user.login) === reviewerGroup && - review.state === 'REQUEST_CHANGES' - ); + if (repoContext.config.labels.review[reviewerGroup]) { + const { data: reviews } = await context.github.pullRequests.listReviews( + context.issue({ per_page: 50 }) + ); - const hasApprovedInReviews = reviews.some( - (review) => - repoContext.getReviewerGroup(review.user.login) === reviewerGroup && - review.state === 'APPROVED' - ); + const hasChangesRequestedInReviews = reviews.some( + (review) => + repoContext.getReviewerGroup(review.user.login) === reviewerGroup && + review.state === 'REQUEST_CHANGES' + ); + + const hasApprovedInReviews = reviews.some( + (review) => + repoContext.getReviewerGroup(review.user.login) === reviewerGroup && + review.state === 'APPROVED' + ); - const approved = - !hasRequestedReviewsForGroup && - !hasChangesRequestedInReviews && - hasApprovedInReviews; - repoContext.updateReviewStatus(context, reviewerGroup, { - add: [ - // if changes requested by the one which requests was removed - hasChangesRequestedInReviews && 'changesRequested', - // if was already approved by another member in the group and has no other requests waiting - approved && 'approved', - ], - // remove labels if has no other requests waiting - remove: [ - approved && 'needsReview', + const approved = !hasRequestedReviewsForGroup && - !hasChangesRequestedInReviews && - 'requested', - ], - }); - } + !hasChangesRequestedInReviews && + hasApprovedInReviews; + repoContext.updateReviewStatus(context, reviewerGroup, { + add: [ + // if changes requested by the one which requests was removed + hasChangesRequestedInReviews && 'changesRequested', + // if was already approved by another member in the group and has no other requests waiting + approved && 'approved', + ], + // remove labels if has no other requests waiting + remove: [ + approved && 'needsReview', + !hasRequestedReviewsForGroup && + !hasChangesRequestedInReviews && + 'requested', + ], + }); + } - if (sender.login === reviewer.login) return; + if (sender.login === reviewer.login) return; - repoContext.slack.postMessage( - reviewer.login, - `:skull_and_crossbones: ${repoContext.slack.mention( - sender.login - )} removed the request for your review on ${pr.html_url}` - ); + repoContext.slack.postMessage( + reviewer.login, + `:skull_and_crossbones: ${repoContext.slack.mention( + sender.login + )} removed the request for your review on ${pr.html_url}` + ); + }); }); // app.on('pull_request.closed', async context => { @@ -159,111 +167,117 @@ module.exports = (app) => { app.on('pull_request_review.submitted', async (context) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - const pr = context.payload.pull_request; - const { user: reviewer, state } = context.payload.review; - if (pr.user.login === reviewer.login) return; - const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); - - if (reviewerGroup && repoContext.config.labels.review[reviewerGroup]) { - const hasRequestedReviewsForGroup = repoContext.reviewShouldWait( - reviewerGroup, - pr.requested_reviewers, - { - includesReviewerGroup: true, - // TODO reenable this when accepted can notify request review to slack (dev accepted => design requested) and flag to disable for label (approved design ; still waiting for dev ?) - // includesWaitForGroups: true, - } - ); - const { data: reviews } = await context.github.pullRequests.listReviews( - context.issue({ per_page: 50 }) - ); - const hasChangesRequestedInReviews = reviews.some( - (review) => - repoContext.getReviewerGroup(review.user.login) === reviewerGroup && - review.state === 'REQUEST_CHANGES' - ); - - const approved = - !hasRequestedReviewsForGroup && - !hasChangesRequestedInReviews && - state === 'approved'; - repoContext.updateReviewStatus(context, reviewerGroup, { - add: [ - approved && 'approved', - state === 'changes_requested' && 'changesRequested', - ], - remove: [ - approved && 'needsReview', - !(hasRequestedReviewsForGroup || state === 'changes_requested') && - 'requested', - state === 'approved' && - !hasChangesRequestedInReviews && - 'changesRequested', - state === 'changes_requested' && 'approved', - ], - }); - } + return repoContext.lockPR(context, async () => { + const pr = context.payload.pull_request; + const { user: reviewer, state } = context.payload.review; + if (pr.user.login === reviewer.login) return; + + const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); + + if (reviewerGroup && repoContext.config.labels.review[reviewerGroup]) { + const hasRequestedReviewsForGroup = repoContext.reviewShouldWait( + reviewerGroup, + pr.requested_reviewers, + { + includesReviewerGroup: true, + // TODO reenable this when accepted can notify request review to slack (dev accepted => design requested) and flag to disable for label (approved design ; still waiting for dev ?) + // includesWaitForGroups: true, + } + ); + const { data: reviews } = await context.github.pullRequests.listReviews( + context.issue({ per_page: 50 }) + ); + const hasChangesRequestedInReviews = reviews.some( + (review) => + repoContext.getReviewerGroup(review.user.login) === reviewerGroup && + review.state === 'REQUEST_CHANGES' + ); + + const approved = + !hasRequestedReviewsForGroup && + !hasChangesRequestedInReviews && + state === 'approved'; + repoContext.updateReviewStatus(context, reviewerGroup, { + add: [ + approved && 'approved', + state === 'changes_requested' && 'changesRequested', + ], + remove: [ + approved && 'needsReview', + !(hasRequestedReviewsForGroup || state === 'changes_requested') && + 'requested', + state === 'approved' && + !hasChangesRequestedInReviews && + 'changesRequested', + state === 'changes_requested' && 'approved', + ], + }); + } - const mention = repoContext.slack.mention(reviewer.login); - const prUrl = pr.html_url; + const mention = repoContext.slack.mention(reviewer.login); + const prUrl = pr.html_url; - const message = (() => { - if (state === 'changes_requested') { - return `:x: ${mention} requests changes on ${prUrl}`; - } - if (state === 'approved') { - return `:clap: :white_check_mark: ${mention} approves ${prUrl}`; - } - return `:speech_balloon: ${mention} commented on ${prUrl}`; - })(); + const message = (() => { + if (state === 'changes_requested') { + return `:x: ${mention} requests changes on ${prUrl}`; + } + if (state === 'approved') { + return `:clap: :white_check_mark: ${mention} approves ${prUrl}`; + } + return `:speech_balloon: ${mention} commented on ${prUrl}`; + })(); - repoContext.slack.postMessage(pr.user.login, message); + repoContext.slack.postMessage(pr.user.login, message); + }); }); app.on('pull_request_review.dismissed', async (context) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - const sender = context.payload.sender; - const pr = context.payload.pull_request; - const reviewer = context.payload.review.user; - const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); + return repoContext.lockPR(context, async () => { + const sender = context.payload.sender; + const pr = context.payload.pull_request; + const reviewer = context.payload.review.user; - if (reviewerGroup && repoContext.config.labels.review[reviewerGroup]) { - const { data: reviews } = await context.github.pullRequests.listReviews( - context.issue({ per_page: 50 }) - ); - const hasChangesRequestedInReviews = reviews.some( - (review) => - repoContext.getReviewerGroup(review.user.login) === reviewerGroup && - review.state === 'REQUEST_CHANGES' - ); + const reviewerGroup = repoContext.getReviewerGroup(reviewer.login); - repoContext.updateReviewStatus(context, reviewerGroup, { - add: ['needsReview', 'requested'], - remove: [ - !hasChangesRequestedInReviews && 'changesRequested', - 'approved', - ], - }); - } + if (reviewerGroup && repoContext.config.labels.review[reviewerGroup]) { + const { data: reviews } = await context.github.pullRequests.listReviews( + context.issue({ per_page: 50 }) + ); + const hasChangesRequestedInReviews = reviews.some( + (review) => + repoContext.getReviewerGroup(review.user.login) === reviewerGroup && + review.state === 'REQUEST_CHANGES' + ); - if (sender.login === reviewer.login) { - repoContext.slack.postMessage( - pr.user.login, - `:skull: ${repoContext.slack.mention( - reviewer.login - )} dismissed his review on ${pr.html_url}` - ); - } else { - repoContext.slack.postMessage( - reviewer.login, - `:skull: ${repoContext.slack.mention( - sender.login - )} dismissed your review on ${pr.html_url}` - ); - } + repoContext.updateReviewStatus(context, reviewerGroup, { + add: ['needsReview', 'requested'], + remove: [ + !hasChangesRequestedInReviews && 'changesRequested', + 'approved', + ], + }); + } + + if (sender.login === reviewer.login) { + repoContext.slack.postMessage( + pr.user.login, + `:skull: ${repoContext.slack.mention( + reviewer.login + )} dismissed his review on ${pr.html_url}` + ); + } else { + repoContext.slack.postMessage( + reviewer.login, + `:skull: ${repoContext.slack.mention( + sender.login + )} dismissed your review on ${pr.html_url}` + ); + } + }); }); app.on( @@ -275,7 +289,9 @@ module.exports = (app) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - await repoContext.updateStatusCheckFromLabels(context); + return repoContext.lockPR(context, async () => { + await repoContext.updateStatusCheckFromLabels(context); + }); } ); @@ -283,19 +299,23 @@ module.exports = (app) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - await Promise.all([ - editOpenedPR(repoContext, context), - repoContext.addStatusCheckToLatestCommit(context), - ]); + return repoContext.lockPR(context, async () => { + await Promise.all([ + editOpenedPR(repoContext, context), + repoContext.addStatusCheckToLatestCommit(context), + ]); + }); }); app.on('pull_request.edited', async (context) => { const repoContext = await obtainRepoContext(context); if (!repoContext) return; - await Promise.all([ - editOpenedPR(repoContext, context), - lintPR(repoContext, context), - ]); + return repoContext.lockPR(context, async () => { + await Promise.all([ + editOpenedPR(repoContext, context), + lintPR(repoContext, context), + ]); + }); }); }; diff --git a/lib/teamconfigs/christophehurpeau.js b/lib/teamconfigs/christophehurpeau.js index 91c3daf2c..20c69e40e 100644 --- a/lib/teamconfigs/christophehurpeau.js +++ b/lib/teamconfigs/christophehurpeau.js @@ -20,6 +20,7 @@ module.exports = { }, dev: { christophehurpeau: 'christophe@hurpeau.com', + 'chris-reviewflow': 'christophe.hurpeau+reviewflow@gmail.com', }, waitForGroups: { dev: [], diff --git a/package.json b/package.json index 16572ba14..d149de227 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,7 @@ }, "dependencies": { "@slack/client": "4.8.0", + "lock": "^1.1.0", "probot": "7.4.0", "probot-config": "probot/probot-config#def82e6b324439f08c1c630c4f3cb57bab689979" }, diff --git a/yarn.lock b/yarn.lock index 44458ba65..a22a55cfe 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4187,6 +4187,11 @@ locate-path@^3.0.0: p-locate "^3.0.0" path-exists "^3.0.0" +lock@^1.1.0: + version "1.1.0" + resolved "https://registry.yarnpkg.com/lock/-/lock-1.1.0.tgz#53157499d1653b136ca66451071fca615703fa55" + integrity sha1-UxV0mdFlOxNspmRRBx/KYVcD+lU= + lodash._basecopy@^3.0.0: version "3.0.1" resolved "https://registry.yarnpkg.com/lodash._basecopy/-/lodash._basecopy-3.0.1.tgz#8da0e6a876cf344c0ad8a54882111dd3c5c7ca36"