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

2234: /reviewers N should remove ready status for merge pull requests #1639

Closed
wants to merge 2 commits into from
Closed

2234: /reviewers N should remove ready status for merge pull requests #1639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1513bcc
SKARA-2234
zhaosongzs Apr 16, 2024
98c954b
update
zhaosongzs Apr 16, 2024
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
9 changes: 7 additions & 2 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ class CheckRun {
private final Set<String> newLabels;
private final boolean reviewCleanBackport;
private final Approval approval;
private final boolean reviewersCommandIssued;

private Duration expiresIn;
// Only set if approval is configured for the repo
Expand All @@ -103,6 +104,12 @@ private CheckRun(CheckWorkItem workItem, PullRequest pr, Repository localRepo, L
this.integrators = integrators;
this.reviewCleanBackport = reviewCleanBackport;
this.approval = approval;
this.reviewersCommandIssued = ReviewersTracker.additionalRequiredReviewers(pr.repository().forge().currentUser(), comments).isPresent();

// If reviewers command is issued, enable reviewers check for merge pull requests
if (reviewersCommandIssued) {
reviewMerge = MergePullRequestReviewConfiguration.ALWAYS;
}

baseHash = PullRequestUtils.baseHash(pr, localRepo);
checkablePullRequest = new CheckablePullRequest(pr, localRepo, ignoreStaleReviews,
Expand Down Expand Up @@ -1365,8 +1372,6 @@ private void checkStatus() {
integrationBlockers.addAll(mergeJCheckMessageWithTargetConf);
integrationBlockers.addAll(mergeJCheckMessageWithCommitConf);

var reviewersCommandIssued = ReviewersTracker.additionalRequiredReviewers(pr.repository().forge().currentUser(), comments).isPresent();

var reviewNeeded = !isCleanBackport || reviewCleanBackport || reviewersCommandIssued;

// Calculate and update the status message if needed
Expand Down
68 changes: 68 additions & 0 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/MergeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,74 @@ void branchMergeWithReviewMergeRequest(TestInfo testInfo) throws IOException {
}
}

@Test
void branchMergeWithReviewersCommand(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {

var author = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();
var censusBuilder = credentials.getCensusBuilder()
.addCommitter(author.forge().currentUser().id())
.addReviewer(integrator.forge().currentUser().id());
var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build();

// Populate the projects repository
var localRepoFolder = tempFolder.path().resolve("localrepo");
var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
assertFalse(CheckableRepository.hasBeenEdited(localRepo));
localRepo.push(masterHash, author.authenticatedUrl(), "master", true);

// Make more changes in another branch
var otherHash1 = CheckableRepository.appendAndCommit(localRepo, "First change in other_/-1.2",
"First other_/-1.2\n\nReviewed-by: integrationreviewer2");
localRepo.push(otherHash1, author.authenticatedUrl(), "other_/-1.2", true);
var otherHash2 = CheckableRepository.appendAndCommit(localRepo, "Second change in other_/-1.2",
"Second other_/-1.2\n\nReviewed-by: integrationreviewer2");
localRepo.push(otherHash2, author.authenticatedUrl(), "other_/-1.2");

// Go back to the original master
localRepo.checkout(masterHash, true);

// Make a change with a corresponding PR
var unrelated = Files.writeString(localRepo.root().resolve("unrelated.txt"), "Unrelated", StandardCharsets.UTF_8);
localRepo.add(unrelated);
var updatedMaster = localRepo.commit("Unrelated", "some", "some@one");
localRepo.merge(otherHash2);
localRepo.push(updatedMaster, author.authenticatedUrl(), "master");

var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.authenticatedUrl(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "Merge " + author.name() + ":other_/-1.2");

pr.addComment("/reviewers 1");

// Let the bot check the status
TestBotRunner.runPeriodicItems(mergeBot);
assertTrue(pr.store().labelNames().contains("clean"));
assertFalse(pr.store().labelNames().contains("ready"));
assertTrue(pr.store().body().contains("[ ] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer))"));

// Approve it
var reviewerPr = integrator.pullRequest(pr.id());
reviewerPr.addReview(Review.Verdict.APPROVED, "LGTM");
TestBotRunner.runPeriodicItems(mergeBot);
assertTrue(pr.store().labelNames().contains("ready"));
assertTrue(pr.store().body().contains("[x] Change must be properly reviewed (1 review required, with at least 1 [Reviewer](https://openjdk.org/bylaws#reviewer))"));

// Push it
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an ok message
var pushed = pr.comments().stream()
.filter(comment -> comment.body().contains("Pushed as commit"))
.count();
assertEquals(1, pushed);
}
}


@Test
void runJCheckTwiceInMergePR(TestInfo testInfo) throws IOException {
Expand Down