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

sofisl · 2021-01-09T08:15:08Z

Attempt to readdress #1159, fixing the issue that caused reverting in #1264.

I believe the issue was that reaction IDs were not being added to stored PRs, causing the main (merge-logic) to fail. This was because I was attempting to make reaction and reaction IDs optional. To fix, I've essentially copied the exact format in which PRs are currently being added, and made it into its own function (addPR).

I have also been doing some manual testing in the GCF function just to confirm this works. I was able to spot another potential bug, which is that we need to perform scans for missing PRs on different cron jobs with different installation IDs depending on the org, otherwise we won't have access to info like branch protection, etc.

I've also rescoped tests, which was a hanging comment left from #1217

bcoe

This is looking really solid to me, a few things jumped out at me:

it feels a bit strange tome that we're checking branch protection in merge-on-green.ts, because I thought our, _"should we merge it?", logic lived elsewhere. I see this logic isn't new, so it might just be I'm not understanding a valuable optimization that's been added.
non blocker: It jumped out at me that we should perhaps have two sets of tests, those using nock, and those new ones you're adding using sinon (I like the movement towards sinon.)
I think we could cleanup tests quite a bit using sinon's sandbox API, and think this would be worth considering.

bcoe · 2021-01-12T02:29:20Z

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

+  owner: string,
+  repo: string,
+  prNumber: number,
+  github: InstanceType<typeof ProbotOctokit>


nit, you could potentially do this once by going:

type GitHubType = InstanceType<typeof ProbotOctokit>

towards the top of the file.

bcoe · 2021-01-12T02:34:24Z

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

+) {
+  let branchProtection: string[] | undefined;
+  try {
+    branchProtection = await handler.checkForBranchProtection(


It feels a little strange to me that we're checking branchProtection before adding the PR, since this is something we check in merge-logic.ts right?

From a separation of functionality point of view, I'd be tempted to try to keep all the logic for if something should be merged in merge-logic.ts, whereas merge-on-green.ts just handles:

receiving Webhooks from GitHub

adding PRs to the DB (either based on webhook or your periodic cron job).

Yeah! I think that was the thinking behind our original logic. The reason I changed where we check for branch protection is because it's a way of reducing API requests during the main merge-logic, especially since branch protection isn't really the kind of information that changes within the life of a PR (so it's not the kind of thing we need to check multiple times before merging a PR). The way we currently have it set up allows us to only check for branch protection once, and then we save it in our Datastore table and don't have to make that API call again. Another reason (though this one is weaker) is that it kind of makes sense for us to only add a PR to our queue of PRs to check if it has branch protection. In other words, instead of: (a) adding a PR to datastore, (b) checking if it has branch protection, (c) deleting the PR, we could save ourselves two steps and just never add the PR to Datastore if it doesn't have branch protection. Open to hearing your thoughts on this process!

My temptation would be to still have the logic for checking branch protection inside of the merge-logic.js class, and you could just call that method here (rather than having the logic in two places).

This doesn't need to be a blocker for this PR, but perhaps we could make a tracking ticket to do this?

bcoe · 2021-01-12T02:34:50Z

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

+      q: `is:open is:pr user:${org} label:"${MERGE_ON_GREEN_LABEL}"`,
+    }),
+    github.paginate(github.search.issuesAndPullRequests, {
+      q: `is:open is:pr user:${org} label:"${MERGE_ON_GREEN_LABEL_SECURE}"`,


I love the search approach that you came up with for this.

bcoe · 2021-01-12T02:35:43Z

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

+      // we cannot search in an org without the bot installation ID, so we need
+      // to divide up the cron jobs based on org
+      if (context.payload.org === 'googleapis') {
+        await handler.scanForMissingPullRequests(context.github, 'googleapis');


rather than using an if statement here, you could just do:

await handler.scanForMissingPullRequests(context.github, context.payload.org);

We could... we're just going to end up with a lot of errors in the logs and additional API calls that we don't need to make. I'm not sure what is better here, less lines of code and more logs or vice versa!

what I"m getting at is that context.payload.org is going to be one of googleapis or GoogleCloudPlatform I believe?

Why do we return early for GoogleCloudPlatform but not googleapis, out of curiosity?

bcoe · 2021-01-12T02:36:34Z

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

-          payload: {org: 'testOwner', cleanUp: true},
-          id: 'abc123',
+      it('adds a PR if branch protection when PR labeled', async () => {
+        //addPRStub.restore();


we can drop this commented out bit I believe. I'd be tempted to use Sinon's sandbox mode, and just always reset the sandbox in a before filter.

Oops! Thanks for catching that! I was testing to make sure the sandboxing worked.

bcoe · 2021-01-12T02:37:01Z

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

        await probot.receive({
          name: 'pull_request',
          payload,
          id: 'abc123',
        });

        scopes.forEach(s => s.done());
-        assert(addPRStub.called);
+        addPRStub.restore();


I think a beforeEach that does the sandbox dance would simplify this.

bcoe · 2021-01-12T02:43:59Z

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

-        logger.info('stub called? ' + addPRStub.called);
+      });
+    });
+    describe('stubbing all GCP functions', () => {


I wouldn't make the describe block describe "how you're going to write your tests" (_which is just an implementation detail), rather it should describe what you're testing.

If I'm following, we're moving away from nock based tests with these new tests towards using sinon more?

What I might do is go so far as splitting it across two test files:

integration.js <-- integration(ish) tests with all our nock tests.

unit.js <-- unit tests using sinon mianly for mocking.

This could be follow up work, not a blocker 👍

I ended up dividing the functions into merge-logic vs. merge-on-green wrapper logic, which I think does a better job of describing the split (although, to your point, maybe it's worth adding more integration tests to have its own separate file)

…tion-bots into addCronHangingPrs

bcoe

This is mostly looking good to me 👍

I'm still confused by why we need to have this:

if (context.payload.org === 'googleapis') {

When we could just pass that string as input to scanForMissingPullRequests?

Otherwise I just had a future refactoring nit.

bcoe · 2021-01-13T19:43:11Z

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

+) {
+  let branchProtection: string[] | undefined;
+  try {
+    branchProtection = await handler.checkForBranchProtection(


My temptation would be to still have the logic for checking branch protection inside of the merge-logic.js class, and you could just call that method here (rather than having the logic in two places).

This doesn't need to be a blocker for this PR, but perhaps we could make a tracking ticket to do this?

bcoe · 2021-01-13T19:46:08Z

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

+      // we cannot search in an org without the bot installation ID, so we need
+      // to divide up the cron jobs based on org
+      if (context.payload.org === 'googleapis') {
+        await handler.scanForMissingPullRequests(context.github, 'googleapis');


what I"m getting at is that context.payload.org is going to be one of googleapis or GoogleCloudPlatform I believe?

Why do we return early for GoogleCloudPlatform but not googleapis, out of curiosity?

bcoe · 2021-01-13T19:48:47Z

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

    });
  });
-
-  describe('merge-on-green wrapper logic', () => {
+  suite('stubbing all GCP functions', () => {


This seems like a good middle-ground, compared to breaking out the tests into multiple files, or completely restructuring tests.

sofisl · 2021-01-13T22:42:04Z

Thanks Ben!

For this comment: if (context.payload.org === 'googleapis')
You were right, I was confused! I fixed it to just await handler.scanForMissingPullRequests(context.github, context.payload.org);
Also, we were not supposed to return early for GoogleCloudPlatform vs. googleapis. The return statement should have been outside of the if-blocks. Another mistake, good catch!
As for moving branch protection to merge-logic vs. keeping it where it is, my only concern would be that it's not really in our merge logic: we're not checking it every time to see if it's there in order to merge, we only check it once to see if it should be added to Datastore.
Perhaps a future ticket might be to create a github-utils file that contains functions that call to the API, and another one for functions that call to GCP? I'm not sure if this solution would address your concern.

sofisl · 2021-01-15T04:32:46Z

I've tested pre-merging, it is working!

sofisl added 2 commits January 8, 2021 23:14

fix(merge-on-green): add cron job to pick up hanging PRs

2524f8e

fix(merge-on-green): clean up testing

85bc0f6

sofisl requested a review from a team January 9, 2021 08:15

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 9, 2021

sofisl requested review from bcoe and chingor13 January 9, 2021 08:15

sofisl changed the title ~~Add cron hanging prs~~ fix(merge-on-green): second attempt to add cron to pick up hanging PRs Jan 9, 2021

Merge branch 'master' into addCronHangingPrs

6ee68d4

bcoe suggested changes Jan 12, 2021

View reviewed changes

Merge branch 'master' into addCronHangingPrs

4ed1a0f

sofisl requested a review from bcoe January 12, 2021 23:31

sofisl added 4 commits January 12, 2021 15:34

fix: testing structure

37195a9

Merge branch 'addCronHangingPrs' of github.com:googleapis/repo-automa…

085df20

…tion-bots into addCronHangingPrs

fix: npm run lint

01868df

fix: year

f537b19

bcoe reviewed Jan 13, 2021

View reviewed changes

fix: nits

f7ccb80

bcoe approved these changes Jan 14, 2021

View reviewed changes

Merge branch 'master' into addCronHangingPrs

e4dd43c

sofisl merged commit cdf0257 into master Jan 15, 2021

JustinBeckwith deleted the addCronHangingPrs branch August 9, 2021 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sofisl commented Jan 9, 2021 •

edited

bcoe left a comment

bcoe Jan 12, 2021

bcoe Jan 12, 2021

sofisl Jan 12, 2021

bcoe Jan 13, 2021

bcoe Jan 12, 2021

bcoe Jan 12, 2021

sofisl Jan 12, 2021

bcoe Jan 13, 2021

bcoe Jan 12, 2021

sofisl Jan 12, 2021

bcoe Jan 12, 2021

bcoe Jan 12, 2021

sofisl Jan 12, 2021

bcoe left a comment

bcoe Jan 13, 2021

bcoe Jan 13, 2021

bcoe Jan 13, 2021

sofisl commented Jan 13, 2021

sofisl commented Jan 15, 2021

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

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

Conversation

sofisl commented Jan 9, 2021 • edited

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

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

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

sofisl commented Jan 13, 2021

sofisl commented Jan 15, 2021

sofisl commented Jan 9, 2021 •

edited