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();+-reviewer:username directives in your drafts.
collaboratorUsernames come from?
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?
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.
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?
Ok, makes sense. Just so my thinking is clear: it seems like there's two related but sort of different concepts at play here:
- 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.
- 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?
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.
docker run --entrypoint /bin/bash and confirmed that path does exist, not sure what i'm doing wrong
REVIEWABLE_DUMP_ENV can be useful for debugging this kind of issue.
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!
.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.