Skip to content

Commit

Permalink
fix(merge-on-green): revert adding cron job (#1264)
Browse files Browse the repository at this point in the history 
* Revert "fix(merge-on-green): hanging cron pr (#1261)"

This reverts commit 985b284.

* Revert "fix: add cron job to pick up hanging prs (#1217)"

This reverts commit 628e32c.
  • Loading branch information
@sofisl
sofisl committed Jan 6, 2021
1 parent 985b284 commit 473b7e6
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 454 deletions.
306 changes: 86 additions & 220 deletions packages/merge-on-green/src/merge-on-green.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
// limitations under the License.

// eslint-disable-next-line node/no-extraneous-import
import {Application, ProbotOctokit, Context} from 'probot';
import {Application, Context} from 'probot';
import {Datastore} from '@google-cloud/datastore';
import {mergeOnGreen} from './merge-logic';
import {logger} from 'gcf-utils';
Expand All @@ -39,12 +39,12 @@ interface WatchPR {
repo: string;
owner: string;
state: 'continue' | 'stop';
branchProtection?: string[];
branchProtection: string[];
label: string;
author: string;
url: string;
reactionId?: number;
installationId?: number;
reactionId: number;
installationId: number;
}

interface Label {
Expand Down Expand Up @@ -132,94 +132,21 @@ handler.cleanUpPullRequest = async function cleanUpPullRequest(
repo: string,
prNumber: number,
label: string,
reactionId: number | undefined,
github: InstanceType<typeof ProbotOctokit>
reactionId: number,
github: Context['github']
) {
await github.issues.removeLabel({
owner,
repo,
issue_number: prNumber,
name: label,
});
if (reactionId) {
await github.reactions.deleteForIssue({
owner,
repo,
issue_number: prNumber,
reaction_id: reactionId,
});
}
};

/**
* Checks if a branch with a PR has branch protection, if not, comments on PR
* @param owner type string
* @param repo type string
* @param prNumber type number
* @param github type githup API surface from payload
*/
handler.checkForBranchProtection = async function checkForBranchProtection(
owner: string,
repo: string,
prNumber: number,
github: InstanceType<typeof ProbotOctokit>
): Promise<string[] | undefined> {
let branchProtection: string[] | undefined;

// Check to see if branch protection exists
try {
branchProtection = (
await github.repos.getBranchProtection({
owner,
repo,
branch: 'master',
})
).data.required_status_checks.contexts;
logger.info(
`checking branch protection for ${owner}/${repo}: ${branchProtection}`
);
// if branch protection doesn't exist, leave a comment on the PR;
} catch (err) {
err.message = `Error in getting branch protection\n\n${err.message}`;
await github.issues.createComment({
owner,
repo,
issue_number: prNumber,
body:
"Your PR doesn't have any required checks. Please add required checks to your master branch and then re-add the label. Learn more about enabling these checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks.",
});
logger.error(err);
}
return branchProtection;
};

/**
* Attempts to create a reaction on the PR, returns the reaction ID
* @param owner type string
* @param repo type string
* @param prNumber type number
* @param github type githup API surface from payload
*/
handler.createReaction = async function createReaction(
owner: string,
repo: string,
prNumber: number,
github: InstanceType<typeof ProbotOctokit>
): Promise<number | undefined> {
let reactionId: number | undefined;
try {
reactionId = (
await github.reactions.createForIssue({
owner,
repo,
issue_number: prNumber,
content: 'eyes',
})
).data.id;
} catch (err) {
logger.error(err);
}
return reactionId;
await github.reactions.deleteForIssue({
owner,
repo,
issue_number: prNumber,
reaction_id: reactionId,
});
};

/**
Expand All @@ -229,17 +156,16 @@ handler.createReaction = async function createReaction(
* @param prNumber type number
* @param label type string
* @param reactionId type number or null
* @param url type string
* @param github type githup API surface from payload
*/
handler.checkIfPRIsInvalid = async function checkIfPRIsInvalid(
owner: string,
repo: string,
prNumber: number,
label: string,
reactionId: number | undefined,
reactionId: number,
url: string,
github: InstanceType<typeof ProbotOctokit>
github: Context['github']
) {
let pr;
let labels;
Expand Down Expand Up @@ -288,56 +214,29 @@ handler.checkIfPRIsInvalid = async function checkIfPRIsInvalid(
};

/**
* Adds a PR to datastore; first, checks if there's branch protection and adds a reaction Id
* Adds a PR to datastore when an automerge label is added to a PR
* @param url type string
* @param wp type Watch PR (owner, repo, pr number, state, url)
* @param github type githup API surface from payload
* @returns void
*/
handler.addPR = async function addPR(
wp: WatchPR,
url: string,
github: InstanceType<typeof ProbotOctokit>
) {
// Since a PR cannot be merged without required status checks, we'll check if they exist first
const branchProtection = await handler.checkForBranchProtection(
wp.owner,
wp.repo,
wp.number,
github
);
let reactionId: number | undefined;
// If the status checks exist, we'll try to react to the PR
if (branchProtection) {
try {
reactionId = await handler.createReaction(
wp.owner,
wp.repo,
wp.number,
github
);
} catch (err) {
logger.error(err);
}
//then, we'll add the PR to the Datastore table
const key = datastore.key([TABLE, url]);
const entity = {
key,
data: {
created: new Date().toJSON(),
owner: wp.owner,
repo: wp.repo,
number: wp.number,
branchProtection: branchProtection,
label: wp.label,
author: wp.author,
reactionId: wp.reactionId,
installationId: reactionId,
},
method: 'upsert',
};
await datastore.save(entity);
}
handler.addPR = async function addPR(wp: WatchPR, url: string) {
const key = datastore.key([TABLE, url]);
const entity = {
key,
data: {
created: new Date().toJSON(),
owner: wp.owner,
repo: wp.repo,
number: wp.number,
branchProtection: wp.branchProtection,
label: wp.label,
author: wp.author,
reactionId: wp.reactionId,
installationId: wp.installationId,
},
method: 'upsert',
};
await datastore.save(entity);
};

/**
Expand Down Expand Up @@ -401,7 +300,7 @@ handler.checkPRMergeability = async function checkPRMergeability(
wp.number,
[MERGE_ON_GREEN_LABEL, MERGE_ON_GREEN_LABEL_SECURE],
wp.state,
wp.branchProtection!,
wp.branchProtection,
wp.label,
wp.author,
github
Expand Down Expand Up @@ -432,70 +331,6 @@ handler.checkPRMergeability = async function checkPRMergeability(
}
};

//TODO: change the search query if expanding the allowlist to search for more authors
/**
* For a given repository, looks through all the PRs and checks to see if they have a MOG label
* @param context the context of the webhook payload
* @returns void
*/
handler.scanForMissingPullRequests = async function scanForMissingPullRequests(
github: InstanceType<typeof ProbotOctokit>
) {
// Github does not support searching the labels with 'OR'.
// The searching for issues is considered to be an "AND" instead of an "OR" .
const [issuesAutomergeLabel, issuesAutomergeExactLabel] = await Promise.all([
github.paginate(github.search.issuesAndPullRequests, {
q: `is:open is:pr user:googleapis user:GoogleCloudPlatform label:"${MERGE_ON_GREEN_LABEL}"`,
}),
github.paginate(github.search.issuesAndPullRequests, {
q: `is:open is:pr user:googleapis user:GoogleCloudPlatform label:"${MERGE_ON_GREEN_LABEL_SECURE}"`,
}),
]);
for (const issue of issuesAutomergeLabel) {
const pullRequestInDatastore: WatchPR = await handler.getPR(issue.html_url);
if (!pullRequestInDatastore) {
const ownerAndRepoArray = issue.repository_url.split('/');
const owner = ownerAndRepoArray[ownerAndRepoArray.length - 2];
const repo = ownerAndRepoArray[ownerAndRepoArray.length - 1];
await handler.addPR(
{
number: issue.number,
owner,
repo,
state: 'continue',
url: issue.html_url,
label: MERGE_ON_GREEN_LABEL,
author: issue.user.login,
},
issue.html_url,
github
);
}
}

for (const issue of issuesAutomergeExactLabel) {
const pullRequestInDatastore: WatchPR = await handler.getPR(issue.html_url);
if (!pullRequestInDatastore) {
const ownerAndRepoArray = issue.repository_url.split('/');
const owner = ownerAndRepoArray[ownerAndRepoArray.length - 2];
const repo = ownerAndRepoArray[ownerAndRepoArray.length - 1];
await handler.addPR(
{
number: issue.number,
owner,
repo,
state: 'continue',
url: issue.html_url,
label: MERGE_ON_GREEN_LABEL_SECURE,
author: issue.user.login,
},
issue.html_url,
github
);
}
}
};

// TODO: refactor into multiple function exports, this will take some work in
// gcf-utils.

Expand All @@ -515,15 +350,6 @@ function handler(app: Application) {
await handler.cleanDatastoreTable(watchedPRs, app, context);
return;
}

//because we're searching for the PRs, and not getting the installation ID, we have to use
//the bot's installation ID to call the API. So, we need to make sure it matches the repo owner
if (context.payload.find_hanging_prs === true) {
logger.info('Entering job to pick up any hanging PRs');
await handler.scanForMissingPullRequests(context.github);
return;
}

const start = Date.now();
await handler.checkPRMergeability(watchedPRs, app, context);
logger.info(`mergeOnGreen check took ${Date.now() - start}ms`);
Expand Down Expand Up @@ -570,20 +396,60 @@ function handler(app: Application) {
return;
}

await handler.addPR(
{
number: prNumber,
// check to see if the owner has branch protection rules
let branchProtection: string[] | undefined;
try {
branchProtection = (
await context.github.repos.getBranchProtection({
owner,
repo,
branch: 'master',
})
).data.required_status_checks.contexts;
logger.info(
`checking branch protection for ${owner}/${repo}: ${branchProtection}`
);
} catch (err) {
err.message = `Error in getting branch protection\n\n${err.message}`;
await context.github.issues.createComment({
owner,
repo,
state: 'continue',
url: context.payload.pull_request.html_url,
label: label.name,
author,
installationId,
},
context.payload.pull_request.html_url,
context.github
);
issue_number: prNumber,
body:
"Your PR doesn't have any required checks. Please add required checks to your master branch and then re-add the label. Learn more about enabling these checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks.",
});
logger.error(err);
}

// if the owner has branch protection set up, add this PR to the Datastore table
if (branchProtection) {
// try to create reaction. Save this reaction in the Datastore table since I don't (think)
// it is on the pull_request payload.
const reactionId = (
await context.github.reactions.createForIssue({
owner,
repo,
issue_number: prNumber,
content: 'eyes',
})
).data.id;

await handler.addPR(
{
number: prNumber,
owner,
repo,
state: 'continue',
url: context.payload.pull_request.html_url,
branchProtection: branchProtection,
label: label.name,
author,
reactionId,
installationId,
},
context.payload.pull_request.html_url
);
}
});

app.on(['pull_request.unlabeled'], async context => {
Expand Down

0 comments on commit 473b7e6

Please sign in to comment.