Skip to content
Permalink
Browse files
340: Infer master for repos in the same org for merge style PRs
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Mar 31, 2020
1 parent 87965d8 commit 32bba611984aa1e8f83e32f1165e1e26ebb93d53
Showing 3 changed files with 80 additions and 5 deletions.
@@ -93,7 +93,7 @@ private List<String> allowedTargetBranches() {
}

// For unknown contributors, check that all commits have the same name and email
private boolean checkCommitAuthor(List<Commit> commits) throws IOException {
private boolean checkCommitAuthor(List<CommitMetadata> commits) throws IOException {
var author = censusInstance.namespace().get(pr.author().id());
if (author != null) {
return true;
@@ -127,6 +127,17 @@ private Optional<MergeSource> mergeSource() {
if (!branchMatcher.matches()) {
return Optional.empty();
}

// Verify that the branch exists
var isValidBranch = prInstance.remoteBranches().stream()
.map(Reference::name)
.anyMatch(branch -> branch.equals(branchMatcher.group(1)));
if (!isValidBranch) {
// Assume the name refers to a sibling repository
var repoName = Path.of(pr.repository().name()).resolveSibling(branchMatcher.group(1)).toString();
return Optional.of(new MergeSource(repoName, "master"));
}

return Optional.of(new MergeSource(pr.repository().name(), branchMatcher.group(1)));
}

@@ -152,7 +163,7 @@ private List<String> botSpecificChecks() throws IOException {

var baseHash = prInstance.baseHash();
var headHash = pr.headHash();
var commits = prInstance.localRepo().commits(baseHash + ".." + headHash).asList();
var commits = prInstance.localRepo().commitMetadata(baseHash, headHash);

if (!checkCommitAuthor(commits)) {
var error = "For contributors who are not existing OpenJDK Authors, commit attribution will be taken from " +
@@ -184,8 +195,12 @@ private List<String> botSpecificChecks() throws IOException {
);
try {
var sourceHash = prInstance.localRepo().fetch(mergeSourceRepo.url(), source.get().branchName);
if (!prInstance.localRepo().isAncestor(commits.get(mergeCommitIndex + 1).hash(), sourceHash)) {
ret.add("The merge contains commits that are not ancestors of the source.");
var mergeCommit = commits.get(mergeCommitIndex);
for (int i = 1; i < mergeCommit.parents().size(); ++i) {
if (!prInstance.localRepo().isAncestor(mergeCommit.parents().get(i), sourceHash)) {
ret.add("The merge contains commits that are not ancestors of the source.");
break;
}
}
} catch (IOException e) {
ret.add("Could not fetch branch `" + source.get().branchName + "` from project `" +
@@ -121,7 +121,7 @@ private Hash commitSquashed(List<Review> activeReviews, Namespace namespace, Str

if (contributor == null) {
// Use the information contained in the head commit - jcheck has verified that it contains sane values
var headCommit = localRepo.commits(headHash.hex() + "^.." + headHash.hex()).asList().get(0);
var headCommit = localRepo.commitMetadata(headHash.hex() + "^.." + headHash.hex()).get(0);
author = headCommit.author();
} else {
author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain);
@@ -590,6 +590,66 @@ void invalidSourceBranch(TestInfo testInfo) throws IOException {
}
}

@Test
void inferredSourceProject(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.url(), "master", true);

// Make a change in another branch
var otherHash = CheckableRepository.appendAndCommit(localRepo, "Change in other",
"Other\n\nReviewed-by: integrationreviewer2");
localRepo.push(otherHash, author.url(), "other", true);

// 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.push(updatedMaster, author.url(), "master");

localRepo.merge(otherHash);
var mergeHash = localRepo.commit("Merge commit", "some", "some@one");
localRepo.push(mergeHash, author.url(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "Merge otherxyz");

// Approve it as another user
var approvalPr = integrator.pullRequest(pr.id());
approvalPr.addReview(Review.Verdict.APPROVED, "Approved");

// Let the bot check the status
TestBotRunner.runPeriodicItems(mergeBot);

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

// The bot should reply with a failure message
var error = pr.comments().stream()
.filter(comment -> comment.body().contains("did not complete successfully"))
.count();
assertEquals(1, error, () -> pr.comments().stream().map(Comment::body).collect(Collectors.joining("\n\n")));

var check = pr.checks(mergeHash).get("jcheck");
assertEquals("- Could not find project `otherxyz` - check that it is correct.", check.summary().orElseThrow());
}
}

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

0 comments on commit 32bba61

Please sign in to comment.