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

Make it possible to only allow PRs against a select set of target branches #377

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -25,13 +25,13 @@
import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.*;
import org.openjdk.skara.vcs.Commit;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.Issue;

import java.io.*;
import java.util.*;
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.regex.*;
import java.util.stream.*;

class CheckRun {
@@ -43,8 +43,6 @@ class CheckRun {
private final List<Review> activeReviews;
private final Set<String> labels;
private final CensusInstance censusInstance;
private final Map<String, String> blockingLabels;
private final IssueProject issueProject;

private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
private final String progressMarker = "<!-- Anything below this marker will be automatically updated, please do not edit manually! -->";
@@ -54,7 +52,7 @@ class CheckRun {

private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List<Comment> comments,
List<Review> allReviews, List<Review> activeReviews, Set<String> labels,
CensusInstance censusInstance, Map<String, String> blockingLabels, IssueProject issueProject) {
CensusInstance censusInstance) {
this.workItem = workItem;
this.pr = pr;
this.prInstance = prInstance;
@@ -64,17 +62,29 @@ private CheckRun(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prI
this.labels = new HashSet<>(labels);
this.newLabels = new HashSet<>(labels);
this.censusInstance = censusInstance;
this.blockingLabels = blockingLabels;
this.issueProject = issueProject;
}

static void execute(CheckWorkItem workItem, PullRequest pr, PullRequestInstance prInstance, List<Comment> comments,
List<Review> allReviews, List<Review> activeReviews, Set<String> labels, CensusInstance censusInstance, Map<String, String> blockingLabels,
IssueProject issueProject) {
var run = new CheckRun(workItem, pr, prInstance, comments, allReviews, activeReviews, labels, censusInstance, blockingLabels, issueProject);
List<Review> allReviews, List<Review> activeReviews, Set<String> labels, CensusInstance censusInstance) {
var run = new CheckRun(workItem, pr, prInstance, comments, allReviews, activeReviews, labels, censusInstance);
run.checkStatus();
}

private boolean isTargetBranchAllowed() {
var matcher = workItem.bot.allowedTargetBranches().matcher(pr.targetRef());
edvbld marked this conversation as resolved.
Show resolved Hide resolved
return matcher.matches();
}

private List<String> allowedTargetBranches() {
var remoteBranches = prInstance.remoteBranches().stream()
.map(Reference::name)
.map(name -> workItem.bot.allowedTargetBranches().matcher(name))
.filter(Matcher::matches)
.map(Matcher::group)
.collect(Collectors.toList());
return remoteBranches;
}

// For unknown contributors, check that all commits have the same name and email
private boolean checkCommitAuthor(List<Commit> commits) throws IOException {
var author = censusInstance.namespace().get(pr.author().id());
@@ -114,6 +124,14 @@ private Optional<String> mergeSourceBranch() {
private List<String> botSpecificChecks() throws IOException {
var ret = new ArrayList<String>();

if (!isTargetBranchAllowed()) {
var error = "The branch `" + pr.targetRef() + "` is not allowed as target branch. The allowed target branches are:\n" +
allowedTargetBranches().stream()
.map(name -> " - " + name)
.collect(Collectors.joining("\n"));
ret.add(error);
}

var baseHash = prInstance.baseHash();
var headHash = pr.headHash();
var commits = prInstance.localRepo().commits(baseHash + ".." + headHash).asList();
@@ -160,7 +178,7 @@ private List<String> botSpecificChecks() throws IOException {
}
}

for (var blocker : blockingLabels.entrySet()) {
for (var blocker : workItem.bot.blockingLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
ret.add(blocker.getValue());
}
@@ -263,6 +281,7 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
progressBody.append(getChecksList(visitor));

var issue = Issue.fromString(pr.title());
var issueProject = workItem.bot.issueProject();
if (issueProject != null && issue.isPresent()) {
var allIssues = new ArrayList<Issue>();
allIssues.add(issue.get());
@@ -149,8 +149,7 @@ public void run(Path scratchPath) {

try {
var prInstance = new PullRequestInstance(scratchPath.resolve("pr"), pr, bot.ignoreStaleReviews());
CheckRun.execute(this, pr, prInstance, comments, allReviews, activeReviews, labels, census,
bot.blockingLabels(), bot.issueProject());
CheckRun.execute(this, pr, prInstance, comments, allReviews, activeReviews, labels, census);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
@@ -44,14 +44,16 @@ class PullRequestBot implements Bot {
private final Map<String, Pattern> readyComments;
private final IssueProject issueProject;
private final boolean ignoreStaleReviews;
private final Pattern allowedTargetBranches;
private final ConcurrentMap<Hash, Boolean> currentLabels;
private final PullRequestUpdateCache updateCache;
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");

PullRequestBot(HostedRepository repo, HostedRepository censusRepo, String censusRef,
Map<String, List<Pattern>> labelPatterns, Map<String, String> externalCommands,
Map<String, String> blockingLabels, Set<String> readyLabels,
Map<String, Pattern> readyComments, IssueProject issueProject, boolean ignoreStaleReviews) {
Map<String, Pattern> readyComments, IssueProject issueProject, boolean ignoreStaleReviews,
Pattern allowedTargetBranches) {
remoteRepo = repo;
this.censusRepo = censusRepo;
this.censusRef = censusRef;
@@ -62,6 +64,7 @@ class PullRequestBot implements Bot {
this.issueProject = issueProject;
this.readyComments = readyComments;
this.ignoreStaleReviews = ignoreStaleReviews;
this.allowedTargetBranches = allowedTargetBranches;

this.currentLabels = new ConcurrentHashMap<>();
this.updateCache = new PullRequestUpdateCache();
@@ -173,4 +176,8 @@ ConcurrentMap<Hash, Boolean> currentLabels() {
boolean ignoreStaleReviews() {
return ignoreStaleReviews;
}

Pattern allowedTargetBranches() {
return allowedTargetBranches;
}
}
@@ -39,6 +39,7 @@ public class PullRequestBotBuilder {
private Map<String, Pattern> readyComments = Map.of();
private IssueProject issueProject = null;
private boolean ignoreStaleReviews = false;
private Pattern allowedTargetBranches = Pattern.compile(".*");

PullRequestBotBuilder() {
}
@@ -93,7 +94,13 @@ public PullRequestBotBuilder ignoreStaleReviews(boolean ignoreStaleReviews) {
return this;
}

public PullRequestBotBuilder allowedTargetBranches(String allowedTargetBranches) {
this.allowedTargetBranches = Pattern.compile(allowedTargetBranches);
return this;
}

public PullRequestBot build() {
return new PullRequestBot(repo, censusRepo, censusRef, labelPatterns, externalCommands, blockingLabels, readyLabels, readyComments, issueProject, ignoreStaleReviews);
return new PullRequestBot(repo, censusRepo, censusRef, labelPatterns, externalCommands, blockingLabels,
readyLabels, readyComments, issueProject, ignoreStaleReviews, allowedTargetBranches);
}
}
@@ -66,33 +66,37 @@ public List<Bot> create(BotConfiguration configuration) {
var censusRepo = configuration.repository(repo.value().get("census").asString());
var censusRef = configuration.repositoryRef(repo.value().get("census").asString());

var labelPatterns = new HashMap<String, List<Pattern>>();
var botBuilder = PullRequestBot.newBuilder()
.repo(configuration.repository(repo.name()))
.censusRepo(censusRepo)
.censusRef(censusRef)
.blockingLabels(blockers)
.readyLabels(readyLabels)
.readyComments(readyComments)
.externalCommands(external);

if (repo.value().contains("labels")) {
var labelPatterns = new HashMap<String, List<Pattern>>();
for (var label : repo.value().get("labels").fields()) {
var patterns = label.value().stream()
.map(JSONValue::asString)
.map(Pattern::compile)
.collect(Collectors.toList());
labelPatterns.put(label.name(), patterns);
}
botBuilder.labelPatterns(labelPatterns);
}
if (repo.value().contains("issues")) {
botBuilder.issueProject(configuration.issueProject(repo.value().get("issues").asString()));
}
if (repo.value().contains("ignorestale")) {
botBuilder.ignoreStaleReviews(repo.value().get("ignorestale").asBoolean());
}
var issueProject = repo.value().contains("issues") ?
configuration.issueProject(repo.value().get("issues").asString()) :
null;
var ignoreStaleReviews = repo.value().contains("ignorestale") && repo.value().get("ignorestale").asBoolean();
var bot = PullRequestBot.newBuilder()
.repo(configuration.repository(repo.name()))
.censusRepo(censusRepo)
.censusRef(censusRef)
.labelPatterns(labelPatterns)
.externalCommands(external)
.blockingLabels(blockers)
.readyLabels(readyLabels)
.readyComments(readyComments)
.issueProject(issueProject)
.ignoreStaleReviews(ignoreStaleReviews)
.build();
ret.add(bot);
if (repo.value().contains("targetbranches")) {
botBuilder.allowedTargetBranches(repo.value().get("targetbranches").asString());
}

ret.add(botBuilder.build());
}

return ret;
@@ -226,4 +226,12 @@ void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestChe
}
}
}

List<Reference> remoteBranches() {
try {
return localRepo.remoteBranches(pr.repository().url().toString());
} catch (IOException e) {
return List.of();
}
}
}
@@ -1184,4 +1184,61 @@ void ignoreStale(TestInfo testInfo) throws IOException {
assertTrue(pr.labels().contains("rfr"));
}
}

@Test
void targetBranchPattern(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var author = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();

var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id());
var checkBot = PullRequestBot.newBuilder().repo(author).censusRepo(censusBuilder.build())
.allowedTargetBranches("^(?!master$).*")
.build();

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
localRepo.push(masterHash, author.url(), "master", true);
localRepo.push(masterHash, author.url(), "notmaster", 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",
"This is a pull request", true);

// Check the status
TestBotRunner.runPeriodicItems(checkBot);

// Verify that the check failed
var checks = pr.checks(editHash);
assertEquals(1, checks.size());
var check = checks.get("jcheck");
assertEquals(CheckStatus.FAILURE, check.status());
assertTrue(check.summary().orElseThrow().contains("The branch `master` is not allowed as target branch"));
assertTrue(check.summary().orElseThrow().contains("notmaster"));

var anotherPr = credentials.createPullRequest(author, "notmaster", "edit",
"This is a pull request", true);

// Check the status
TestBotRunner.runPeriodicItems(checkBot);

// Approve it as another user
var approvalPr = reviewer.pullRequest(anotherPr.id());
approvalPr.addReview(Review.Verdict.APPROVED, "Approved");
TestBotRunner.runPeriodicItems(checkBot);

// Verify that the check passed
checks = anotherPr.checks(editHash);
assertEquals(1, checks.size());
check = checks.get("jcheck");
assertEquals(CheckStatus.SUCCESS, check.status());
}
}

}