Skip to content
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.

Sign up for GitHub

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

2083: Add option for omitting messages about /approval command #1580

Closed
wants to merge 2 commits into from
+43 −21
Closed

2083: Add option for omitting messages about /approval command #1580

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
565b17e
SKARA-2083
zhaosongzs Oct 31, 2023
85ee262
fix test
zhaosongzs Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion bots/pr/src/main/java/org/openjdk/skara/bots/pr/Approval.java
View file
Original file line number Diff line number Diff line change
@@ -33,14 +33,18 @@ public class Approval {
private final String rejected;
private final String documentLink;
private final Map<Pattern, String> branchPrefixes;
private final boolean approvalComment;
private final String approvalTerm;

public Approval(String prefix, String request, String approved, String rejected, String documentLink) {
public Approval(String prefix, String request, String approved, String rejected, String documentLink, boolean approvalComment, String approvalTerm) {
this.prefix = prefix;
this.request = request;
this.approved = approved;
this.rejected = rejected;
this.branchPrefixes = new HashMap<>();
this.documentLink = documentLink;
this.approvalComment = approvalComment;
this.approvalTerm = approvalTerm;
}

public void addBranchPrefix(Pattern branchPattern, String prefix) {
@@ -85,4 +89,12 @@ public boolean needsApproval(String targetRef) {
}
return false;
}

public boolean approvalComment() {
return approvalComment;
}

public String approvalTerm() {
return approvalTerm;
}
}
10 changes: 5 additions & 5 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
View file
Original file line number Diff line number Diff line change
@@ -279,9 +279,9 @@ private Map<String, Boolean> botSpecificProgresses(Map<Issue, Optional<IssueTrac
var issue = issueOpt.get();
var labelNames = issue.labelNames();
if (labelNames.contains(approval.approvedLabel(pr.targetRef()))) {
ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval", true);
ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs " + approval.approvalTerm(), true);
} else {
ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs maintainer approval", false);
ret.put("[" + issue.id() + "](" + issue.webUrl() + ") needs " + approval.approvalTerm(), false);
}
}
}
@@ -1335,9 +1335,9 @@ private void checkStatus() {
var readyForIntegration = readyToPostApprovalNeededComment &&
!additionalProgresses.containsValue(false);

if (approvalNeeded() && readyToPostApprovalNeededComment) {
if (approvalNeeded() && approval.approvalComment() && readyToPostApprovalNeededComment) {
for (var entry : additionalProgresses.entrySet()) {
if (!entry.getKey().endsWith("needs maintainer approval") && !entry.getValue()) {
if (!entry.getKey().endsWith("needs " + approval.approvalTerm()) && !entry.getValue()) {
readyToPostApprovalNeededComment = false;
break;
}
@@ -1534,7 +1534,7 @@ private void postApprovalNeededComment() {
return;
}
String message = "⚠️ @" + pr.author().username() +
" This change is now ready for you to apply for maintainer [approval](" + approval.documentLink() + "). " +
" This change is now ready for you to apply for [" + approval.approvalTerm() + "](" + 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." +
6 changes: 5 additions & 1 deletion bots/pr/src/main/java/org/openjdk/skara/bots/pr/PullRequestBotFactory.java
View file
Original file line number Diff line number Diff line change
@@ -231,7 +231,11 @@ public List<Bot> create(BotConfiguration configuration) {
String approved = approvalJSON.get("approved").asString();
String rejected = approvalJSON.get("rejected").asString();
String documentLink = approvalJSON.get("documentLink").asString();
Approval approval = new Approval(prefix, request, approved, rejected, documentLink);
// default value is true
boolean approvalComment = !approvalJSON.contains("approvalComment") || approvalJSON.get("approvalComment").asBoolean();
//default value is maintainer approval
String approvalTerm = approvalJSON.contains("approvalTerm") ? approvalJSON.get("approvalTerm").asString() : "maintainer approval";
Approval approval = new Approval(prefix, request, approved, rejected, documentLink, approvalComment, approvalTerm);
if (approvalJSON.contains("branches")) {
for (var branch : approvalJSON.get("branches").fields()) {
approval.addBranchPrefix(Pattern.compile(branch.name()), branch.value().get("prefix").asString());
6 changes: 3 additions & 3 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/ApprovalAndApproveCommandTests.java
View file
Original file line number Diff line number Diff line change
@@ -60,7 +60,7 @@ void simple(TestInfo testInfo) throws IOException {
.addCommitter(author.forge().currentUser().id());
Map<String, List<PRRecord>> issuePRMap = new HashMap<>();
Approval approval = new Approval("", "-critical-request", "-critical-approved",
"-critical-rejected", "https://example.com");
"-critical-rejected", "https://example.com", true, "maintainer approval");
approval.addBranchPrefix(Pattern.compile("jdk20.0.1"), "CPU23_04");
approval.addBranchPrefix(Pattern.compile("jdk20.0.2"), "CPU23_05");

@@ -132,7 +132,7 @@ void simple(TestInfo testInfo) throws IOException {
reviewerPr.addReview(Review.Verdict.APPROVED, "LGTM");
TestBotRunner.runPeriodicItems(prBot);
assertFalse(pr.store().labelNames().contains("ready"));
assertLastCommentContains(pr, " This change is now ready for you to apply for maintainer");
assertLastCommentContains(pr, " This change is now ready for you to apply for [maintainer approval]");

reviewerPr.addComment("/approve yes");
TestBotRunner.runPeriodicItems(prBot);
@@ -174,7 +174,7 @@ void multipleIssues(TestInfo testInfo) throws IOException {
.addCommitter(author.forge().currentUser().id());
Map<String, List<PRRecord>> issuePRMap = new HashMap<>();
Approval approval = new Approval("", "-critical-request", "-critical-approved",
"-critical-rejected", "https://example.com");
"-critical-rejected", "https://example.com", true, "maintainer approval");
approval.addBranchPrefix(Pattern.compile("jdk20.0.1"), "CPU23_04");
approval.addBranchPrefix(Pattern.compile("jdk20.0.2"), "CPU23_05");

12 changes: 9 additions & 3 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/ApprovalTests.java
View file
Original file line number Diff line number Diff line change
@@ -32,22 +32,26 @@ public class ApprovalTests {
@Test
void simple() {
Approval approval = new Approval("", "jdk17u-fix-request", "jdk17u-fix-yes",
"jdk17u-fix-no", "https://example.com");
"jdk17u-fix-no", "https://example.com", true, "maintainer approval");
assertEquals("jdk17u-fix-request", approval.requestedLabel("master"));
assertEquals("jdk17u-fix-yes", approval.approvedLabel("master"));
assertEquals("jdk17u-fix-no", approval.rejectedLabel("master"));
assertEquals("https://example.com", approval.documentLink());
assertTrue(approval.approvalComment());
assertEquals("maintainer approval", approval.approvalTerm());
assertTrue(approval.needsApproval("master"));

approval = new Approval("jdk17u-fix-", "request", "yes", "no",
"https://example.com");
"https://example.com", false, "maintainer approval");
assertEquals("jdk17u-fix-request", approval.requestedLabel("master"));
assertEquals("jdk17u-fix-yes", approval.approvedLabel("master"));
assertEquals("jdk17u-fix-no", approval.rejectedLabel("master"));
assertFalse(approval.approvalComment());
assertEquals("maintainer approval", approval.approvalTerm());
assertTrue(approval.needsApproval("master"));

approval = new Approval("", "-critical-request", "-critical-approved",
"-critical-rejected", "https://example.com");
"-critical-rejected", "https://example.com", false, "critical request");
approval.addBranchPrefix(Pattern.compile("jdk20.0.1"), "CPU23_04");
approval.addBranchPrefix(Pattern.compile("jdk20.0.2"), "CPU23_05");
assertEquals("CPU23_04-critical-request", approval.requestedLabel("jdk20.0.1"));
@@ -60,5 +64,7 @@ void simple() {
assertTrue(approval.needsApproval("jdk20.0.1"));
assertTrue(approval.needsApproval("jdk20.0.2"));
assertFalse(approval.needsApproval("jdk20.0.3"));
assertFalse(approval.approvalComment());
assertEquals("critical request", approval.approvalTerm());
}
}
2 changes: 1 addition & 1 deletion bots/pr/src/test/java/org/openjdk/skara/bots/pr/CheckTests.java
View file
Original file line number Diff line number Diff line change
@@ -3078,7 +3078,7 @@ void maintainerApprovalWithDependentPR(TestInfo testInfo) throws IOException {
.addCommitter(author.forge().currentUser().id());
Map<String, List<PRRecord>> issuePRMap = new HashMap<>();
Approval approval = new Approval("", "-critical-request", "-critical-approved",
"-critical-rejected", "https://example.com");
"-critical-rejected", "https://example.com", true, "maintainer approval");
approval.addBranchPrefix(Pattern.compile("jdk20.0.1"), "CPU23_04");
approval.addBranchPrefix(Pattern.compile("jdk20.0.2"), "CPU23_05");

14 changes: 7 additions & 7 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/IssueBotTests.java
View file
Original file line number Diff line number Diff line change
@@ -318,7 +318,7 @@ void maintainerApproval(TestInfo testInfo) throws IOException {
.censusRepo(censusBuilder.build())
.issuePRMap(issuePRMap)
.approval(new Approval("", "jdk17u-fix-request", "jdk17u-fix-yes",
"jdk17u-fix-no", "https://example.com"))
"jdk17u-fix-no", "https://example.com", true, "maintainer approval"))
.build();
var issueBot = new IssueBot(issueProject, List.of(author), Map.of(bot.name(), prBot), issuePRMap);

@@ -365,7 +365,7 @@ void maintainerApprovalWithBranchPattern(TestInfo testInfo) throws IOException {
.addCommitter(author.forge().currentUser().id());
Map<String, List<PRRecord>> issuePRMap = new HashMap<>();
Approval approval = new Approval("", "-critical-request", "-critical-approved",
"-critical-rejected", "https://example.com");
"-critical-rejected", "https://example.com", true, "critical request");
approval.addBranchPrefix(Pattern.compile("jdk20.0.1"), "CPU23_04");
approval.addBranchPrefix(Pattern.compile("jdk20.0.2"), "CPU23_05");

@@ -396,7 +396,7 @@ void maintainerApprovalWithBranchPattern(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(prBot);
assertFalse(pr.store().labelNames().contains("ready"));
assertTrue(pr.store().labelNames().contains("rfr"));
assertFalse(pr.store().body().contains("[TEST-1](http://localhost/project/testTEST-1) needs maintainer approval"));
assertFalse(pr.store().body().contains("[TEST-1](http://localhost/project/testTEST-1) needs critical request"));

var reviewerPr = reviewer.pullRequest(pr.id());
reviewerPr.addReview(Review.Verdict.APPROVED, "Looks good");
@@ -407,8 +407,8 @@ void maintainerApprovalWithBranchPattern(TestInfo testInfo) throws IOException {
reviewerPr.addReview(Review.Verdict.APPROVED, "Looks good");
TestBotRunner.runPeriodicItems(prBot);
assertFalse(pr.store().labelNames().contains("ready"));
assertTrue(pr.store().body().contains("[TEST-1](http://localhost/project/testTEST-1) needs maintainer approval"));
assertEquals("⚠️ @user1 This change is now ready for you to apply for maintainer [approval](https://example.com). " +
assertTrue(pr.store().body().contains("[TEST-1](http://localhost/project/testTEST-1) needs critical request"));
assertEquals("⚠️ @user1 This change is now ready for you to apply for [critical request](https://example.com). " +
"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." +
"<!-- PullRequestBot approval needed comment -->"
@@ -421,8 +421,8 @@ void maintainerApprovalWithBranchPattern(TestInfo testInfo) throws IOException {
issue.addLabel("CPU23_04-critical-approved");
TestBotRunner.runPeriodicItems(issueBot);
assertTrue(pr.store().body().contains("Approved"));
assertTrue(pr.store().body().contains("[TEST-1](http://localhost/project/testTEST-1) needs maintainer approval"));
assertEquals("⚠️ @user1 This change is now ready for you to apply for maintainer [approval](https://example.com). " +
assertTrue(pr.store().body().contains("[TEST-1](http://localhost/project/testTEST-1) needs critical request"));
assertEquals("⚠️ @user1 This change is now ready for you to apply for [critical request](https://example.com). " +
"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." +
"<!-- PullRequestBot approval needed comment -->"