Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 00:55
    jverce commented #26
  • May 06 11:34
    pkaminski closed #833
  • May 06 11:34
    pkaminski commented #833
  • May 06 11:34
    pkaminski labeled #833
  • May 06 11:13
    RaduBerinde commented #833
  • May 06 06:53
    pkaminski commented #833
  • May 06 06:53
    pkaminski labeled #833
  • May 06 06:50
    pkaminski closed #834
  • May 06 06:50
    pkaminski commented #834
  • May 06 06:50
    pkaminski labeled #834
  • May 06 06:48
    pkaminski commented #835
  • May 06 06:48
    pkaminski labeled #835
  • May 06 06:47
    pkaminski assigned #835
  • May 06 03:13
    jakebiesinger-storyhealth opened #835
  • May 05 21:05
    RaduBerinde commented #834
  • May 05 20:51
    jwnimmer-tri commented #834
  • May 05 20:38
    RaduBerinde opened #834
  • May 05 20:34
    RaduBerinde opened #833
  • May 05 17:59
    Cellule commented #832
  • May 05 17:18
    pkaminski closed #832
Michał Kowalczyk
@mkow
:)
Michał Kowalczyk
@mkow
@feldgendler you may be interested in this ^
Dan Halperin
@dhalperi

I just tried out simul-unsubscribing and -reviewer on batfish/batfish#6974

I can't remove myself from the reviewers list, oddly.

Screen Shot 2021-05-04 at 4.22.33 PM.png

It does generate its own "pending reviewers" list but this is not communicated to GitHub, and won't cause any GitHub notifications. (It would cause Reviewable notifications but you're probably not using those.)

@pkaminski Do you mean this, which definitely notifies me on github? - every action anyone takes on the review IIUC

Piotr Kaminski
@pkaminski
The "waiting on" list is indeed what I refer to as "pending reviewers". It's set by the completion condition (or our default version of it). However, by itself, it doesn't interact with GitHub and won't cause notifications (unless you also have webhook set, but I'm guessing not). That's why I was asking about syncRequestedReviewers and the corresponding toggle under Publish since that will cause Reviewable to sync pending reviewers with GitHub's requested reviewers, leading to notifications. So we should dig into that side, and also figure out why Reviewable keeps putting you on the pending reviewers list (which isn't affected by unsubscription, though it can be impacted by requested reviewers -- there may be a race condition of sorts there since there's a potential dependency cycle).
Dan Halperin
@dhalperi
Reviewable auto-comments with @me in its description, which github does notify me for. E.g., even if I've unsubscribed the issue, I will get re-subscribed and notified.
Piotr Kaminski
@pkaminski
OK, so the real problem is that you keep ending up in the "waiting on" list. This gets computed in the completion condition -- it looks like you have a custom one, but it's based on some very old example code and AFAICT only adds disableGitHubApprovals: true to the result. Does that sound right?
Dan Halperin
@dhalperi
Sounds right.
I think that perhaps if I -reviewers:me and then immediately unsubscribe from github issue, it might work. It's just a tetchy pattern to get right.
Piotr Kaminski
@pkaminski
You should be able to replace the whole condition with just return {disableGitHubApprovals: true}; and get the benefit of some recent bugfixes to the default condition.
Dan Halperin
@dhalperi
Confirming: reviewable upstream merges in the default one? Was not aware
Piotr Kaminski
@pkaminski
Yes, it does a merge of the default and the user's results, with priority going to the user. (Only for top-level keys.)
I'm also thinking I should instrument the default logic so it returns reasons why people end up on the list, and find a way to display those in Reviewable. That would help us figure out why you keep getting put back on -- it might well be a bug in the algorithm.
(And the merging wasn't always the case, I added it in at some point since copying the whole condition to make one tiny change was getting too crazy.)
Dan Halperin
@dhalperi
Am I doing something wrong? The result preview is not working with your suggestion
Screen Shot 2021-05-05 at 9.28.19 AM.png
Piotr Kaminski
@pkaminski
Nope, works for me.
Maybe just slow? I don't think there's any situation where the evaluation result would remain completely blank. It should either show a result or an error.
But then it should show a tiny spinner... Hmm...
Dan Halperin
@dhalperi
Ahh
I think the bug is that if you click the "finger down" button to apply PR, without changing the text or the PR #, then it clears the result but doesn't update.
Piotr Kaminski
@pkaminski
Huh, that sounds annoyingly plausible. Oops.
Dan Halperin
@dhalperi
I was changing condition, clicking finger, then scrolling down, since I couldn't see condition and result at the same time I didn't realize it was auto-updating.
Piotr Kaminski
@pkaminski
Assuming you're not blocking a discussion and haven't marked any files as reviewed, though, you're most likely getting picked as a candidate reviewer for some file that has no previous reviewers.
Dan Halperin
@dhalperi
I will religiously -reviewer:myself and GH unsubscribe simultaneously, and see if that solves the problem
Circle back :)
Piotr Kaminski
@pkaminski
It should work, in principle... Personally, I've turned off GitHub notifications altogether and instead rely on Reviewable's new webhook feature to pipe review status updates into a shared channel.
Dan Halperin
@dhalperi
Yeah, if I was on GHE and the only repos I contributed to were my company and they all used Reviewable, that might work.
Piotr Kaminski
@pkaminski
Ah, I should've said that I unwatched repositories we have Reviewable set up on, and still have GH comment notifications turned on.
But it's definitely not ideal and can miss non-review updates. I just got sufficiently annoyed with the GH spam that I decided this was the lesser evil.
Dan Halperin
@dhalperi
Yeah. GitHub isn't allowed to email me, but I use a Chrome extension that tells me if I have participating isuses/PRs to look at.
Piotr Kaminski
@pkaminski
Oh? What API/logic does it use?
Piotr Kaminski
@pkaminski
Ah, it just hooks into GitHub's web notifications. OK, that's essentially the same as email, but at least the spam is contained to their own page.
Dan Halperin
@dhalperi
The key thing for me is the participating button
That has so far been accurate, unless reviewable adds me back in because I did something wrong :)
Piotr Kaminski
@pkaminski
I guess getting @-mentioned counts as "participating"?
Dan Halperin
@dhalperi
Yes, it brings you back into participating
Piotr Kaminski
@pkaminski
Does it care about whether you're subscribed to the PR?
Dan Halperin
@dhalperi
If you unsubscribe, being @-mentioned resubscribes you
Piotr Kaminski
@pkaminski
So is participating === subscribed?
Dan Halperin
@dhalperi
I'm not sure about that
Piotr Kaminski
@pkaminski
If you were @-mentioned in a PR, which makes you a participant per above, how do you stop being a participant since you can't remove the mention? Is that what unsubscribing does?
1 reply
Dan Halperin
@dhalperi

One q: is there a way to change the "waiting on" definition? We typically use it as an OR, so at this stage:

all files reviewed, all discussions resolved (waiting on @arifogel)

it should not be waiting on anyone.

Piotr Kaminski
@pkaminski
Hmm, assuming that's the default logic's output it's a bit mysterious. Looking over the code I don't see a way for it to produce this result TBH. I really need to get the algorithm to save its reasons along with the output...
Ah, wait, is arifogel the author of that PR?
Dan Halperin
@dhalperi
No
Two people as reviewers on github, one of them reviewed it. arifogel is the other
Piotr Kaminski
@pkaminski
Can you share the state structure for that PR? (email or in private chat here)