New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
401: Allow a PR author to manually add reviewers #630
Conversation
|
Webrevs
|
I presume that the added reviewers don't count towards satisfying the requirement of number of reviewers? |
They do count, unless the project is configured to require up-to-date reviews of the latest commit (like jfx for example). |
So it will count the same as a stale review? That seems fine. |
Yeah exactly, when a project is configured to ignore stale reviews it will also ignore any manually added ones. |
return; | ||
} | ||
if (bot.ignoreStaleReviews()) { | ||
reply.println("This project requires authenticated reviews - please ask your reviewer to flag this PR as reviewed."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should contain an @<pr-author>
as well? (both to notify and make it clear who is being addressed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but all command replies are automatically prepended with @comment-author
- and since the check above this one ensures that the comment author is the pr-author the message should only happen as a reply to the author of the pr.
var authenticatedReviewers = pr.reviews().stream() | ||
.map(review -> namespace.get(review.reviewer().id())) | ||
.map(Contributor::username) | ||
.filter(Objects::nonNull) | ||
.collect(Collectors.toSet()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this stream pipeline duplicated a few times already (in CheckablePullRequest and CheckRun). Maybe you want to move this to a helper method somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll move it to somewhere common.
@rwestberg This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type
Since the source branch of this PR was last updated there has been 1 commit pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge
|
/integrate |
@rwestberg The following commits have been pushed to master since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit f6b8bd8. |
Hi all,
Please review this change that adds a
/reviewer
command that allows the PR author to manually add reviewers. The implementation is fairly similar to the/contributor
command.Best regards,
Robin
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/skara pull/630/head:pull/630
$ git checkout pull/630