From 648700a0cce45f3aca99ded19f1fe6db600d070a Mon Sep 17 00:00:00 2001 From: Simon Imbrogno Date: Fri, 7 Feb 2020 12:19:14 -0600 Subject: [PATCH 1/5] Collapse awaits in webhookHandler --- src/lib/github/webhookHandlers.js | 68 +++++++++++++++++-------------- src/lib/github/webhookRouter.js | 1 + 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/src/lib/github/webhookHandlers.js b/src/lib/github/webhookHandlers.js index 44ad8f4..9febf86 100644 --- a/src/lib/github/webhookHandlers.js +++ b/src/lib/github/webhookHandlers.js @@ -78,21 +78,15 @@ async function sendOpenerInitialStateMessage(opener, reviewers, prObj) { async function requestReviewersAndAssignees(users, body) { try { const githubUsers = users.map(user => user.github); + const owner = body.pull_request.base.repo.owner.login; + const repo = body.pull_request.base.repo.name; + const number = body.pull_request.number; // Should probably look at the results to check if reviewres are there. - const reviewRequests = await octokit.pullRequests.createReviewRequest({ - owner: body.pull_request.base.repo.owner.login, - repo: body.pull_request.base.repo.name, - number: body.pull_request.number, - reviewers: githubUsers, - }); - - const assignees = await octokit.issues.addAssignees({ - owner: body.pull_request.base.repo.owner.login, - repo: body.pull_request.base.repo.name, - number: body.pull_request.number, - assignees: githubUsers, - }); + const [reviewRequests, assignees] = await Promise.all([ + octokit.pullRequests.createReviewRequest({ owner, repo, number, reviewers: githubUsers }), + octokit.issues.addAssignees({ owner, repo, number, assignees: githubUsers }), + ]); logger.info(`[Add Users to PR] Repo: ${body.pull_request.base.repo.name}. ` + `Assigned and Request reviews from: ${githubUsers}`); @@ -105,9 +99,13 @@ async function requestReviewersAndAssignees(users, body) { async function requestReviewByGithubName(body) { const logId = shortid.generate(); - const opener = await findByGithubName(body.pull_request.user.login, logId); + + const [opener, requestedReviewer] = await Promise.all([ + findByGithubName(body.pull_request.user.login, logId), + findByGithubName(body.requested_reviewer.login, logId), + ]); + const openerName = opener ? opener.name : body.pull_request.user.login; - const requestedReviewer = await findByGithubName(body.requested_reviewer.login, logId); if (requestedReviewer && requestedReviewer.slack) { return await sendReviewRequestMessage(openerName, requestedReviewer, body); } @@ -132,9 +130,10 @@ async function prOpened(body) { const numReviewersAlready = body.pull_request.assignees.length; const numReviewersToRandomlySelect = NUM_REVIEWERS - numReviewersAlready; - const preselectedUsers = await Promise.all(body.pull_request.assignees.map(user => { - return findByGithubName(user.login, logId); - })); + const preselectedUsers = await Promise.all( + body.pull_request.assignees.map(user => findByGithubName(user.login, logId)) + ); + const notTheseUsers = opener ? preselectedUsers.concat(opener.github) : preselectedUsers; const randomUsers = await selectRandomGithubUsersNot(notTheseUsers, numReviewersToRandomlySelect); const users = preselectedUsers.concat(randomUsers); @@ -195,8 +194,11 @@ async function prReviewed(body) { let reviewer, coder; const logId = shortid.generate(); try { - reviewer = await findByGithubName(body.review.user.login, logId); - coder = await findByGithubName(body.pull_request.user.login, logId); + const [reviewer, coder] = await Promise.all( + findByGithubName(body.review.user.login, logId), + findByGithubName(body.pull_request.user.login, logId), + ); + if (!reviewer) throw new Error(`[github.prReviewed:${logId}] Reviewer not registered with git slackin`); if (!coder) throw new Error(`[github.prReviewed:${logId}] Coder not registered with git slackin`); } catch (e) { @@ -237,13 +239,16 @@ async function prReviewed(body) { try { // let coder know its been done - await send(coder.slack.id, message); - let shouldNotify = await checkForReviews({ - owner: body.repository.owner.login, - repo: body.repository.name, - number: body.pull_request.number }); + const [, passedReviewThreshold] = await Promise.all([ + send(coder.slack.id, message), + checkForReviews({ + owner: body.repository.owner.login, + repo: body.repository.name, + number: body.pull_request.number, + }), + ]); - shouldNotify = shouldNotify && body.review.state.toUpperCase() === 'APPROVED'; + const shouldNotify = passedReviewThreshold && body.review.state.toUpperCase() === 'APPROVED'; if (shouldNotify) { const mergerMessage = @@ -297,11 +302,14 @@ function sendPrUpdatedMessage(openerName, users, body) { async function prSynchronize(body) { const logId = shortid.generate(); - const opener = await findByGithubName(body.pull_request.user.login, logId); + const reviewer_list = body.pull_request.requested_reviewers; + + const [opener, ...reviewers] = await Promise.all( + findByGithubName(body.pull_request.user.login, logId), + ...reviewer_list.map(user => findByGithubName(user.login, logId)), + ); + const openerName = opener ? opener.name : body.pull_request.user.login; - const reviewers = await Promise.all(body.pull_request.requested_reviewers.map(user => { - return findByGithubName(user.login, logId); - })); return await sendPrUpdatedMessage(openerName, reviewers, body); } diff --git a/src/lib/github/webhookRouter.js b/src/lib/github/webhookRouter.js index 58fa712..9532b70 100644 --- a/src/lib/github/webhookRouter.js +++ b/src/lib/github/webhookRouter.js @@ -34,6 +34,7 @@ async function routeIt(body, { signature }) { throw new Error('Signatures do not match!'); } } + logger.info(`[RouteIt] ${body.action} on ${body.pull_request.base.repo.name}`); try { From 924374c42c3fd650954930f47dac94f8bf12f6e3 Mon Sep 17 00:00:00 2001 From: Simon Imbrogno Date: Fri, 7 Feb 2020 12:19:33 -0600 Subject: [PATCH 2/5] Collapse awaits in common --- src/lib/slack/common.js | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/lib/slack/common.js b/src/lib/slack/common.js index 481d964..a6ab605 100644 --- a/src/lib/slack/common.js +++ b/src/lib/slack/common.js @@ -5,30 +5,23 @@ const simpleGit = require('simple-git/promise')(appRoot.path); const users = require('../users'); async function generateAndSendBootMessage(channel = null, { msgText = null } = {}) { - const { available, benched } = await users.listAllUserNamesByAvailability(); - const SHA = await simpleGit.revparse(['HEAD']); + const [{ availableUsers, benchedUsers }, SHA] = await Promise.all([ + users.listAllUserNamesByAvailability(), + simpleGit.revparse(['HEAD']), + ]); + const messageObject = { text: msgText || `Git Slackin: ONLINE. SHA \`${SHA.trim()}\``, attachments: [ { text: '', color: 'good', - fields: [ - { - title: 'Available Users', - value: available, - }, - ], + fields: [{ title: 'Available Users', value: availableUsers }], }, { text: '', color: 'warning', - fields: [ - { - title: 'Benched Users', - value: benched, - }, - ], + fields: [{ title: 'Benched Users', value: benchedUsers }], }, ], }; @@ -36,7 +29,7 @@ async function generateAndSendBootMessage(channel = null, { msgText = null } = { if (channel && typeof channel === 'string') { return messenger.sendToChannel(channel, messageObject, { force: true }); } else { - logger.info(`Available Users: ${available}\nBenched Users: ${benched}`); + logger.info(`Available Users: ${availableUsers}\nBenched Users: ${benchedUsers}`); } } From 5ad9a5312a7ec9404a291598d816ee849acc7a2a Mon Sep 17 00:00:00 2001 From: Simon Imbrogno Date: Fri, 7 Feb 2020 12:19:52 -0600 Subject: [PATCH 3/5] Collapse awaits in eventHandler and users --- src/lib/slack/eventHandlers.js | 86 +++++++++++++++++++--------------- src/lib/users.js | 30 ++++++++---- src/server.js | 32 ++++++------- 3 files changed, 86 insertions(+), 62 deletions(-) diff --git a/src/lib/slack/eventHandlers.js b/src/lib/slack/eventHandlers.js index 7441106..aa1098b 100644 --- a/src/lib/slack/eventHandlers.js +++ b/src/lib/slack/eventHandlers.js @@ -30,8 +30,9 @@ async function updateConfigurations(configOverrides) { // especially if changing branches. async function updateGitSlackin(theEvent, branch = 'master') { let updateResult = null; + try { - // Let's discard these changes first. + // Discard any unstaged changes. await simpleGit.stash(); await simpleGit.stash(['drop']); @@ -43,17 +44,17 @@ async function updateGitSlackin(theEvent, branch = 'master') { } const triggeringUser = await findBySlackUserId(theEvent.user); - return sendToChannel(theEvent.channel, `Update trigger by ${triggeringUser.name}. Be back shortly! :wave:\n` + - `Changes: ${updateResult}`) - .then(() => { - // this works since we've already pulled so restarting should work. - return process.exit(0); - }); + + const message = `Update trigger by ${triggeringUser.name}. Be back shortly! :wave:\nChanges: ${updateResult}`; + await sendToChannel(theEvent.channel, message); + + // This works since we've already pulled so restarting should work. + return process.exit(0); } async function handleAdminCommands(command, theEvent, res, logId) { - if (!config.has('slack_manager_ids') - || !config.get('slack_manager_ids').includes(theEvent.user)) { + if (!config.has('slack_manager_ids') || + !config.get('slack_manager_ids').includes(theEvent.user)) { return sendEphemeralMessage(theEvent.channel, theEvent.user, 'This command is Admin-only or does not exist.'); } @@ -71,10 +72,8 @@ async function handleAdminCommands(command, theEvent, res, logId) { try { const newConfig = JSON.parse(setConfigRegexResult[1]); await updateConfigurations(newConfig); - return await sendToChannel(theEvent.channel, 'Updated config, restarting Git Slackin...') - .then(() => { - return process.exit(0); - }); + await sendToChannel(theEvent.channel, 'Updated config, restarting Git Slackin...'); + return process.exit(0); } catch (e) { return sendEphemeralMessage(theEvent.channel, theEvent.user, 'Error updating configuration'); } @@ -90,26 +89,33 @@ async function handleAdminCommands(command, theEvent, res, logId) { if (!slackUserIdToBench) { logger.warn(`[commands.admin.bench:${logId}] Could not find user to user ${slackUserIdToBench}`); - return await sendEphemeralMessage(theEvent.channel, theEvent.user, - `:whatsgoingon: I could not find the user '<@${slackUserIdToBench}>' to bench. ` + - `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``); + const failureMessage = `:whatsgoingon: I could not find the user '<@${slackUserIdToBench}>' to bench. ` + + `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``; + + return await sendEphemeralMessage(theEvent.channel, theEvent.user, failureMessage); } const success = await benchUserBySlackId(slackUserIdToBench, logId); if (success) { - await send(slackUserIdToBench, `You have been benched by <@${theEvent.user}>. ` + - 'Send me, Git Slackin, `start` to start receiving Review Requests again.'); + const responseMessage = `I have benched <@${slackUserIdToBench}> as requested.`; + const benchedUserMessage = `You have been benched by <@${theEvent.user}>. ` + + 'Send me, Git Slackin, `start` to start receiving Review Requests again.'; - return await sendEphemeralMessage(theEvent.channel, theEvent.user, - `I have benched <@${slackUserIdToBench}> as requested.`); + const [, result] = await Promise.all([ + send(slackUserIdToBench, benchedUserMessage), + sendEphemeralMessage(theEvent.channel, theEvent.user, responseMessage), + ]); + + return result; } else { const logId = shortid.generate(); logger.warn(`[commands.admin.bench:${logId}] Could not bench user ${slackUserIdToBench}`); - return await sendEphemeralMessage(theEvent.channel, theEvent.user, - `:whatsgoingon: I could not bench <@${slackUserIdToBench}> as requested. ` + - `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``); + const failureMessage = `:whatsgoingon: I could not bench <@${slackUserIdToBench}> as requested. ` + + `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``; + + return await sendEphemeralMessage(theEvent.channel, theEvent.user, failureMessage); } } @@ -118,26 +124,33 @@ async function handleAdminCommands(command, theEvent, res, logId) { if (!slackUserIdToUnbench) { logger.warn(`[commands.admin.bench:${logId}] Could not find user to user ${slackUserIdToUnbench}`); - return await sendEphemeralMessage(theEvent.channel, theEvent.user, - `:whatsgoingon: I could not find the user '<@${slackUserIdToUnbench}>' to unbench. ` + - `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``); + const failureMessage = `:whatsgoingon: I could not find the user '<@${slackUserIdToUnbench}>'. ` + + `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``; + + return await sendEphemeralMessage(theEvent.channel, theEvent.user, failureMessage); } const success = await activateUserBySlackId(slackUserIdToUnbench, logId); if (success) { - send(slackUserIdToUnbench, `You have been unbenched by <@${theEvent.user}>. ` + - 'Send me, Git Slackin, `start` to start receiving Review Requests again.'); + const responseMessage = `I have unbenched <@${slackUserIdToUnbench}> as requested.`; + const unbenchedUserMessage = `You have been unbenched by <@${theEvent.user}>. ` + + 'Send me, Git Slackin, `start` to start receiving Review Requests again.'; - return sendEphemeralMessage(theEvent.channel, theEvent.user, - `I have unbenched <@${slackUserIdToUnbench}> as requested.`); + const [, result] = await Promise.all([ + send(slackUserIdToUnbench, unbenchedUserMessage), + sendEphemeralMessage(theEvent.channel, theEvent.user, responseMessage), + ]); + + return result; } else { const logId = shortid.generate(); logger.warn(`[commands.admin.unbench:${logId}] Could not unbench user: ${slackUserIdToUnbench}`); - return await sendEphemeralMessage(theEvent.channel, theEvent.user, - `:whatsgoingon: I could not unbench <@${slackUserIdToUnbench}> as requested. ` + - `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``); + const failureMessage = `:whatsgoingon: I could not unbench <@${slackUserIdToUnbench}> as requested. ` + + `Please inform James if you think this is a bug. And refer to log code: \`${logId}\``; + + return await sendEphemeralMessage(theEvent.channel, theEvent.user, failureMessage); } } @@ -151,10 +164,7 @@ async function handleAdminCommands(command, theEvent, res, logId) { if (command === 'shutdown') { logger.info(`[ADMIN Event] ${theEvent.user} requested shutdown`); - return sendToChannel(theEvent.channel, 'Shutting down!') - .then(() => { - return process.exit(0); - }); + return sendToChannel(theEvent.channel, 'Shutting down!').then(() => process.exit(0)); } } @@ -259,7 +269,9 @@ async function handleCommands(text, theEvent, res, logId = 'NoId') { } if (smallText === 'status') { const user = await findBySlackUserId(theEvent.user); + logger.info(`[commands.user.status:${logId}] ${theEvent.user} requested their status.`); + return sendEphemeralMessage(theEvent.channel, theEvent.user, `You are <@${user.slack.id}> here and ` + ` on GitHub.\n` + `Your current Git Slackin' status is: ${user.requestable ? 'Requestable :yes:' : 'UnRequestable :no:'}.\n` + diff --git a/src/lib/users.js b/src/lib/users.js index 913cd27..0abd13f 100644 --- a/src/lib/users.js +++ b/src/lib/users.js @@ -106,7 +106,9 @@ async function benchUserBySlackId(id, logId) { } return user; }); + await synchronizeUserList(); + if (updated) { logger.info(`[users.benchUserBySlackId:${logId}] Benched user: ${id}. user_list file`); } else { @@ -127,7 +129,9 @@ async function activateUserBySlackId(id, logId) { } return user; }); + await synchronizeUserList(); + if (updated) { logger.info(`[users.activateUserBySlackId:${logId}] Benched user: ${id}. user_list file`); } else { @@ -143,9 +147,12 @@ async function muteNotificationsBySlackId(id, logId) { if (user.slack && user.slack.id.toLowerCase() === id.toLowerCase()) { user.notifications = false; } + return user; }); + await synchronizeUserList(); + logger.info('[USERS] Update user_list file'); return users; } @@ -157,24 +164,29 @@ async function unmuteNotificationsBySlackId(id, logId) { if (user.slack && user.slack.id.toLowerCase() === id.toLowerCase()) { user.notifications = true; } + return user; }); + await synchronizeUserList(); + return users; } async function listAllUserNamesByAvailability() { - const availableUsers = await listAvailableUsers(true); - const benchedUsers = await listBenchedUsers(true); - - let availableUsersString = availableUsers.join(); - let benchedUsersString = benchedUsers.join(); - if (availableUsersString.length === 0) availableUsersString = 'None'; - if (benchedUsersString.length === 0) benchedUsersString = 'None'; + const [available, benched] = await Promise.all([ + listAvailableUsers(true), + listBenchedUsers(true), + ]); + + const toString = (arr) => { + if (arr.length === 0) return 'None'; + return arr.join(); + }; return { - available: availableUsersString, - benched: benchedUsersString, + available: toString(available), + benched: toString(benched), }; } diff --git a/src/server.js b/src/server.js index 3d39aa6..39ff839 100644 --- a/src/server.js +++ b/src/server.js @@ -48,9 +48,11 @@ app.use(bodyParser.urlencoded({ extended: true })); // Basic web server to handle payloads app.post('/payload', (req, res) => { - if (req.headers['x-github-event'] === 'pull_request' || - req.headers['x-github-event'] === 'pull_request_review' || - req.headers['x-github-event'] === 'pull_request_review_comment') { + const eventType = req.headers['x-github-event']; + + if (eventType === 'pull_request' || + eventType === 'pull_request_review' || + eventType === 'pull_request_review_comment') { return githubWebhooks.handle(req.body, { signature: req.headers['x-hub-signature'], webhookId: req.headers['x-github-delivery'], @@ -100,17 +102,15 @@ http.createServer(app) logger.info(`server is listening on ${port} in mode: ${process.env.NODE_ENV}`); }); -if (!process.env.GS_INSECURE) { - https.createServer({ - key: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'key.pem')), - cert: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'cert.pem')), - ca: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'chain.pem')), - }, app) - .listen(httpsPort, (err) => { - if (err) { - return logger.error('something bad happened', err); - } +https.createServer({ + key: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'key.pem')), + cert: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'cert.pem')), + ca: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'chain.pem')), +}, app) + .listen(httpsPort, (err) => { + if (err) { + return logger.error('something bad happened', err); + } - logger.info(`server is listening on ${port} in mode: ${process.env.NODE_ENV}`); - }); -} + logger.info(`server is listening on ${port} in mode: ${process.env.NODE_ENV}`); + }); From d16ef1ca225e1e6e71975eb1ba5e2bae7353232d Mon Sep 17 00:00:00 2001 From: Simon Imbrogno Date: Fri, 7 Feb 2020 12:28:02 -0600 Subject: [PATCH 4/5] Undo clobbered change --- src/server.js | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/server.js b/src/server.js index 39ff839..82d1d08 100644 --- a/src/server.js +++ b/src/server.js @@ -102,15 +102,17 @@ http.createServer(app) logger.info(`server is listening on ${port} in mode: ${process.env.NODE_ENV}`); }); -https.createServer({ - key: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'key.pem')), - cert: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'cert.pem')), - ca: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'chain.pem')), -}, app) - .listen(httpsPort, (err) => { - if (err) { - return logger.error('something bad happened', err); - } +if (!process.env.GS_INSECURE) { + https.createServer({ + key: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'key.pem')), + cert: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'cert.pem')), + ca: fs.readFileSync(path.join(__dirname, '..', 'letsencrypt', 'chain.pem')), + }, app) + .listen(httpsPort, (err) => { + if (err) { + return logger.error('something bad happened', err); + } - logger.info(`server is listening on ${port} in mode: ${process.env.NODE_ENV}`); - }); + logger.info(`server is listening on ${port} in mode: ${process.env.NODE_ENV}`); + }); +} From 18dfdf07e06b616ce7d4505f8bf6ce2d6ba56c1a Mon Sep 17 00:00:00 2001 From: Simon Imbrogno Date: Mon, 10 Feb 2020 11:29:16 -0600 Subject: [PATCH 5/5] Fix variable Names --- src/lib/slack/common.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib/slack/common.js b/src/lib/slack/common.js index a6ab605..5c5dac6 100644 --- a/src/lib/slack/common.js +++ b/src/lib/slack/common.js @@ -5,7 +5,7 @@ const simpleGit = require('simple-git/promise')(appRoot.path); const users = require('../users'); async function generateAndSendBootMessage(channel = null, { msgText = null } = {}) { - const [{ availableUsers, benchedUsers }, SHA] = await Promise.all([ + const [{ available, benched }, SHA] = await Promise.all([ users.listAllUserNamesByAvailability(), simpleGit.revparse(['HEAD']), ]); @@ -16,12 +16,12 @@ async function generateAndSendBootMessage(channel = null, { msgText = null } = { { text: '', color: 'good', - fields: [{ title: 'Available Users', value: availableUsers }], + fields: [{ title: 'Available Users', value: available }], }, { text: '', color: 'warning', - fields: [{ title: 'Benched Users', value: benchedUsers }], + fields: [{ title: 'Benched Users', value: benched }], }, ], }; @@ -29,7 +29,7 @@ async function generateAndSendBootMessage(channel = null, { msgText = null } = { if (channel && typeof channel === 'string') { return messenger.sendToChannel(channel, messageObject, { force: true }); } else { - logger.info(`Available Users: ${availableUsers}\nBenched Users: ${benchedUsers}`); + logger.info(`Available Users: ${available}\nBenched Users: ${benched}`); } }