RFR: 1199: Enforce maintainer approval in JBS before allowing to integrate backports into updates projects [v2]
Guoxiong Li
gli at openjdk.org
Fri Sep 2 14:30:13 UTC 2022
On Wed, 31 Aug 2022 13:25:24 GMT, Erik Joelsson <erikj at openjdk.org> wrote:
>> Guoxiong Li has updated the pull request incrementally with five additional commits since the last revision:
>>
>> - Fix some comments and messages the bot replies.
>> - Refactor to remove the redundant expressions.
>> - Remove the redundant scheduling log info.
>> - Fix the decription in ApprovalCommand.
>> - Rename the methods and fields about 'update change' and fix related comments.
>
> bots/approval/src/main/java/org/openjdk/skara/bots/approval/AbstractApprovalBot.java line 48:
>
>> 46: }
>> 47:
>> 48: boolean isUpdateChange(PullRequest pr) {
>
> The name of this method confuses me. I think something like 'requiresApproval` would be better. The fact that this is currently (only) done for JDK update releases doesn't need to be reflected our method names.
>
> There are several other methods and fields that use the term `updateChange`. I think all of them should be renamed to something about approvals instead.
I renamed the related name and fixed the related comments. If any name is not good now, please help to point out.
> bots/approval/src/main/java/org/openjdk/skara/bots/approval/ApprovalIssueBot.java line 55:
>
>> 53: for (var issue : poller.getUpdatedIssues(IssueProject::issues)) {
>> 54: var issueWorkItem = new ApprovalIssueWorkItem(this, issue);
>> 55: log.fine("Scheduling: " + issueWorkItem);
>
> Since SKARA-1552, new WorkItems are logged in the BotRunner, making this log message redundant. This goes for all "scheduling" messages in getPeriodicItems, so I won't comment on each one.
Removed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 57:
>
>> 55: private void disapprovalReply(PullRequest pr, PrintWriter writer) {
>> 56: writer.println(String.format("@%s this pull request was rejected by the maintainer. "
>> 57: + "The bot will close this pull request automatically.", pr.author().username()));
>
> Suggestion:
>
> + "This pull request will be closed.", pr.author().username()));
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 65:
>
>> 63: CommandInvocation command, List<Comment> allComments, PrintWriter reply, PullRequestWorkItem workItem) {
>> 64: if (!workItem.isUpdateChange()) {
>> 65: reply.println("this repository or the target branch of this pull request have not been configured to use the `approval` command.");
>
> Suggestion:
>
> reply.println("the `approval` command can only be used on pull requests targeting branches and repositories that require approval.");
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 93:
>
>> 91: if (issue.labelNames().contains(workItem.approvalLabelName())) {
>> 92: // If the maintainers have approved the PR before,
>> 93: // the bot should remove the approval label at first.
>
> Suggestion:
>
> // the bot should remove the approval label first.
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 109:
>
>> 107: }
>> 108:
>> 109: pr.setState(Issue.State.OPEN);
>
> Why change the state of the PR?
The pull request would be automatically closed by the bot if the maintainer had rejected the pull request before. And at second thought, the maintainer approves the pull request, we need to open it to confirm the CheckWorkItem can be re-run in this pull request.
But now I find a bug about opening/closing the pull request automatically. When the maintainer has rejected the pull request, the pull request was closed by the bot. But if the author of the pull request re-open the pull request by using the `open` command (the author may want to change the pull request), the bot will re-close the pull request again because the disapproval label still exists. This bug seems not easy to solve now, so I suggest that the bot doesn't set the pull request state automatically. What is your opinion.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/ApprovalCommand.java line 121:
>
>> 119: if (issue.labelNames().contains(workItem.disapprovalLabelName())) {
>> 120: // If the maintainers have disapproved the PR before,
>> 121: // the bot should remove the disapproval label at first.
>
> Suggestion:
>
> // the bot should remove the disapproval label first.
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 204:
>
>> 202: }
>> 203: } else {
>> 204: // The PR doesn't contain a right issue.
>
> Suggestion:
>
> // The PR doesn't have an issue.
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 959:
>
>> 957: var existing = findComment(comments, updateChangeSuggestionMarker);
>> 958: if (existing.isPresent()) {
>> 959: // Only add the comment once per PR
>
> Maybe consider updating it if it's different from the existing one. If we were to change the message in the future, it would be nice if active PRs were updated.
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 982:
>
>> 980: after applying it. The risk of this backport is low because the change is little and the \
>> 981: issue fixed by this change also exists in other update repositories.
>> 982: ```
>
> This is too detailed. The process is officially documented here: https://openjdk.org/projects/jdk-updates/approval.html. We should just link to that and add the additional information about PR commands that can support the process. Maintaining a copy of the process description in the Skara source code is not something I want to do. Something like:
>
> This pull request requires [maintainer approval](https://openjdk.org/projects/jdk-updates/approval.html).
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 986:
>
>> 984: If you don't have permission to add a comment in JBS. Please use the command `request-approval` \
>> 985: to provide the related content, then the bot can help you add a comment by using the content \
>> 986: you provided. Below is an example for you:
>
> Suggestion:
>
> As a convenience, or if you don't have permission to post comments in JBS, \
> you may use the command `/request-approval` to supply the fix request comment \
> and automatically set the correct request label.
>
> usage: `/request-approval <comment-text>`
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 993:
>
>> 991: after applying it. The risk of this backport is low because the change is little and the \
>> 992: issue fixed by this change also exists in other update repositories.
>> 993: ```
>
> I don't think we should post an example here.
Removed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 999:
>
>> 997: And you don't need to add the fix request label manually to the issue like before, \
>> 998: now the bot can help you add the label automatically when this pull request is ready \
>> 999: for maintainers to approve.
>
> Suggestion:
>
> Please note that approval discussions should take place in the issue and not in the pull request.
Fixed.
> bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 1195:
>
>> 1193: .filter(entry -> !entry.getKey().equals(APPROVAL_PROGRESS))
>> 1194: .allMatch(Map.Entry::getValue) &&
>> 1195: integrationBlockers.isEmpty();
>
> Could we share the common parts of these logic expressions?
I place the result of the common expressions to a variable to reuse.
-------------
PR: https://git.openjdk.org/skara/pull/1364
More information about the skara-dev
mailing list