fix(merge-on-green): add cleanup cron job, fix blocking issue for removing #1112

sofisl · 2020-11-12T08:31:04Z

I did some further digging into this multiple commenting issue, that did not seem to be solved by #1091. I think the real issue here is when people add an 'automerge' label and then either remove that label, close, or merge that PR soon after. I think a race condition is happening where the PR is not yet recorded in the Datastore table when the PR is closed or removed, making so that it doesn't actually get removed.

I don't know how to solve this problem categorically. What I ended up doing is creating a clean-up cron job that runs every 30 minutes. Every 30 minutes, if the cron job is the 'cleanup' job, the app will go through the PRs in the Datastore table and clean up ones that are either closed, merged, or do not have an automerge label on them anymore.

tbpg

Design question: could we do this cleanup "check" before doing anything with a PR in the normal loop/cron? That way, there won't be a delay between the problematic state and when it gets fixed by the next cleanup cron iteration.

Also, I saw the multiple-comment behavior on #1111, which doesn't seem like the same issue?

tbpg · 2020-11-12T13:16:42Z

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

@@ -148,6 +148,59 @@ handler.cleanUpPullRequest = async function cleanUpPullRequest(
    .catch(logger.error);
 };

+/**
+ * Check if PR has been merged, closed, or unlabeled, then remove


Missing end of sentence?

sofisl · 2020-11-12T20:23:55Z

Thanks for the review @tbpg!

Design question: could we do this cleanup "check" before doing anything with a PR in the normal loop/cron? That way, there won't be a delay between the problematic state and when it gets fixed by the next cleanup cron iteration.

I think what you're asking is if we can check to see if the PR is closed/merged/unlabeled before doing something in the regular workflow. This was the previous implementation of the bot, but I changed it in #1023 because checking this info was causing us to run into rate limits very quickly. Now, we wait for the unlabeled/closed/merged event to update the Datastore table. But, now we're running into a separate issue on whether or not the PR is in the table before it gets deleted if it gets added and deleted in the table too quickly.

But, maybe we run the cleanup cron job every 15 minutes, instead of every 30? It is an expensive task to do, but it might be worth to address these issues. I'll leave that suggestion up to you and/or anyone else who has thoughts on it :)

Also, I saw the multiple-comment behavior on #1111, which doesn't seem like the same issue?

Funny you mention that PR, it is actually the same issue! I was testing it last night. I quickly added and removed the automerge label.

If you see there, MOG actually ended up merging that PR, even though it didn't have an automerge label on it. I am not exactly clear on what causes the multiple commenting issue, but I do know that this fix should solve it, since that PR shouldn't have even been in the Datastore table to begin with.

…n-bots into fixCommentingIssue

tmatsuo · 2020-11-12T20:51:59Z

packages/sync-repo-settings/src/sync-repo-settings.ts

@@ -26,7 +26,7 @@ import Ajv from 'ajv';
 import yaml from 'js-yaml';
 import {PullsListFilesResponseData} from '@octokit/types';

-export const configFileName = 'sync-repo-settings.yaml';
+export const configFileName = 'sync-repo-settings.yml';


There seem to be some changes unrelated?

oof, thanks for the heads up. Need to resync to main branch.

tmatsuo · 2020-11-12T23:01:35Z

packages/sync-repo-settings/src/sync-repo-settings.ts

@@ -340,12 +340,6 @@ async function updateRepoTeams(
  logger.info(`Update team access for ${repo}`);
  const [owner, name] = repo.split('/');



Is this also an irrelevant change?

bcoe · 2020-11-12T23:01:57Z

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

+        assert(removePRStub.called);
+      });
+
+      it('deletes a PR if label is not found when cleaning up repository', async () => {


awesome to see the increased coverage 👍

bcoe

The logic in schedule.repository is getting pretty expansive, perhaps we could split it into two helpers that get called depending on the type of payload:

runCleanUp/runMergeOnGreen or something like that?

bcoe · 2020-11-12T23:02:28Z

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

@@ -210,15 +297,19 @@ function handler(app: Application) {
              github as any
            );
            if (remove || wp.state === 'stop') {
+              await handler.removePR(wp.url);
+              try {


spacing looks a little weird here, might need to run npm run fix.

bcoe · 2020-11-12T23:03:21Z

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

@@ -188,6 +252,29 @@ function handler(app: Application) {
  //open issue for a fix: https://github.com/octokit/webhooks.js/issues/277
  app.on('schedule.repository' as any, async context => {
    const watchedPRs = await handler.listPRs();
+    if (context.payload.cleanUp === true) {
+      while (watchedPRs.length) {


I might be tempted to pull this into a helper function.

tmatsuo · 2020-11-13T04:50:56Z

This is kind of meta comment. Take this a grain of solt.

The PR title should be something like:

fix(merge-on-green): add cleanup cron job

Maybe we should use package name in the paren
More concrete title is better in my opinion

…n-bots into fixCommentingIssue

tbpg

A few small comments. Otherwise, LGTM.

tbpg · 2020-11-13T18:33:35Z

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

+      })
+    ).data;
+  } catch (err) {
+    pr = undefined;


Can this return early rather than continue unnecessarily?

I might leave this just in case the github API is unavailable for some reason, but we can confirm by checking to see if a label is on the PR in the next function.

tbpg · 2020-11-13T18:34:16Z

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

+      label.name === MERGE_ON_GREEN_LABEL_SECURE
+  );
+
+  if (pr?.merged === true || pr?.state === 'closed' || !foundLabel) {


Suggested change

if (pr?.merged === true || pr?.state === 'closed' || !foundLabel) {

if (pr?.merged || pr?.state === 'closed' || !foundLabel) {

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

VERSION is used in @google-cloud/cloud-rad to publish ref docs for a particular version. Pass VERSION in via Stubby or Fusion. Source-Link: googleapis/synthtool@740366b Post-Processor: gcr.io/repo-automation-bots/owlbot-nodejs:latest@sha256:bbdd52de226c00df3356cdf25460397b429ab49552becca645adbc412f6a4ed5

sofisl added 5 commits October 19, 2020 13:49

feat

de4e23d

Merge branch 'master' of github.com:googleapis/repo-automation-bots

671c152

Merge branch 'master' of github.com:googleapis/repo-automation-bots

73612e6

fix: multiple commenting issues

897c7d3

return cleanup function once it finishes

09981c5

sofisl requested a review from a team November 12, 2020 08:31

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

tbpg suggested changes Nov 12, 2020

View reviewed changes

Merge branch 'master' into fixCommentingIssue

a4ed3c9

sofisl added 2 commits November 12, 2020 12:26

npm run lint

869a22f

Merge branch 'fixCommentingIssue' of github.com:sofisl/repo-automatio…

42a7534

…n-bots into fixCommentingIssue

tmatsuo reviewed Nov 12, 2020

View reviewed changes

sofisl requested a review from tbpg November 12, 2020 22:19

sofisl added 5 commits November 12, 2020 14:45

revert irrelevant changes

3b24381

add try-catch block when deleting the reaction and label

13c37c2

add comment

5d74f1c

revert file

d33bb24

revert

f3581e7

sofisl requested a review from tmatsuo November 12, 2020 22:59

tmatsuo reviewed Nov 12, 2020

View reviewed changes

bcoe reviewed Nov 12, 2020

View reviewed changes

bcoe suggested changes Nov 12, 2020

View reviewed changes

sofisl added 2 commits November 12, 2020 15:50

npm run lint

48fb4e7

revert to head

d22d430

sofisl requested review from bcoe and tmatsuo November 12, 2020 23:53

bcoe approved these changes Nov 13, 2020

View reviewed changes

fix broken tests;

d883e69

sofisl and others added 2 commits November 13, 2020 10:00

npm run fix

a1ce5c7

Merge branch 'master' into fixCommentingIssue

c959f9b

sofisl changed the title ~~fix: multiple commenting issue~~ fix(merge-on-green): add cleanup cron job Nov 13, 2020

sofisl changed the title ~~fix(merge-on-green): add cleanup cron job~~ fix(merge-on-green): add cleanup cron job, fix blocking issue for removing Nov 13, 2020

sofisl added 2 commits November 13, 2020 10:04

add logging

e4f7e65

Merge branch 'fixCommentingIssue' of github.com:sofisl/repo-automatio…

55f9836

…n-bots into fixCommentingIssue

tbpg approved these changes Nov 13, 2020

View reviewed changes

address comment

af6b077

sofisl merged commit 7a9ac98 into googleapis:master Nov 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(merge-on-green): add cleanup cron job, fix blocking issue for removing #1112

fix(merge-on-green): add cleanup cron job, fix blocking issue for removing #1112

sofisl commented Nov 12, 2020

tbpg left a comment

tbpg Nov 12, 2020

sofisl commented Nov 12, 2020

tmatsuo Nov 12, 2020

sofisl Nov 12, 2020

tmatsuo Nov 12, 2020

bcoe Nov 12, 2020

bcoe left a comment

bcoe Nov 12, 2020

bcoe Nov 12, 2020

tmatsuo commented Nov 13, 2020

tbpg left a comment

tbpg Nov 13, 2020

sofisl Nov 13, 2020

tbpg Nov 13, 2020

		@@ -340,12 +340,6 @@ async function updateRepoTeams(
		logger.info(`Update team access for ${repo}`);
		const [owner, name] = repo.split('/');

	if (pr?.merged === true \|\| pr?.state === 'closed' \|\| !foundLabel) {
	if (pr?.merged \|\| pr?.state === 'closed' \|\| !foundLabel) {

fix(merge-on-green): add cleanup cron job, fix blocking issue for removing #1112

fix(merge-on-green): add cleanup cron job, fix blocking issue for removing #1112

Conversation

sofisl commented Nov 12, 2020

tbpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sofisl commented Nov 12, 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

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

tmatsuo commented Nov 13, 2020

tbpg 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