diff --git a/lib/bot.js b/lib/bot.js index 5db41af..7b52473 100644 --- a/lib/bot.js +++ b/lib/bot.js @@ -61,8 +61,13 @@ async function handleCheckSuiteCompleted(context) { pull_requests } = check_suite; + const ctx = context.repo({ + head_sha: context.payload.check_suite.head_sha + }); + if (conclusion !== 'success') { - context.log.info(`skipping: check_suite conclusion == ${conclusion}`); + context.log.info(ctx, `skipping: check_suite conclusion == ${conclusion}`); + return; } @@ -72,6 +77,11 @@ async function handleCheckSuiteCompleted(context) { const pullRequest = await findPullRequestByShallowRef(context, pull_requests[0]); if (!pullRequest) { + context.log.warn({ + ...ctx, + pull_number: pullRequest.number + }, 'skipping: no PR found for shallow ref'); + return; } @@ -93,7 +103,9 @@ async function handlePullRequestReviewSubmitted(context) { } = context.payload; if (review.state !== 'approved') { - context.log.info(`skipping: review in state ${review.state}`); + context.log.info(context.repo({ + pull_number: pull_request.number + }), `skipping: review in state ${review.state}`); return; } @@ -130,14 +142,19 @@ async function handleStatus(context) { state } = context.payload; + const ctx = context.repo({ + status_ref: context.payload.sha + }); + if (state !== 'success') { - context.log.info(`skipping: status == ${state}`); + context.log.info(ctx, `skipping: status == ${state}`); return; } const pullRequest = await findPullRequestByStatus(context, context.payload); if (!pullRequest) { + context.log.debug(ctx, `skipping: status == ${state}`); return; } diff --git a/lib/core.js b/lib/core.js index 70615bf..7a7e573 100644 --- a/lib/core.js +++ b/lib/core.js @@ -291,22 +291,29 @@ async function getCollaboratorReviews(context, pullRequest) { */ async function getReviewApproval(context, pullRequest) { + const ctx = context.repo({ + pull_number: pullRequest.number + }); + const config = await getReviewConfig(context, pullRequest); - context.log.debug(PR(pullRequest, { config }), 'checking review approval'); + context.log.debug({ + ...ctx, + config + }, 'checking review approval'); const reviews = await getCollaboratorReviews(context, pullRequest); - context.log.debug(PR(pullRequest), `found ${reviews.length} collaborator reviews`); + context.log.debug(ctx, `found ${reviews.length} collaborator reviews`); if (reviews.some(isChangesRequested)) { - context.log.debug(PR(pullRequest), 'skipping: reviews request changes'); + context.log.debug(ctx, 'skipping: reviews request changes'); return CHANGES_REQUESTED; } if (reviews.filter(isApproved).length < config.minApprovals) { - context.log.debug(PR(pullRequest), `skipping: lacks minApprovals=${config.minApprovals}`); + context.log.debug(ctx, `skipping: lacks minApprovals=${config.minApprovals}`); return REVIEWS_MISSING; } @@ -325,7 +332,7 @@ async function getReviewApproval(context, pullRequest) { } - context.log.debug(PR(pullRequest), 'approved via review(s)'); + context.log.debug(ctx, 'approved via review(s)'); return APPROVED; } @@ -502,7 +509,6 @@ async function getStatusApproval(context, pullRequest) { // quick reject if there are unsuccessful status checks if (statuses.length && statusState !== SUCCESS) { - context.log.debug(`skipping: combined status == ${statusState}`); // returns STATUS_FAILED, STATUS_PENDING return `STATUS_${statusState}`; @@ -550,15 +556,17 @@ async function getStatusApproval(context, pullRequest) { */ async function canMerge(context, pullRequest) { - if (pullRequest.draft) { - context.log.info(PR(pullRequest), 'skipping: pull request is a draft'); + const ctx = context.repo({ + pull_number: pullRequest.number + }); + if (pullRequest.draft) { + context.log.info(ctx, 'skipping: pull request is a draft'); return false; } if (pullRequest.merged) { - context.log.info(PR(pullRequest), 'skipping: pull request is already merged'); - + context.log.info(ctx, 'skipping: pull request is already merged'); return false; } @@ -566,15 +574,13 @@ async function canMerge(context, pullRequest) { // rebaseable => true, false or null if (mergeMethod === 'rebase' && !pullRequest.rebaseable) { - context.log.info(PR(pullRequest), 'skipping: pull request is not rebaseable'); - + context.log.info(ctx, 'skipping: pull request is not rebaseable'); return false; } // mergeable => true, false or null if (mergeMethod === 'merge' && !pullRequest.mergeable) { - context.log.info(PR(pullRequest), 'skipping: pull request is not mergeable'); - + context.log.info(ctx, 'skipping: pull request is not mergeable'); return false; } @@ -589,7 +595,7 @@ async function canMerge(context, pullRequest) { // we always attempt to merge if a branch is protected; // GitHub enforces the protection and our merge will fail if (branchProtected) { - context.log.debug('branch is protected, merge check skipped'); + context.log.debug(ctx, 'branch is protected, merge check skipped'); return true; } @@ -602,14 +608,14 @@ async function canMerge(context, pullRequest) { // reviews: minimum one for external PRs and a // configurable minimum of reviews for - context.log.debug(PR(pullRequest), 'checking status and reviews'); + context.log.debug(ctx, 'checking status and reviews'); // (1) verify checks + status ////////// const statusApproval = await getStatusApproval(context, pullRequest); if (statusApproval !== SUCCESS) { - context.log.info(PR(pullRequest), `skipping: failed status check (${statusApproval})`); + context.log.info(ctx, `skipping: failed status check (${statusApproval})`); return false; } @@ -619,12 +625,12 @@ async function canMerge(context, pullRequest) { const reviewApproval = await getReviewApproval(context, pullRequest); if (reviewApproval !== APPROVED) { - context.log.info(PR(pullRequest), `skipping: failed review check (${reviewApproval})`); + context.log.info(ctx, `skipping: failed review check (${reviewApproval})`); return false; } - context.log.debug(PR(pullRequest), 'merge check passed'); + context.log.debug(ctx, 'merge check passed'); return true; } @@ -640,6 +646,10 @@ async function canMerge(context, pullRequest) { */ async function merge(context, pullRequest) { + const ctx = context.repo({ + pull_number: pullRequest.number + }); + const { number, head @@ -651,7 +661,7 @@ async function merge(context, pullRequest) { const { mergeMethod } = await getAppConfig(context); - context.log.debug(PR(pullRequest), `attempting merge (method=${mergeMethod})`); + context.log.debug(ctx, `attempting merge (method=${mergeMethod})`); try { @@ -673,7 +683,7 @@ async function merge(context, pullRequest) { // 404 - not found // 409 - merge conflict - context.log.info(PR(pullRequest), `merge failed (message=${error.message}, status=${error.status})`); + context.log.info(ctx, `merge failed (message=${error.message}, status=${error.status})`); return false; } else { @@ -693,25 +703,29 @@ async function merge(context, pullRequest) { */ async function checkMerge(context, pullRequest) { - context.log.info(PR(pullRequest), 'checking merge'); + const ctx = context.repo({ + pull_number: pullRequest.number + }); + + context.log.info(ctx, 'checking merge'); let shouldMerge = false; try { shouldMerge = await canMerge(context, pullRequest); - } catch (err) { + } catch (error) { - if (isConfigError(err)) { - context.log.warn(PR(pullRequest), `skipping: ${err.message}`); - } else if (isMergeCheckError(err)) { - context.log.debug(PR(pullRequest), `skipping: ${err.message}`); + if (isConfigError(error)) { + context.log.warn(ctx, `skipping: ${error.message}`); + } else if (isMergeCheckError(error)) { + context.log.debug(ctx, `skipping: ${error.message}`); } else { - throw err; + throw error; } } if (!shouldMerge) { - context.log.info(PR(pullRequest), 'skipping: merge rejected'); + context.log.info(ctx, 'skipping: merge rejected'); return false; } @@ -719,11 +733,11 @@ async function checkMerge(context, pullRequest) { const merged = await merge(context, pullRequest); if (merged) { - context.log.info(PR(pullRequest), 'merged'); + context.log.info(ctx, 'merged'); return true; } else { - context.log.info(PR(pullRequest), 'skipping: merge failed'); + context.log.info(ctx, 'skipping: merge failed'); return false; } @@ -766,6 +780,10 @@ function getPullRequestTargetOrg(pullRequest) { */ async function getTeamsApproval(context, pullRequest, config, reviews) { + const ctx = context.repo({ + pull_number: pullRequest.number + }); + const { minApprovals, reviewTeams: configuredTeams @@ -784,7 +802,10 @@ async function getTeamsApproval(context, pullRequest, config, reviews) { const effectiveTeams = getEffectiveReviewTeams(teams, reviewers, requestedTeams); - context.log.debug(PR(pullRequest, { effectiveTeams }), 'effective review teams'); + context.log.debug({ + ...ctx, + effectiveTeams + }, 'effective review teams'); return getTeamsReviewApproval(context, pullRequest, reviews, effectiveTeams, minApprovals); } @@ -801,6 +822,10 @@ async function getTeamsApproval(context, pullRequest, config, reviews) { */ async function getTeamsReviewApproval(context, pullRequest, reviews, teams, minApprovals) { + const ctx = context.repo({ + pull_number: pullRequest.number + }); + const approvals = reviews.filter(isApproved); for (const team of teams) { @@ -810,15 +835,15 @@ async function getTeamsReviewApproval(context, pullRequest, reviews, teams, minA ); if (teamsApprovals.length < minApprovals) { - context.log.debug(PR(pullRequest), `skipping: lacks minApprovals=${minApprovals} by team ${team.name}`); + context.log.debug(ctx, `skipping: lacks minApprovals=${minApprovals} by team ${team.name}`); return REVIEWS_MISSING; } - context.log.debug(PR(pullRequest), `approved by team ${team.name}`); + context.log.debug(ctx, `approved by team ${team.name}`); } - context.log.debug(PR(pullRequest), 'approved via team review(s)'); + context.log.debug(ctx, 'approved via team review(s)'); return APPROVED; } @@ -898,6 +923,10 @@ function getEffectiveReviewTeams(teams, reviewers, requestedTeams) { */ async function getTeamsWithMembers(context, pullRequest, reviewTeams) { + const ctx = context.repo({ + pull_number: pullRequest.number + }); + const org = getPullRequestTargetOrg(pullRequest); const teamMembers = await Promise.all(reviewTeams.map(async (teamSlug) => { @@ -909,7 +938,7 @@ async function getTeamsWithMembers(context, pullRequest, reviewTeams) { team_slug: teamSlug }).catch(error => { - context.log.debug(`failed to fetch team ${teamSlug}`, error); + context.log.debug(ctx, `failed to fetch team ${teamSlug}`, error); // app is missing missing permissions if (error.status === 403) { @@ -935,16 +964,6 @@ async function getTeamsWithMembers(context, pullRequest, reviewTeams) { ); } -/** - * @param { { number: number } } pullRequest - */ -function PR(pullRequest, details={}) { - return { - pull_number: pullRequest.number, - ...details - }; -} - // module exports ////////////////////////////// module.exports = {