fix: add cron job to pick up hanging prs #1217

sofisl · 2020-12-05T03:59:08Z

This addresses #1159

Following Jeff's suggestion, this PR adds functionality for a cron job that will run once an hour.

I believe that, if we format the cron job correctly, we will be able to trigger the gcf-utils mechanism of rotating through all of the repositories under which MOG is installed.

Taking a lead from the SRS cron job, I think we can just check if the cron_org exists in the payload to trigger the rotation through all of the repos, which is different from the current MOG cron job in Cloud Scheduler.

I am cautious about this approach - we may trigger an abuse mechanism or hit our rate limit faster since we'd be checking all PRs in all our repos. I feel like we should try this solution, but if it doesn't work, we may want to write another bot that just picks up PRs.

chingor13 · 2020-12-08T20:40:05Z

packages/merge-on-green/src/merge-on-green.ts

+  const prs = await github.paginate(await github.pulls.list, {
+    owner,
+    repo,
+    state: 'open',
+  });


You can probably reduce the # of queries, by using the search API: https://docs.github.com/en/free-pro-team@latest/rest/reference/search#search-issues-and-pull-requests

chingor13 · 2020-12-08T20:41:14Z

packages/merge-on-green/src/merge-on-green.ts

+      // check to see if the owner has branch protection rules
+      let branchProtection: string[] | undefined;
+      try {
+        branchProtection = (
+          await 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 github.issues.createComment({
+          owner,
+          repo,
+          issue_number: pr.number,
+          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);


Should this logic share the same code as the webhook handling?

👍 I agree, I think the logic could be simplified if we:

check to see if the PR is already tracked in datastore (if it is tracked do nothing.).

if the PR is not tracked, but an automerge label exists, make a synthetic call to app.on('pull_request.labeled', async context; I think you could do this by looking at the fields you use from context, and then making sure you populate these in a fake context object you pass.

I'm not sure about faking an webhook - we could probably extract that common logic into a function to execute from both paths.

I simplified the logic here!

bcoe

Added a couple notes. I agree with @chingor13, I think you could be clever and make it so that finding a labeled PR that isn't tracked hits the same code path as adding a label.

bcoe · 2020-12-08T22:34:34Z

packages/merge-on-green/src/merge-on-green.ts

+        label.name === MERGE_ON_GREEN_LABEL_SECURE
+    );
+
+    if (labelFound) {


we should only do this if there's not already an entry in datastore for the given PR right?

bcoe · 2020-12-08T22:36:42Z

packages/merge-on-green/src/merge-on-green.ts

+      // check to see if the owner has branch protection rules
+      let branchProtection: string[] | undefined;
+      try {
+        branchProtection = (
+          await 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 github.issues.createComment({
+          owner,
+          repo,
+          issue_number: pr.number,
+          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);


👍 I agree, I think the logic could be simplified if we:

check to see if the PR is already tracked in datastore (if it is tracked do nothing.).

if the PR is not tracked, but an automerge label exists, make a synthetic call to app.on('pull_request.labeled', async context; I think you could do this by looking at the fields you use from context, and then making sure you populate these in a fake context object you pass.

…-bots into collectHangingPrs

sofisl · 2020-12-14T20:30:21Z

@chingor13 and @bcoe, made some adjustments, thanks for the review!

chingor13 · 2020-12-14T23:28:02Z

packages/merge-on-green/src/merge-on-green.ts

+ * @param context the context of the webhook payload
+ * @returns void
+ */
+handler.pickUpPR = async function pickUpPR(


nit: can we call this scanForMissingPullRequests or something like that?

chingor13 · 2020-12-14T23:31:18Z

packages/merge-on-green/src/merge-on-green.ts

+handler.addPR = async function addPR(
+  wp: WatchPR,
+  url: string,
+  github: Context['github']


Other bots are using this type:

repo-automation-bots/packages/gcf-utils/src/gcf-utils.ts

Line 41 in 78950c8

type ProbotOctokitType = InstanceType<typeof ProbotOctokit>;

chingor13 · 2020-12-14T23:32:30Z

packages/merge-on-green/src/merge-on-green.ts

  label: string;
  author: string;
  url: string;
-  reactionId: number;
-  installationId: number;
+  reactionId?: number | undefined;


nit: is undefined implicitly possible with the ?

chingor13 · 2020-12-14T23:36:21Z

packages/merge-on-green/src/merge-on-green.ts

+      await handler.pickUpPR(owner, repo, context.github);
+      return;
+    }
+
    const start = Date.now();
    await handler.checkPRMergeability(watchedPRs, app, context);


Is it worth trying to do these both async and awaiting them at the end? (it's going to complete all of pickUpPR before trying to do the merges.)

Oh, I'm expecting this to run on a separate cron job - we won't try merging on the same run as we try picking up the PRs.

chingor13 · 2020-12-14T23:38:29Z

packages/merge-on-green/src/merge-on-green.ts

+    await github.search.issuesAndPullRequests,
+    {
+      q:
+        'is:open is:pr user:googleapis user:GoogleCloudPlatform label:"automerge"',


use the constant?

chingor13 · 2020-12-14T23:38:34Z

packages/merge-on-green/src/merge-on-green.ts

+    await github.search.issuesAndPullRequests,
+    {
+      q:
+        'is:open is:pr user:googleapis user:GoogleCloudPlatform label:"automerge: exact"',


use the constant?

chingor13 · 2020-12-14T23:38:42Z

packages/merge-on-green/src/merge-on-green.ts

+          repo,
+          state: 'continue',
+          url: issue.html_url,
+          label: 'automerge',


use the constant?

chingor13 · 2020-12-14T23:38:49Z

packages/merge-on-green/src/merge-on-green.ts

+          repo,
+          state: 'continue',
+          url: issue.html_url,
+          label: 'automerge: exact',


use the constant?

chingor13 · 2020-12-14T23:39:56Z

packages/merge-on-green/src/merge-on-green.ts

+) {
+  // 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 = await github.paginate(


These 2 searches can probably be started at the same time then awaited before we want to start executing on them.

sofisl · 2020-12-16T23:59:02Z

@chingor13, really appreciate the in-depth review: it's super helpful to make sure we stay consistent within and outside the package. Thank you!

bcoe

This is looking good to me, thanks for DRY(don't repeat yourself)ing things up 👍

bcoe · 2020-12-17T22:29:33Z

packages/merge-on-green/test/merge-on-green.ts

@@ -960,6 +1226,7 @@ describe('merge-on-green', () => {

      //This function is supposed to respond with an error
      it('does not add a PR if branch protection errors and comments on PR', async () => {
+        addPRStub.restore();


one thought here, I think we could use Sinon's sandbox functionality and just always restart the sandbox in a beforeEach?

I think we are, line 852.

We need to restore sometimes in order to provide specific functionality for some specific tests.

If you need different setup scenarios, you can split describe blocks with separate beforeEach implementations.

chingor13 · 2020-12-23T17:27:47Z

packages/merge-on-green/test/merge-on-green.ts

@@ -960,6 +1226,7 @@ describe('merge-on-green', () => {

      //This function is supposed to respond with an error
      it('does not add a PR if branch protection errors and comments on PR', async () => {
+        addPRStub.restore();


If you need different setup scenarios, you can split describe blocks with separate beforeEach implementations.

This reverts commit 628e32c.

* 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.

fix: add cron job to pick up hanging prs

7881e54

sofisl requested a review from a team December 5, 2020 03:59

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 5, 2020

sofisl requested review from bcoe and chingor13 December 5, 2020 03:59

sofisl and others added 2 commits December 4, 2020 19:59

npm run lint

c248993

Merge branch 'master' into collectHangingPrs

6fc6505

chingor13 requested changes Dec 8, 2020

View reviewed changes

bcoe suggested changes Dec 8, 2020

View reviewed changes

sofisl added 4 commits December 11, 2020 11:16

fix: address comments

f6a4842

save for now

4c6daff

fix: address comments

0a15f1a

Merge branch 'collectHangingPrs' of github.com:sofisl/repo-automation…

8a89d49

…-bots into collectHangingPrs

sofisl requested review from chingor13 and bcoe December 12, 2020 09:09

sofisl added 2 commits December 12, 2020 01:21

run lint

2440f97

remove var declaration

8ff02f6

chingor13 reviewed Dec 14, 2020

View reviewed changes

fix: further comments

38d3d8f

Merge branch 'master' into collectHangingPrs

82edf1c

sofisl requested a review from chingor13 December 17, 2020 17:22

bcoe approved these changes Dec 17, 2020

View reviewed changes

Merge branch 'master' into collectHangingPrs

2550522

chingor13 approved these changes Dec 23, 2020

View reviewed changes

sofisl merged commit 628e32c into googleapis:master Dec 23, 2020

chingor13 mentioned this pull request Dec 23, 2020

Merge-on-Green: sometimes misses pull requests #1159

Closed

sofisl added a commit that referenced this pull request Jan 6, 2021

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

5cf2f16

This reverts commit 628e32c.

sofisl mentioned this pull request Jan 9, 2021

fix(merge-on-green): second attempt to add cron to pick up hanging PRs #1269

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add cron job to pick up hanging prs #1217

fix: add cron job to pick up hanging prs #1217

sofisl commented Dec 5, 2020

chingor13 Dec 8, 2020

chingor13 Dec 8, 2020

bcoe Dec 8, 2020

chingor13 Dec 9, 2020

sofisl Dec 12, 2020

bcoe left a comment

bcoe Dec 8, 2020

bcoe Dec 8, 2020

sofisl commented Dec 14, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

sofisl Dec 16, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

chingor13 Dec 14, 2020

sofisl commented Dec 16, 2020

bcoe left a comment

bcoe Dec 17, 2020

sofisl Dec 23, 2020

chingor13 Dec 23, 2020

chingor13 Dec 23, 2020

fix: add cron job to pick up hanging prs #1217

fix: add cron job to pick up hanging prs #1217

Conversation

sofisl commented Dec 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl commented Dec 16, 2020

bcoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment