Skip to content

Commit

Permalink
chore: log with repository context
Browse files Browse the repository at this point in the history 
Allows to track down merge errors more easily.
  • Loading branch information
@nikku
nikku committed Feb 10, 2021
1 parent 9b02687 commit 1980083
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 49 deletions.
23 changes: 20 additions & 3 deletions lib/bot.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
111 changes: 65 additions & 46 deletions lib/core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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}`;
Expand Down Expand Up @@ -550,31 +556,31 @@ 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;
}

const { mergeMethod } = await getAppConfig(context);

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

Expand All @@ -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;
}

Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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
Expand All @@ -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 {

Expand All @@ -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 {
Expand All @@ -693,37 +703,41 @@ 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;
}

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;
}
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
Expand All @@ -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) {
Expand All @@ -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;
}
Expand Down Expand Up @@ -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) => {
Expand All @@ -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) {
Expand All @@ -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 = {
Expand Down

0 comments on commit 1980083

Please sign in to comment.