Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • Mar 30 21:12
    dozoisch commented #666
  • Mar 29 22:09
    pkaminski commented #496
  • Mar 29 21:57
    pkaminski closed #608
  • Mar 29 21:57
    pkaminski commented #608
  • Mar 29 21:54
    pkaminski commented #645
  • Mar 29 21:52
    pkaminski labeled #645
  • Mar 29 21:48
    pkaminski labeled #692
  • Mar 29 21:46
    pkaminski closed #702
  • Mar 29 21:46
    pkaminski labeled #702
  • Mar 29 06:00
    dozoisch commented #666
  • Mar 29 05:53
    pkaminski closed #666
  • Mar 29 05:53
    pkaminski commented #666
  • Mar 29 05:44
    Server build 2306 deployed
  • Mar 29 05:39
    Client build 3994 deployed
  • Mar 29 05:35
    aaronfi commented #666
  • Mar 27 22:40
    Server build 2296 deployed
  • Mar 27 04:48
    Client build 3980 deployed
  • Mar 23 20:20
    pkaminski labeled #725
  • Mar 23 20:17
    dhalperi edited #725
  • Mar 23 20:17
    dhalperi opened #725
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
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! :-)