Reviewable/Reviewable

Support for Reviewable

Raman Gupta

@rocketraman

Lol

Dan Halperin

@dhalperi

Screen Shot 2020-03-23 at 1.10.06 PM.png

Got this error when merging a PR from a 3rd-party repository.

I have squash&delete as default.
3rd party may or may not have github branch deletion turned on
I probably don't have access to delete their branch.

Piotr Kaminski

@pkaminski

Have you confirmed the branch actually exists?

Ah, I see.

Dan Halperin

@dhalperi

My guess is this is another corner case/race condition we want to whitelist.

Piotr Kaminski

@pkaminski

Reviewable tries to check for deletion permissions, but I don't have any code to handle auto branch deletion yet. That's likely the problem -- probably worth opening an issue.

Dan Halperin

@dhalperi

Can do

Not seeing it in the activity log, but Reviewable/Reviewable#725

Alexey Feldgendler

@feldgendler

@pkaminski Hi! We made a change in the custom function that should affect existing PRs. Now we see a few PRs in an inconsistent state.

Is there a limit on how many custom function runs Reviewable does for one repository? How does it work?

Alexey Feldgendler

@feldgendler

…looks like it's working through our old PRs. Hopefully it will eventually complete.

Piotr Kaminski

@pkaminski

@feldgendler There's no limit per se, and all open PRs get put on a queue to recompute, but queue throughput is affected by GitHub API quota limits.

Alexey Feldgendler

@feldgendler

Suggestion: in such a case, could you skip closed PRs?

Piotr Kaminski

@pkaminski

It only does open PRs AFAIR, but now that I think of it it does them in numerical order, whereas reverse would probably be better.

Alexey Feldgendler

@feldgendler

We have thousands of closed PRs, and Reviewable seems to be running the custom function on every one of them, causing useless notifications about review requests as it applies the “reviewer sync”.

I received (and still still receiving) a flood of emails “xx requested your review on some long-closed PR”.

Piotr Kaminski

@pkaminski

Oops, you're right, it does closed PRs too. Obviously that decision was made long before the reviewer sync feature got put in...

Actually, I take that back. It should only be doing open ones, unless the GH API docs are wrong.

function *reconcileReviews(gh, db) {
  let count = 0;
  const pullRequests = yield gh.request('GET /repos/:owner/:repo/pulls');
  yield _.map(pullRequests, function*(pullRequest) {
    yield db.child(
      'queues/reconciliation/:owner|:repo|:prNumber', {prNumber: pullRequest.number}
    ).update({
      owner: db.$scope.owner, repo: db.$scope.repo, prNumber: pullRequest.number,
      updateCompletion: true, timestamp: NodeFire.SERVER_TIMESTAMP
    });
    count++;
  });
  return count;
}

And the GET is supposed to default to state=open.

Alexey Feldgendler

@feldgendler

I'm pretty sure it's done hundreds of closed PRs.

Piotr Kaminski

@pkaminski

Can you point me to one of them?

Alexey Feldgendler

@feldgendler

ridge/tectonic#1077

They're all in a private repo.

Piotr Kaminski

@pkaminski

We're missing something. You triggered the reconciliation around 10am, and I confirmed that this only touched open PRs. Action on 1077 only started around 10:50am and it was in response to a PR sync, not a reconciliation. It's not clear to me what triggered a PR sync on a closed (and archived!) review -- is there anything that could explain it from your side?

Alexey Feldgendler

@feldgendler

10am in what time zone?

Piotr Kaminski

@pkaminski

PDT

Alexey Feldgendler

@feldgendler

Eh… how many hours ago is that?

Piotr Kaminski

@pkaminski

1:20

Alexey Feldgendler

@feldgendler

Yes, sounds about right.

Piotr Kaminski

@pkaminski

When did you get the reviewer notification for that PR?

Alexey Feldgendler

@feldgendler

19 min ago

Piotr Kaminski

@pkaminski

Yup, that lines up perfectly with when the reconciliation actually completed for 1077 (it was delayed for ~13 minutes due to lack of quota).

Alexey Feldgendler

@feldgendler

I have a guess. One of our very active reviewers wanted to change what GitHub account he uses, so I removed one of his accounts from our organization and added a different one.

Piotr Kaminski

@pkaminski

Yep, that would fire PR events for everything he was involved with.

And force a resync on all those PRs... which would in turn trigger reconciliation.

Alexey Feldgendler

@feldgendler

Alright… now could you perhaps skip reconcilation for closed PRs?

Piotr Kaminski

@pkaminski

I can't skip the sync (since the PR might be getting reopened) but I might be able to skip reconciliation. Let me think a bit about what unexpected consequences that might have.

Alexey Feldgendler

@feldgendler

If you skip reconciliation, that will at least cut the flood of emails.

Piotr Kaminski

@pkaminski

Understood, I just need to think what other undesirable effects it might have. If nothing else, though, I think it would be safe to skip requested reviewer sync for closed PRs.

Alexey Feldgendler

@feldgendler

:thumbsup:

Piotr Kaminski

@pkaminski

After thinking it through, I decided that triggering reconciliation from background PR sync is wrong for closed PRs and disabled it. Reconciliation will still trigger if a closed review is loaded in the browser, or a new GH review or comment posted to a closed PR.

Thanks for reporting the issue!

Meenal Acharya

@Microsoft4552

Good Morning Everyone!

Piotr Kaminski

@pkaminski

Hey, what's up?

Meenal Acharya

@Microsoft4552

I'm good, how are you?

What's going on here?

Piotr Kaminski

@pkaminski

Not much, it's getting pretty late here.

Meenal Acharya

@Microsoft4552

Ohh, then have a good night! :-)

Where communities thrive

People

Repo info

Activity