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

694: Checking invalid email gives an exception #837

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

@@ -139,24 +139,6 @@ private IssueProject issueProject() {
.collect(Collectors.toList());
}

// For unknown contributors, check that all commits have the same name and email
private boolean checkCommitAuthor(List<CommitMetadata> commits) throws IOException {
var author = censusInstance.namespace().get(pr.author().id());
if (author != null) {
return true;
}

var names = new HashSet<String>();
var emails = new HashSet<String>();

for (var commit : commits) {
names.add(commit.author().name());
emails.add(commit.author().email());
}

return ((names.size() == 1) && emails.size() == 1);
}

// Additional bot-specific checks that are not handled by JCheck
private List<String> botSpecificChecks(Hash finalHash) throws IOException {
var ret = new ArrayList<String>();
@@ -191,13 +173,6 @@ private boolean checkCommitAuthor(List<CommitMetadata> commits) throws IOExcepti
var headHash = pr.headHash();
var originalCommits = localRepo.commitMetadata(baseHash, headHash);

if (!checkCommitAuthor(originalCommits)) {
var error = "For contributors who are not existing OpenJDK Authors, commit attribution will be taken from " +
"the commits in the PR. However, the commits in this PR have inconsistent user names and/or " +
"email addresses. Please amend the commits.";
ret.add(error);
}

for (var blocker : workItem.bot.blockingCheckLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
ret.add(blocker.getValue());
@@ -115,7 +115,6 @@ Hash commit(Hash finalHead, Namespace namespace, String censusDomain, String spo
throw new CommitFailure("Merge PRs can only be created by known OpenJDK authors.");
}

// Use the information contained in the head commit - jcheck has verified that it contains sane values
var headCommit = localRepo.commitMetadata(pr.headHash().hex() + "^.." + pr.headHash().hex()).get(0);
author = headCommit.author();
} else {
@@ -162,12 +162,16 @@ public void visit(CommitterEmailIssue issue) {

@Override
public void visit(AuthorNameIssue issue) {
throw new IllegalStateException("Invalid author name: " + issue.commit().author());
// We only get here for contributors without an OpenJDK username
addFailureMessage(issue.check(), "Pull request's HEAD commit must contain a full name");
This conversation was marked as resolved by edvbld

This comment has been minimized.

@rwestberg

rwestberg Sep 21, 2020
Member

This is not strictly true, CheckRun.java around line 194 will check that all commits in a PR contain the same name and email address. Could of course start using just the one in HEAD without that check..

This comment has been minimized.

@edvbld

edvbld Sep 22, 2020
Author Member

Thanks, updated in b49bd90

readyForReview = false;
}

@Override
public void visit(AuthorEmailIssue issue) {
throw new IllegalStateException("Invalid author email: " + issue.commit().author());
// We only get here for contributors without an OpenJDK username
addFailureMessage(issue.check(), "Pull request's HEAD commit must contain a valid e-mail");
readyForReview = false;
}

@Override
@@ -298,57 +298,6 @@ void selfReview(TestInfo testInfo) throws IOException {
}
}

@Test
void multipleCommitters(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()
.addReviewer(reviewer.forge().currentUser().id());
var checkBot = PullRequestBot.newBuilder().repo(author).censusRepo(censusBuilder.build()).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);

// Make two changes with different authors
CheckableRepository.appendAndCommit(localRepo, "First edit", "Edit by number 1",
"number1", "number1@none.none");
var editHash = CheckableRepository.appendAndCommit(localRepo, "Second edit", "Edit by number 2",
"number2", "number2@none.none");
localRepo.push(editHash, author.url(), "refs/heads/edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

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

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

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

// The check should still be failing
checks = pr.checks(editHash);
assertEquals(1, checks.size());
check = checks.get("jcheck");
assertEquals(CheckStatus.FAILURE, check.status());

// The PR should not be flagged as ready for review, as multiple committers is a problem
assertFalse(pr.labels().contains("rfr"));
}
}

@Test
void updatedContentFailsCheck(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
@@ -783,4 +783,45 @@ void existingContributingFile(TestInfo testInfo) throws IOException {
assertTrue(lastComment.body().contains("CONTRIBUTING.md"));
}
}

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

var censusBuilder = credentials.getCensusBuilder()
.addCommitter(committer.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id());
var mergeBot = PullRequestBot.newBuilder().repo(reviewer).censusRepo(censusBuilder.build()).build();

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), 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 with an empty e-mail
var editHash = CheckableRepository.appendAndCommit(localRepo, "Content", "A commit", "Duke", "");
localRepo.push(editHash, author.url(), "refs/heads/edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Run the bot
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should respond with a failure about missing e-mail
pr = author.pullRequest(pr.id());
assertFalse(pr.labels().contains("ready"));
var checks = pr.checks(pr.headHash());
assertTrue(checks.containsKey("jcheck"));
var jcheck = checks.get("jcheck");
assertEquals(CheckStatus.FAILURE, jcheck.status());
assertTrue(jcheck.summary().isPresent());
var summary = jcheck.summary().get();
assertTrue(summary.contains("Pull request's HEAD commit must contain a valid e-mail"));
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.