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.

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

pr: hint about two reviewers for some labels #844

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

@@ -545,6 +545,14 @@ private String getMergeReadyComment(String commitMessage, List<Review> reviews)
throw new UncheckedIOException(e);
}

if (labels.stream().anyMatch(label -> workItem.bot.twoReviewersLabels().contains(label))) {
message.append("\n\n");
message.append(":mag: One or more changes in this pull request modifies files in areas of ");
message.append("the source code that often require two reviewers. Please consider if this is ");
message.append("the case for this pull request, and if so, await a second reviewer to approve ");
message.append("this pull request before you integrate it.");
}

message.append("\n\n");
message.append("After integration, the commit message for the final commit will be:\n");
message.append("```\n");
@@ -44,6 +44,7 @@
private final Map<String, String> externalCommands;
private final Map<String, String> blockingCheckLabels;
private final Set<String> readyLabels;
private final Set<String> twoReviewersLabels;
private final Map<String, Pattern> readyComments;
private final IssueProject issueProject;
private final boolean ignoreStaleReviews;
@@ -61,17 +62,18 @@
PullRequestBot(HostedRepository repo, HostedRepository censusRepo, String censusRef,
LabelConfiguration labelConfiguration, Map<String, String> externalCommands,
Map<String, String> blockingCheckLabels, Set<String> readyLabels,
Map<String, Pattern> readyComments, IssueProject issueProject, boolean ignoreStaleReviews,
Set<String> allowedIssueTypes, Pattern allowedTargetBranches, Path seedStorage,
HostedRepository confOverrideRepo, String confOverrideName, String confOverrideRef,
String censusLink) {
Set<String> twoReviewersLabels, Map<String, Pattern> readyComments, IssueProject issueProject,
boolean ignoreStaleReviews, Set<String> allowedIssueTypes, Pattern allowedTargetBranches,
Path seedStorage, HostedRepository confOverrideRepo, String confOverrideName,
String confOverrideRef, String censusLink) {
remoteRepo = repo;
this.censusRepo = censusRepo;
this.censusRef = censusRef;
this.labelConfiguration = labelConfiguration;
this.externalCommands = externalCommands;
this.blockingCheckLabels = blockingCheckLabels;
this.readyLabels = readyLabels;
this.twoReviewersLabels = twoReviewersLabels;
this.issueProject = issueProject;
this.readyComments = readyComments;
this.ignoreStaleReviews = ignoreStaleReviews;
@@ -172,6 +174,10 @@ LabelConfiguration labelConfiguration() {
return blockingCheckLabels;
}

Set<String> twoReviewersLabels() {
return twoReviewersLabels;
}

IssueProject issueProject() {
return issueProject;
}
@@ -39,6 +39,7 @@
private Map<String, String> externalCommands = Map.of();
private Map<String, String> blockingCheckLabels = Map.of();
private Set<String> readyLabels = Set.of();
private Set<String> twoReviewersLabels = Set.of();
private Map<String, Pattern> readyComments = Map.of();
private IssueProject issueProject = null;
private boolean ignoreStaleReviews = false;
@@ -88,6 +89,11 @@ public PullRequestBotBuilder readyLabels(Set<String> readyLabels) {
return this;
}

public PullRequestBotBuilder twoReviewersLabels(Set<String> twoReviewersLabels) {
this.twoReviewersLabels = twoReviewersLabels;
return this;
}

public PullRequestBotBuilder readyComments(Map<String, Pattern> readyComments) {
this.readyComments = readyComments;
return this;
@@ -140,7 +146,7 @@ public PullRequestBotBuilder censusLink(String censusLink) {

public PullRequestBot build() {
return new PullRequestBot(repo, censusRepo, censusRef, labelConfiguration, externalCommands,
blockingCheckLabels, readyLabels, readyComments, issueProject,
blockingCheckLabels, readyLabels, twoReviewersLabels, readyComments, issueProject,
ignoreStaleReviews, allowedIssueTypes, allowedTargetBranches,
seedStorage, confOverrideRepo, confOverrideName, confOverrideRef,
censusLink);
@@ -97,6 +97,13 @@ public String name() {
}
botBuilder.labelConfiguration(labelConfigurations.get(labelGroup));
}
if (repo.value().contains("two-reviewers")) {
var labels = repo.value().get("two-reviewers")
.stream()
.map(label -> label.asString())
.collect(Collectors.toSet());
botBuilder.twoReviewersLabels(labels);
}
if (repo.value().contains("issues")) {
botBuilder.issueProject(configuration.issueProject(repo.value().get("issues").asString()));
}
@@ -356,4 +356,58 @@ void stripSuffix(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr,"The `group` label was successfully added.");
}
}

@Test
void twoReviewersLabels(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()
.addReviewer(integrator.forge().currentUser().id())
.addCommitter(author.forge().currentUser().id());
var labelConfiguration = LabelConfigurationJson.builder()
.addMatchers("1", List.of(Pattern.compile("cpp$")))
.addMatchers("2", List.of(Pattern.compile("hpp$")))
.addGroup("group", List.of("1", "2"))
.addExtra("extra")
.build();
var prBot = PullRequestBot.newBuilder()
.repo(integrator)
.censusRepo(censusBuilder.build())
.twoReviewersLabels(Set.of("1"))
.labelConfiguration(labelConfiguration)
.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.url(), "master", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "123: This is a pull request");

// Add a label with -dev suffix
pr.addComment("/label add 1");
TestBotRunner.runPeriodicItems(prBot);

// The bot should reply with a success message
assertLastCommentContains(pr,"The `1` label was successfully added.");

// Review the PR
var prAsReviewer = integrator.pullRequest(pr.id());
prAsReviewer.addReview(Review.Verdict.APPROVED, "Looks good!");
TestBotRunner.runPeriodicItems(prBot);

// The bot should reply with a integration message
assertLastCommentContains(pr,"This change now passes all *automated* pre-integration checks");
assertLastCommentContains(pr,":mag: One or more changes in this pull request modifies files");
assertLastCommentContains(pr,"in areas of the source code that often require two reviewers.");
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.