RFR: 1949: Handle maintainer approval from pull request
Erik Joelsson
erikj at openjdk.org
Wed Aug 16 14:13:18 UTC 2023
On Tue, 15 Aug 2023 22:35:04 GMT, Zhao Song <zsong at openjdk.org> wrote:
> As Erik said in the issue, "In SKARA-1199, we added the optional ability for a PR to block integration until a certain approval label appears on all the associated bugs. To further improve the workflow, we would like to add a couple of PR commands that will make it possible to perform the whole maintainer approval workflow through the PR"
>
> Therefore, in this patch, two pull request commands(`/approval` and `/approve`) are introduced.
>
> 1. Command `/approval`
> Usage: `/approval [<id>] (request|cancel) [<text>]`
> Examples: `/approval request my reason`, `/approval cancel`,
> `/approval JDK-123 request my reason`, `/approval 123 cancel`
>
> Only the author of the pull request is allowed to issue this command. If the user requested approval, the text will be posted to the bug. The user will be able to update the text by requesting the approval again. Once maintainer processed the request(approved or rejected), the user will not be able to request or cancel the approval.
>
> If there is only one issue associated with the pr, the user will not need to specify the issue id. If there are more than one issues associated with the pr, the user should specify the issue id in the command.
>
> 2. Command `/approve`
> Usage: `/approve [<id>] (yes|no)`
> Examples: `/approve yes`, `/approve no`, `/approve JDK-123 yes`, `/approve 123 no`
>
> Only maintainers of the repo are allowed to issue this command. Maintainers can approve or reject the approval whether the request exists.
>
> If there is only one issue associated with the pr, the user will not need to specify the issue id. If there are more than one issues associated with the pr, the user should specify the issue id in the command.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 60:
> 58: var targetRef = pr.targetRef();
> 59: if (approval == null || !approval.needsApproval(targetRef)) {
> 60: reply.println("No need to apply for maintainer approval!");
Bots aren't using `!` in their communication style. I also think we can make this even clearer.
If `approval == null`: "Changes in this repository do not require maintainer approval."
if `!approval.needsApproval(targetRef))`: "Changes to branch " + pr.targetRef() + " do not require maintainer approval."
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 81:
> 79:
> 80: if (issue.project().isPresent() && !issue.project().get().equalsIgnoreCase(issueProject.name())) {
> 81: reply.print("Can only request approval for issues in " + issueProject.name() + "!");
Suggestion:
reply.print("Approval can only be request for issues in the " + issueProject.name() + " project.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 87:
> 85: var issueTrackerIssueOpt = issueProject.issue(issue.shortId());
> 86: if (issueTrackerIssueOpt.isEmpty()) {
> 87: reply.print("Can not find " + issue.id() + " in " + issueProject.name() + ".");
Suggestion:
reply.print("Can not find " + issue.id() + " in the " + issueProject.name() + " project.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 98:
> 96: var existingComment = comments.stream()
> 97: .filter(comment -> comment.author().equals(issueProject.issueTracker().currentUser()))
> 98: .filter(comment -> comment.body().contains(prefix))
Maybe narrow the filter to `startsWith(prefix)`?
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 104:
> 102: if (option.equals("cancel")) {
> 103: if (labels.contains(approvedLabel) || labels.contains(rejectedLabel)) {
> 104: reply.print("The request has been processed by maintainer! Could not cancel the request now.");
Suggestion:
reply.print("The request has already been handled by a maintainer and can no longer be canceled.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 108:
> 106: issueTrackerIssue.removeLabel(requestLabel);
> 107: existingComment.ifPresent(issueTrackerIssue::removeComment);
> 108: reply.print("The request has already been cancelled successfully!");
Suggestion:
reply.print("The approval request has been cancelled successfully.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 112:
> 110: } else if (option.equals("request")) {
> 111: if (labels.contains(approvedLabel)) {
> 112: reply.print("The request has been approved by maintainer!");
Suggestion:
reply.print("Approval has already been requested and approved.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 114:
> 112: reply.print("The request has been approved by maintainer!");
> 113: } else if (labels.contains(rejectedLabel)) {
> 114: reply.print("The request has been rejected by maintainer!");
Suggestion:
reply.print("Approval has already been requested and rejected.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 116:
> 114: reply.print("The request has been rejected by maintainer!");
> 115: } else {
> 116: issueTrackerIssue.addLabel(requestLabel);
Adding the label should be done after the comment is added.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 117:
> 115: } else {
> 116: issueTrackerIssue.addLabel(requestLabel);
> 117: var messageToPost = prefix + ":Maintainer Approval Request from " + command.user().fullName() + "\n" + message.trim();
Suggestion:
var messageToPost = prefix + " Approval Request from " + command.user().fullName() + "\n" + message.trim();
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 121:
> 119: if (!existingComment.get().body().equals(messageToPost)) {
> 120: issueTrackerIssue.updateComment(existingComment.get().id(), messageToPost);
> 121: reply.print("The maintainer approval request has been updated successfully! Please wait for maintainers to process this request.");
Suggestion:
reply.print("The approval request has been updated successfully.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 122:
> 120: issueTrackerIssue.updateComment(existingComment.get().id(), messageToPost);
> 121: reply.print("The maintainer approval request has been updated successfully! Please wait for maintainers to process this request.");
> 122: } else{
Suggestion:
} else {
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 123:
> 121: reply.print("The maintainer approval request has been updated successfully! Please wait for maintainers to process this request.");
> 122: } else{
> 123: reply.print("The maintainer approval request is already up to date. Please wait for maintainers to process this request.");
Suggestion:
reply.print("The approval request was already up to date.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 127:
> 125: } else {
> 126: issueTrackerIssue.addComment(messageToPost);
> 127: reply.print("The maintainer approval request has been created successfully! Please wait for maintainers to process this request.");
Suggestion:
reply.print("The approval request has been created successfully.");
it would be neat to link to the created comment in each of these messages. We could move `commentUrl(Comment)` to `Issue`. Then the word `request` can be made into a link.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 149:
> 147: return Optional.empty();
> 148: } else {
> 149: reply.print("There are multiple issues associated with this pull request. Please specify an issue ID in your command.");
Suggestion:
reply.print("There are multiple issues associated with this pull request, you need to request approval for each one individually.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 155:
> 153: Issue issue = new Issue(issueId, null);
> 154: if (!issues.contains(issue.shortId())) {
> 155: reply.print("Can only request approval for issues that this pr solves.");
Suggestion:
reply.print("Approval can only be requested for issues that this pull request solves.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApproveCommand.java line 53:
> 51: public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, ScratchArea scratchArea, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
> 52: if (!bot.integrators().contains(command.user().username())) {
> 53: reply.println("Only integrators of this repo are allowed to issue the `/approve` command.");
Suggestion:
reply.println("Only integrators for this repository are allowed to issue the `/approve` command.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApproveCommand.java line 61:
> 59: reply.println("This target branch doesn't need maintainer approval.");
> 60: return;
> 61: }
Same comment as for ApprovalCommand.
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApproveCommand.java line 78:
> 76:
> 77: if (issue.project().isPresent() && !issue.project().get().equalsIgnoreCase(issueProject.name())) {
> 78: reply.print("Can only approve issues in " + issueProject.name() + "!");
Suggestion:
reply.print("Can only approve issues in the " + issueProject.name() + " project.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApproveCommand.java line 84:
> 82: var issueTrackerIssueOpt = issueProject.issue(issue.shortId());
> 83: if (issueTrackerIssueOpt.isEmpty()) {
> 84: reply.print("Can not find " + issue.id() + " in " + issueProject.name() + ".");
Suggestion:
reply.print("Can not find " + issue.id() + " in the " + issueProject.name() + " project.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApproveCommand.java line 94:
> 92: issueTrackerIssue.removeLabel(rejectedLabel);
> 93: issueTrackerIssue.addLabel(approvedLabel);
> 94: reply.print("The maintainer approval request has been approved!");
Suggestion:
reply.print("The approval request has been approved.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApproveCommand.java line 98:
> 96: issueTrackerIssue.removeLabel(approvedLabel);
> 97: issueTrackerIssue.addLabel(rejectedLabel);
> 98: reply.print("The maintainer approval request has been rejected!");
Suggestion:
reply.print("The approval request has been rejected.");
bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1527:
> 1525: String message = "⚠️ @" + pr.author().username() +
> 1526: " This change is now ready for you to apply for maintainer [approval](" + approval.documentLink() + ").\n" +
> 1527: "To learn how to apply for approval using the Skara command, please refer to this [link](" + approval.commandLink() + ")." +
No need for a configurable link for the command documentation. See the mergeReadyComment for appropriate style. I would suggest the following:
Suggestion:
" This change is now ready for you to apply for maintainer [approval](" + approval.documentLink() + "). " +
"This can be done directly in each associated issue or by using the " +
"[/approval](https://wiki.openjdk.org/display/SKARA/Pull+Request+Commands#PullRequestCommands-/approval) " +
"command" +
-------------
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295877549
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295926108
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295975508
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295952939
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295939507
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295942837
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295943968
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295944602
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295945966
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295950651
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295961493
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295970257
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295963233
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295963717
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295888716
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295892368
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295894223
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295971578
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295973999
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295975003
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295976486
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295976801
PR Review Comment: https://git.openjdk.org/skara/pull/1544#discussion_r1295860313
More information about the skara-dev
mailing list