Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
  • 11:57
    pkaminski commented #668
  • 11:56
    pkaminski labeled #668
  • 11:55
    pkaminski closed #669
  • 11:55
    pkaminski commented #669
  • 11:55
    pkaminski labeled #669
  • 08:58
    misha-tectonic opened #669
  • 08:58
    misha-tectonic opened #668
  • Aug 21 20:24
    rictic commented #498
  • Aug 15 07:01
    Server build 2156 deployed
  • Aug 14 13:23
    Server build 2154 deployed
  • Aug 14 09:43
    Server build 2152 deployed
  • Aug 14 09:17
    pkaminski commented #667
  • Aug 14 09:16
    pkaminski assigned #667
  • Aug 13 09:21
    misha-tectonic edited #667
  • Aug 13 09:21
    misha-tectonic opened #667
  • Aug 09 15:20
    DamienCassou commented #457
  • Aug 01 14:54
    seansfkelley commented #665
  • Aug 01 14:45
    seansfkelley commented #665
  • Aug 01 04:21
    pkaminski commented #665
  • Aug 01 02:03
    seansfkelley commented #665
Piotr Kaminski
@pkaminski
Yes, everybody in pendingReviewers should be requested by default. You're probably running into another edge condition. Here's the exact code to determine who gets requested:
        const newPendingReviewerUsernames = _.pluck(completionResult.pendingReviewers, 'username');
        const authorUsername = avatars.users[authorKey].username;
        const requestedReviewerUsernames = _(~this.state.review.people)
          .filter('requestedReviewer')
          .pluck('username')
          .value();
        if (willPostReview) {
          // User posting the review will automatically be unrequested by GitHub
          _.pull(requestedReviewerUsernames, this.$store.users.current.core.public.username);
        }
        const usernamesToAdd = _(newPendingReviewerUsernames)
          .difference(requestedReviewerUsernames)
          .difference(previousDirectiveUsernames)
          .intersection(collaboratorUsernames)
          .pull(authorUsername)  // Can never request review from PR author
          .sortBy()
          .value();
Alec Benzer
@AlecBenzer
Hm, what's previousDirectiveUsernames here?
Piotr Kaminski
@pkaminski
Assuming the other user isn't the PR's author, the most likely edge you're hitting is the intersection with collaboratorUsernames.
Those are any explicit +-reviewer:username directives in your drafts.
Basically, if you explicitly added or removed a reviewer, the algorithm goes hands-off on them.
Alec Benzer
@AlecBenzer
Ah, ok, no, shouldn't have been any of those...
And where does collaboratorUsernames come from?
Piotr Kaminski
@pkaminski
That comes from GET /repos/:owner/:repo/collaborators on GitHub.
Alec Benzer
@AlecBenzer
reviewer in question shows up in that list too
and isn't the author
Piotr Kaminski
@pkaminski
Can you check if Reviewable for some reason thinks the user is/was already requested? They'll show up with a "requested" bubble in the people list under the main discussion.
Alec Benzer
@AlecBenzer
Oh, well they are now b/c I requested manually (and it shows up in reviewable), I didn't notice what it was before that though.
I'll try to check next time this comes up
Just to confirm: it definitely doesn't matter that they were already in pendingReviewers prior to publishing, right? If they're still in pendingReviewers after publishing, and not requested on GitHub, then they should be requested on GitHub. Right?
Piotr Kaminski
@pkaminski
Yeah, it should be synced when you load the review, but who knows. I'm not seeing any other reason at the moment...
And yes, everybody in pendingReviewers should be requested, modulo the exclusions you saw above. This is asymmetric with request deletions, which only happen if the user was in the old but not the new list.
Piotr Kaminski
@pkaminski
If you see this happening again and I happen to be around, we can jump in and poke the model directly to figure out why the person disappears (before you publish, of course).
Alec Benzer
@AlecBenzer
Yeah, I'll let you know. This does all seem consistent with reviewable simply think the reviewer is already requested, even if they're not... checking for the bubble next time this happens should confirm.
Alec Benzer
@AlecBenzer
Just happened again in the same PR, reviewable doesn't show the "review requested" bubble next to the reviewer in question. So I think that hypothesis is out....
Piotr Kaminski
@pkaminski
Do you still have it in this state? We can poke around.
Alec Benzer
@AlecBenzer
I do
Piotr Kaminski
@pkaminski
OK, please reload the page with ?debug=model.
We can go into private chat if you'd like, in case details of your PR come out during the investigation.
Alec Benzer
@AlecBenzer
sure
Remi Torracinta
@RemiTorracinta
Hi, we had an engineer who wanted to do a full pass of a PR which had already been reviewed by someone else. But the usual "mark file as reviewed" button/flow, which auto-scrolls to the next unreviewed file, was not available. Are passes by multiple people not a typical use case for Reviewable?
Piotr Kaminski
@pkaminski
Hey @RemiTorracinta, by default Reviewable assumes that one reviewer per file-revision is sufficient. There are many ways to customize this -- please see Reviewable/Reviewable#560 and other issues it links to for relevant discussions.
Remi Torracinta
@RemiTorracinta
Ok, we're going to take a look at updating the "file reviewed" condition for our org, thanks
Alec Benzer
@AlecBenzer

Just to be sure: Here you say:

If reviewed == false then the mark reviewed button will be red even if other people have reviewed the revision.

I assume that if the current reviewer has reviewed the given file at the given rev (but other reviewers haven't), the button will not be red (even though the file's overall state is still "not reviewed"), correct?

Piotr Kaminski
@pkaminski
Correct. reviewed == false only overrides the "grey" button states.
Alec Benzer
@AlecBenzer

Ok, makes sense. Just so my thinking is clear: it seems like there's two related but sort of different concepts at play here:

  1. Whether or not it's "easy" for a person new to a PR to quickly go through it and see all the deltas they haven't yet seen.
  2. Whether we want to force all requested reviewers to review each file independently.

What we really care about is 1, and we're using 2 as a proxy to get there. I think 2 is fine for us to do (especially since we don't require reviewable to be green to submit, so reviewable "blocking" is only a soft-block), but I'm curious if there's a more "direct" way to get what we want?

I guess, an here argument might be: if you don't actually want someone to review files before merging, why do you care if it's easy for them to see the deltas? Which seems consistent with the ethos of reviewable. (Show people exactly what they need to see for the review to progress and nothing more). So maybe it's reasonable that 1 & 2 are tied, and it would just complicate things further to try and get 1 a different way?

Piotr Kaminski
@pkaminski
Yeah, I think that's a good assessment. Also, I see 1 as a sort of special case, and showing that diff is just one click away anyway (drag from base to latest revision in file matrix header) so I don't think we need to special-case it any further.
alex-xnor
@alex-xnor
We have a PR that's doing a big refactor, renaming a directory that holds a good chunk of our entire code base. It touches about 2000 files and GitHub measures about 100,000 additions and 100,000 deletions (most of these are just moves though). We've done most of the review outside of Reviewable, but Reviewable is set as blocking for merge for us; is there a good way to mark all files as reviewed, just to satisfy GitHub? The problem is that Reviewable just hangs when we open it to that PR, even after waiting around 20 minutes. We're looking into some alternative ways of getting this merged but if there's some trick we can use to just mark everything as reviewed so we don't have to do anything otherwise special, it would be good to know it.
Piotr Kaminski
@pkaminski
Possibly the easiest thing is to temporarily modify your custom review completion condition to simply set the completed flag to true for that specific review (based on matching review.pullRequest.target and review.pullRequest.number).
alex-xnor
@alex-xnor
Thanks, that works.
prseshad
@prseshad
Hi @pkaminski, first of all, love your product and been using it for a while with my team. Quick question - I noticed that for a while now the "Merge and Delete Branch" button takes a while to appear after a PR is approved - like maybe 10-20s. It used to be much more "instant". Is this a known issue, and is there a fix planned?
Piotr Kaminski
@pkaminski
Thanks @prseshad! I'm not aware of any systemic issues with the merge button appearing after only after a long delay, and haven't heard from other users about this yet (not even @dhalperi, who seems to be a magnet for these problems). The last client push was on July 17th -- did the issue start around that time, or earlier/later? Also, can you double-check that the repo is connected, and whether it has a custom review completion condition? Thanks.
thomasatnuro
@thomasatnuro
Hi, I'm attempting to install a trial of reviewable on my local workstation but hitting to some issues running the docker image: Error: Unable to authenticate the Firebase Admin SDK. The path to a Firebase service account key JSON file specified via the REVIEWABLE_FIREBASE_CREDENTIALS_FILE environment variable ("/app/firebase-creds.json") does not exist.
I've docker run --entrypoint /bin/bash and confirmed that path does exist, not sure what i'm doing wrong
^^ @pkaminski maybe you can help me out :D
thomasatnuro
@thomasatnuro
Ah, disregard. I had double quotes around he values in the env file
Piotr Kaminski
@pkaminski
:) Sorry, was busy for a bit, glad to see you got it in the meantime!
Setting REVIEWABLE_DUMP_ENV can be useful for debugging this kind of issue.
Elichai Turkel
@elichai
Trying to sign in just gives me 200 OK as a plain text
Piotr Kaminski
@pkaminski
Hey @elichai, sorry about that. This kind of issue can often be caused by overly aggressive filtering plugins. Could you please try again with plugins disabled and no unusual browser settings? If this doesn't work, please let me know which browser you're using. Thanks.
Alec Benzer
@AlecBenzer
minor-ish annoyance, but: it seems you can't enable reviewable for all current and future repos if any of the repos in your org are archived? seems hitting the button results in an error and doesn't seem to take effect for the "future" part?
Piotr Kaminski
@pkaminski
@AlecBenzer That sounds like a bug -- could you please open an issue?
Marc Zych
@marczych

I'm having a hard time searching for this so I'll just ask here: Does Reviewable support Git LFS?

I'm not super familiar with Git LFS but presumably the files tracked by LFS would show up as some arbitrary IDs rather than the underlying content in Reviewable? Or perhaps they would get collapsed as auto generated via .gitattributes? Any info/thoughts would be appreciated!

Piotr Kaminski
@pkaminski
@marczych I haven't tried but there's a fair chance it will work. It all depends on whether GitHub's API passes through the raw LFS index file that replaces the large file in git, or whether it automatically resolves it and returns its contents. If you happen to have LFS set up perhaps you can give it a try? If you see diffs of files that look like the spec at https://github.com/git-lfs/git-lfs/blob/master/docs/spec.md then it's working. If your browser blows up with an out of memory message then it probably tried to pull in the full file. :)
You should also be able to use .gitattributes to prevent the files from being diffed. These get processed before Reviewable attempts to fetch the file contents so should be effective even for large files.
Marc Zych
@marczych
I played around with it a bit more last night and clients that don't support LFS just see the pointer files so I'd expect Reviewable to just display those files in the diff which is perfect. I'll try it in Reviewable soon and let you know if it blows up. Thanks!