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

340: Infer master for repos in the same org for merge style PRs #554

Closed
wants to merge 2 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
@@ -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
Copy link
Member

@edvbld edvbld Mar 31, 2020

Choose a reason for hiding this comment

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

Suggested change
// Verify that the branch exists
// Check if name refers to branch or repository

Copy link
Member Author

@rwestberg rwestberg Mar 31, 2020

Choose a reason for hiding this comment

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

So the check is only done partially here.. :)

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();
Copy link
Member

@edvbld edvbld Mar 31, 2020

Choose a reason for hiding this comment

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

Perhaps check that the repo actually exists?

Copy link
Member Author

@rwestberg rwestberg Mar 31, 2020

Choose a reason for hiding this comment

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

It will be checked a bit later (around line 193).

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);