Skip to content
Permalink
Browse files
prbot: Allow overriding jcheck configuration
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Aug 31, 2020
1 parent b028725 commit e36b503e7a3e078021d20cd0ad1f0370cbce1ca6
@@ -76,12 +76,13 @@ private static Namespace namespace(Census census, String hostNamespace) {
return namespace;
}

private static JCheckConfiguration configuration(HostedRepository remoteRepo, String ref) {
var confFile = remoteRepo.fileContents(".jcheck/conf", ref);
private static JCheckConfiguration configuration(HostedRepository remoteRepo, String name, String ref) {
var confFile = remoteRepo.fileContents(name, ref);
return JCheckConfiguration.parse(confFile.lines().collect(Collectors.toList()));
}

static CensusInstance create(HostedRepository censusRepo, String censusRef, Path folder, PullRequest pr) {
static CensusInstance create(HostedRepository censusRepo, String censusRef, Path folder, PullRequest pr,
HostedRepository confOverrideRepo, String confOverrideName, String confOverrideRef) {
var repoName = censusRepo.url().getHost() + "/" + censusRepo.name();
var repoFolder = folder.resolve(URLEncoder.encode(repoName, StandardCharsets.UTF_8));
try {
@@ -95,7 +96,12 @@ static CensusInstance create(HostedRepository censusRepo, String censusRef, Path
}

try {
var configuration = configuration(pr.repository(), pr.targetRef());
JCheckConfiguration configuration;
if (confOverrideRepo == null) {
configuration = configuration(pr.repository(), ".jcheck/conf", pr.targetRef());
} else {
configuration = configuration(confOverrideRepo, confOverrideName, confOverrideRef);
}
var census = Census.parse(repoFolder);
var project = project(configuration, census);
var namespace = namespace(census, pr.repository().namespace());
@@ -78,7 +78,10 @@ private CheckRun(CheckWorkItem workItem, PullRequest pr, Repository localRepo, L
this.ignoreStaleReviews = ignoreStaleReviews;

baseHash = PullRequestUtils.baseHash(pr, localRepo);
checkablePullRequest = new CheckablePullRequest(pr, localRepo, ignoreStaleReviews);
checkablePullRequest = new CheckablePullRequest(pr, localRepo, ignoreStaleReviews,
workItem.bot.confOverrideRepository().orElse(null),
workItem.bot.confOverrideName(),
workItem.bot.confOverrideRef());
}

static void execute(CheckWorkItem workItem, PullRequest pr, Repository localRepo, List<Comment> comments,
@@ -135,7 +135,8 @@ public String toString() {
@Override
public Collection<WorkItem> run(Path scratchPath) {
// First determine if the current state of the PR has already been checked
var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);
var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr,
bot.confOverrideRepository().orElse(null), bot.confOverrideName(), bot.confOverrideRef());
var comments = pr.comments();
var allReviews = pr.reviews();
var labels = new HashSet<>(pr.labels());
@@ -25,9 +25,10 @@
import org.openjdk.skara.census.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.jcheck.JCheck;
import org.openjdk.skara.jcheck.*;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.*;
import org.openjdk.skara.vcs.openjdk.Issue;

import java.io.*;
import java.util.*;
@@ -37,11 +38,19 @@ public class CheckablePullRequest {
private final PullRequest pr;
private final Repository localRepo;
private final boolean ignoreStaleReviews;
private final List<String> confOverride;

CheckablePullRequest(PullRequest pr, Repository localRepo, boolean ignoreStaleReviews) {
CheckablePullRequest(PullRequest pr, Repository localRepo, boolean ignoreStaleReviews,
HostedRepository jcheckRepo, String jcheckName, String jcheckRef) {
this.pr = pr;
this.localRepo = localRepo;
this.ignoreStaleReviews = ignoreStaleReviews;

if (jcheckRepo != null) {
confOverride = jcheckRepo.fileContents(jcheckName, jcheckRef).lines().collect(Collectors.toList());
} else {
confOverride = null;
}
}

private String commitMessage(List<Review> activeReviews, Namespace namespace) throws IOException {
@@ -131,8 +140,17 @@ PullRequestCheckIssueVisitor createVisitor(Hash localHash, CensusInstance census
}

void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestCheckIssueVisitor visitor, List<String> additionalConfiguration) throws IOException {
Optional<JCheckConfiguration> conf;
if (confOverride != null) {
conf = JCheck.parseConfiguration(confOverride, additionalConfiguration);
} else {
conf = JCheck.parseConfiguration(localRepo, pr.targetHash(), additionalConfiguration);
}
if (conf.isEmpty()) {
throw new RuntimeException("Failed to parse jcheck configuration at: " + pr.targetHash() + " with extra: " + additionalConfiguration);
}
try (var issues = JCheck.check(localRepo, censusInstance.census(), CommitMessageParsers.v1, localHash,
pr.targetHash(), additionalConfiguration)) {
conf.get())) {
for (var issue : issues) {
issue.accept(visitor);
}
@@ -217,7 +217,8 @@ public Collection<WorkItem> run(Path scratchPath) {
return List.of(new LabelerWorkItem(bot, pr, errorHandler));
}

var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);
var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr,
bot.confOverrideRepository().orElse(null), bot.confOverrideName(), bot.confOverrideRef());
var command = nextCommand.get();
log.info("Processing command: " + command.id() + " - " + command.name());
processCommand(pr, census, scratchPath.resolve("pr").resolve("command"), command, comments);
@@ -100,7 +100,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews());
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews(),
bot.confOverrideRepository().orElse(null),
bot.confOverrideName(),
bot.confOverrideRef());

// Validate the target hash if requested
var rebaseMessage = new StringWriter();
@@ -48,6 +48,9 @@ class PullRequestBot implements Bot {
private final Set<String> allowedIssueTypes;
private final Pattern allowedTargetBranches;
private final Path seedStorage;
private final HostedRepository confOverrideRepo;
private final String confOverrideName;
private final String confOverrideRef;
private final ConcurrentMap<Hash, Boolean> currentLabels;
private final PullRequestUpdateCache updateCache;
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
@@ -56,7 +59,8 @@ class PullRequestBot implements Bot {
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) {
Set<String> allowedIssueTypes, Pattern allowedTargetBranches, Path seedStorage,
HostedRepository confOverrideRepo, String confOverrideName, String confOverrideRef) {
remoteRepo = repo;
this.censusRepo = censusRepo;
this.censusRef = censusRef;
@@ -70,6 +74,9 @@ class PullRequestBot implements Bot {
this.allowedIssueTypes = allowedIssueTypes;
this.allowedTargetBranches = allowedTargetBranches;
this.seedStorage = seedStorage;
this.confOverrideRepo = confOverrideRepo;
this.confOverrideName = confOverrideName;
this.confOverrideRef = confOverrideRef;

this.currentLabels = new ConcurrentHashMap<>();
this.updateCache = new PullRequestUpdateCache();
@@ -196,4 +203,16 @@ Pattern allowedTargetBranches() {
Optional<Path> seedStorage() {
return Optional.ofNullable(seedStorage);
}

Optional<HostedRepository> confOverrideRepository() {
return Optional.ofNullable(confOverrideRepo);
}

String confOverrideName() {
return confOverrideName;
}

String confOverrideRef() {
return confOverrideRef;
}
}
@@ -22,8 +22,7 @@
*/
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.forge.HostedRepository;
import org.openjdk.skara.forge.LabelConfiguration;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.issuetracker.IssueProject;

import java.nio.file.Path;
@@ -44,6 +43,9 @@ public class PullRequestBotBuilder {
private Set<String> allowedIssueTypes = null;
private Pattern allowedTargetBranches = Pattern.compile(".*");
private Path seedStorage = null;
private HostedRepository confOverrideRepo = null;
private String confOverrideName = ".conf/jcheck";
private String confOverrideRef = "master";

PullRequestBotBuilder() {
}
@@ -113,10 +115,25 @@ public PullRequestBotBuilder seedStorage(Path seedStorage) {
return this;
}

public PullRequestBotBuilder confOverrideRepo(HostedRepository confOverrideRepo) {
this.confOverrideRepo = confOverrideRepo;
return this;
}

public PullRequestBotBuilder confOverrideName(String confOverrideName) {
this.confOverrideName = confOverrideName;
return this;
}

public PullRequestBotBuilder confOverrideRef(String confOverrideRef) {
this.confOverrideRef = confOverrideRef;
return this;
}

public PullRequestBot build() {
return new PullRequestBot(repo, censusRepo, censusRef, labelConfiguration, externalCommands,
blockingCheckLabels, readyLabels, readyComments, issueProject,
ignoreStaleReviews, allowedIssueTypes, allowedTargetBranches,
seedStorage);
seedStorage, confOverrideRepo, confOverrideName, confOverrideRef);
}
}
@@ -105,6 +105,13 @@ public List<Bot> create(BotConfiguration configuration) {
if (repo.value().contains("targetbranches")) {
botBuilder.allowedTargetBranches(repo.value().get("targetbranches").asString());
}
if (repo.value().contains("jcheck")) {
botBuilder.confOverrideRepo(configuration.repository(repo.value().get("jcheck").get("repo").asString()));
botBuilder.confOverrideRef(configuration.repositoryRef(repo.value().get("jcheck").get("repo").asString()));
if (repo.value().get("jcheck").contains("name")) {
botBuilder.confOverrideName(repo.value().get("jcheck").get("name").asString());
}
}

ret.add(botBuilder.build());
}
@@ -85,7 +85,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews());
var checkablePr = new CheckablePullRequest(pr, localRepo, bot.ignoreStaleReviews(),
bot.confOverrideRepository().orElse(null),
bot.confOverrideName(),
bot.confOverrideRef());

// Validate the target hash if requested
var rebaseMessage = new StringWriter();
@@ -1528,4 +1528,95 @@ void expandTitleWithIssueId(TestInfo testInfo) throws IOException {
assertEquals(numericId + ": " + bug.title(), bugPR.title());
}
}

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

var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id());
var checkBot = PullRequestBot.newBuilder()
.repo(author)
.censusRepo(censusBuilder.build())
.confOverrideRepo(conf)
.confOverrideName("jcheck.conf")
.confOverrideRef("jcheck-branch")
.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);

// Create a different conf on a different branch
var defaultConf = Files.readString(localRepo.root().resolve(".jcheck/conf"), StandardCharsets.UTF_8);
var newConf = defaultConf.replace("reviewers=1", "reviewers=0");
Files.writeString(localRepo.root().resolve("jcheck.conf"), newConf, StandardCharsets.UTF_8);
localRepo.add(localRepo.root().resolve("jcheck.conf"));
var confHash = localRepo.commit("Separate conf", "duke", "duke@openjdk.org");
localRepo.push(confHash, author.url(), "jcheck-branch", true);
localRepo.checkout(masterHash, true);

// Make a change with a corresponding PR
var testHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(testHash, author.url(), "test", true);
var pr = credentials.createPullRequest(author, "master", "test", "This is a PR");

// Check the status (should become ready immediately as reviewercount is overridden to 0)
TestBotRunner.runPeriodicItems(checkBot);
assertEquals(Set.of("rfr", "ready"), new HashSet<>(pr.labels()));
}
}

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

var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id());
var checkBot = PullRequestBot.newBuilder()
.repo(author)
.censusRepo(censusBuilder.build())
.confOverrideRepo(conf)
.confOverrideName("jcheck.conf")
.confOverrideRef("jcheck-branch")
.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);

// Create a different conf on a different branch
var defaultConf = Files.readString(localRepo.root().resolve(".jcheck/conf"), StandardCharsets.UTF_8);
var newConf = defaultConf.replace("reviewers=1", "reviewers=0");
Files.writeString(localRepo.root().resolve("jcheck.conf"), newConf, StandardCharsets.UTF_8);
localRepo.add(localRepo.root().resolve("jcheck.conf"));
var confHash = localRepo.commit("Separate conf", "duke", "duke@openjdk.org");
localRepo.push(confHash, author.url(), "jcheck-branch", true);
localRepo.checkout(masterHash, true);

// Remove the default one
localRepo.remove(localRepo.root().resolve(".jcheck/conf"));
var newMasterHash = localRepo.commit("No more conf", "duke", "duke@openjdk.org");
localRepo.push(newMasterHash, author.url(), "master");

// Make a change with a corresponding PR
var testHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(testHash, author.url(), "test", true);
var pr = credentials.createPullRequest(author, "master", "test", "This is a PR");

// Check the status (should become ready immediately as reviewercount is overridden to 0)
TestBotRunner.runPeriodicItems(checkBot);
assertEquals(Set.of("rfr", "ready"), new HashSet<>(pr.labels()));
}
}
}

1 comment on commit e36b503

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented on e36b503 Aug 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.