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

1076: Not possible to use 'Backport <sha>' if original bug doesn't have public sha #1186

Closed
wants to merge 7 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
@@ -40,13 +40,13 @@
import java.util.logging.Logger;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

class CheckWorkItem extends PullRequestWorkItem {
private final Pattern metadataComments = Pattern.compile("<!-- (?:(add|remove) (?:contributor|reviewer))|(?:summary: ')|(?:solves: ')|(?:additional required reviewers)");
private final Logger log = Logger.getLogger("org.openjdk.skara.bots.pr");
static final Pattern ISSUE_ID_PATTERN = Pattern.compile("^(?:(?<prefix>[A-Za-z][A-Za-z0-9]+)-)?(?<id>[0-9]+)(?::\\s+(?<title>.+))?$");
private static final Pattern BACKPORT_TITLE_PATTERN = Pattern.compile("^Backport\\s*([0-9a-z]{40})\\s*$");
private static final Pattern BACKPORT_HASH_TITLE_PATTERN = Pattern.compile("^Backport\\s*([0-9a-z]{40})\\s*$");
private static final Pattern BACKPORT_ISSUE_TITLE_PATTERN = Pattern.compile("^Backport\\s*(?:(?<prefix>[A-Za-z][A-Za-z0-9]+)-)?(?<id>[0-9]+)\\s*$");
private static final String ELLIPSIS = "…";

CheckWorkItem(PullRequestBot bot, PullRequest pr, Consumer<RuntimeException> errorHandler) {
@@ -227,9 +227,9 @@ public Collection<WorkItem> run(Path scratchPath) {
return List.of();
}

var m = BACKPORT_TITLE_PATTERN.matcher(pr.title());
if (m.matches()) {
var hash = new Hash(m.group(1));
var backportHashMatcher = BACKPORT_HASH_TITLE_PATTERN.matcher(pr.title());
if (backportHashMatcher.matches()) {
var hash = new Hash(backportHashMatcher.group(1));
try {
var localRepo = materializeLocalRepo(scratchPath, hostedRepositoryPool);
if (localRepo.isAncestor(hash, pr.headHash())) {
@@ -299,6 +299,37 @@ public Collection<WorkItem> run(Path scratchPath) {
}
}

// Check for a title of the form Backport <issueid>
var backportIssueMatcher = BACKPORT_ISSUE_TITLE_PATTERN.matcher(pr.title());
if (backportIssueMatcher.matches()) {
var prefix = getMatchGroup(backportIssueMatcher, "prefix");
var id = getMatchGroup(backportIssueMatcher, "id");
var project = bot.issueProject();

if (!prefix.isEmpty() && !prefix.equalsIgnoreCase(project.name())) {
var text = "<!-- backport error -->\n" +
":warning: @" + pr.author().username() + " the issue prefix `" + prefix + "` does not" +
" match project [" + project.name() + "](" + project.webUrl() + ").";
addBackportErrorComment(text, comments);
return List.of();
}
var issue = project.issue(id);
if (issue.isEmpty()) {
var text = "<!-- backport error -->\n" +
":warning: @" + pr.author().username() + " the issue with id `" + id + "` " +
"does not exist in project [" + project.name() + "](" + project.webUrl() + ").";
addBackportErrorComment(text, comments);
return List.of();
}
pr.setTitle(id + ": " + issue.get().title());
var text = "This backport pull request has now been updated with the original issue," +
" but not the original commit. If you have the original commit hash, please update" +
" the pull request title with `Backport <hash>`.";
pr.addComment(text);
pr.addLabel("backport");
return List.of(new CheckWorkItem(bot, pr.repository().pullRequest(pr.id()), errorHandler));
}

// If the title needs updating, we run the check again
if (updateTitle()) {
return List.of(new CheckWorkItem(bot, pr.repository().pullRequest(pr.id()), errorHandler));
@@ -266,6 +266,10 @@ Optional<Hash> mergeTarget(PrintWriter reply) {
}

Hash findOriginalBackportHash() {
return findOriginalBackportHash(pr);
}

static Hash findOriginalBackportHash(PullRequest pr) {
var botUser = pr.repository().forge().currentUser();
var backportLines = pr.comments()
.stream()
@@ -61,8 +61,10 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
return;
}

if (!pr.labelNames().contains("backport")) {
reply.println("@" + username + " can only mark [backport pull requests](https://wiki.openjdk.java.net/display/SKARA/Backports#Backports-BackportPullRequests) as clean");
if (!pr.labelNames().contains("backport") || CheckablePullRequest.findOriginalBackportHash(pr) == null) {
reply.println("@" + username + " can only mark [backport pull requests]" +
"(https://wiki.openjdk.java.net/display/SKARA/Backports#Backports-BackportPullRequests)," +
" with an original hash, as clean");
return;
}

@@ -1347,4 +1347,116 @@ void badIssueInOriginal(TestInfo testInfo) throws IOException {
assertFalse(pr.labelNames().contains("backport"));
}
}

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

var author = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();
var issues = credentials.getIssueProject();
var censusBuilder = credentials.getCensusBuilder()
.addCommitter(author.forge().currentUser().id())
.addReviewer(integrator.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id());
var bot = PullRequestBot.newBuilder()
.repo(integrator)
.censusRepo(censusBuilder.build())
.issueProject(issues)
.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);

var issue1 = credentials.createIssue(issues, "An issue");
var issue1Number = issue1.id().split("-")[1];

// Create change
localRepo.checkout(localRepo.defaultBranch());
var editBranch = localRepo.branch(masterHash, "edit");
localRepo.checkout(editBranch);
var newFile2 = localRepo.root().resolve("a_new_file.txt");
Files.writeString(newFile2, "hello");
localRepo.add(newFile2);
var editHash = localRepo.commit("Backport", "duke", "duke@openjdk.java.net");
localRepo.push(editHash, author.url(), "refs/heads/edit", true);

// Create various kinds of bad pull request titles
// Use a bad project
var pr = credentials.createPullRequest(author, "master", "edit",
"Backport " + "FOO-" + issue1.id().split("-")[1]);
TestBotRunner.runPeriodicItems(bot);
var backportComment = pr.comments().get(0).body();
assertTrue(backportComment.contains("does not match project"));
assertFalse(pr.labelNames().contains("backport"));

// Use bad issue ID
pr.setTitle("Backport TEST-4711");
TestBotRunner.runPeriodicItems(bot);
backportComment = pr.comments().get(1).body();
assertTrue(backportComment.contains("does not exist in project"));
assertFalse(pr.labelNames().contains("backport"));

// Use different kinds of good titles
// Use the full issue ID
pr.setTitle("Backport " + issue1.id());
TestBotRunner.runPeriodicItems(bot);
backportComment = pr.comments().get(2).body();
assertTrue(backportComment.contains("This backport pull request has now been updated with the original issue"));
assertEquals(issue1Number + ": An issue", pr.title());
assertTrue(pr.labelNames().contains("backport"));

// Set the title without project name
pr.setTitle("Backport " + issue1.id().split("-")[1]);
TestBotRunner.runPeriodicItems(bot);
backportComment = pr.comments().get(3).body();
assertTrue(backportComment.contains("This backport pull request has now been updated with the original issue"));
assertEquals(issue1Number + ": An issue", pr.title());
assertTrue(pr.labelNames().contains("backport"));

// Approve PR and re-run bot
var prAsReviewer = reviewer.pullRequest(pr.id());
prAsReviewer.addReview(Review.Verdict.APPROVED, "Looks good");
TestBotRunner.runPeriodicItems(bot);
assertLastCommentContains(pr, "This change now passes all *automated* pre-integration checks");

// Integrate
var prAsCommitter = author.pullRequest(pr.id());
prAsCommitter.addComment("/integrate");
TestBotRunner.runPeriodicItems(bot);

// Find the commit
assertLastCommentContains(pr, "Pushed as commit");

String hex = null;
var comment = pr.comments().get(pr.comments().size() - 1);
var lines = comment.body().split("\n");
var pattern = Pattern.compile(".* Pushed as commit ([0-9a-z]{40}).*");
for (var line : lines) {
var m = pattern.matcher(line);
if (m.matches()) {
hex = m.group(1);
break;
}
}
assertNotNull(hex);
assertEquals(40, hex.length());
localRepo.checkout(localRepo.defaultBranch());
localRepo.pull(author.url().toString(), "master", false);
var commit = localRepo.lookup(new Hash(hex)).orElseThrow();

var message = CommitMessageParsers.v1.parse(commit);
assertEquals(1, message.issues().size());
assertEquals("An issue", message.issues().get(0).description());
assertEquals(List.of("integrationreviewer3"), message.reviewers());
assertEquals(Optional.empty(), message.original());
assertEquals(List.of(), message.contributors());
assertEquals(List.of(), message.summaries());
assertEquals(List.of(), message.additional());
Copy link
Member

@kevinrushforth kevinrushforth Jun 11, 2021

Choose a reason for hiding this comment

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

It might be good to also check that the commit message does not contain "Backport of", or is this covered by checking that message.additional() is empty?

Copy link
Member Author

@erikj79 erikj79 Jun 11, 2021

Choose a reason for hiding this comment

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

Good point. I just removed the assert for the existence of the original hash without thinking about inverting that assert.

}
}
}
@@ -73,7 +73,7 @@ void cleanCommandOnRegularPullRequestShouldNotWork(TestInfo testInfo) throws IOE
assertFalse(pr.labelNames().contains("backport"));
assertFalse(pr.labelNames().contains("clean"));
assertLastCommentContains(pr, "can only mark [backport pull requests]");
assertLastCommentContains(pr, "as clean");
assertLastCommentContains(pr, ", with an original hash, as clean");
}
}

@@ -297,4 +297,49 @@ void authorShouldNotBeAllowed(TestInfo testInfo) throws IOException {
assertLastCommentContains(pr, "can use the `/clean` command");
}
}

@Test
void missingBackportHash(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 issues = credentials.getIssueProject();
var prBot = PullRequestBot.newBuilder()
.repo(integrator)
.censusRepo(censusBuilder.build())
.issueProject(issues)
.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);

var issue = credentials.createIssue(issues, "An issue");

// 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", "Backport " + issue.id());
TestBotRunner.runPeriodicItems(prBot);

assertTrue(pr.labelNames().contains("backport"));
assertFalse(pr.labelNames().contains("clean"));

// Try to issue the "/clean" PR command, should not work
pr.addComment("/clean");
TestBotRunner.runPeriodicItems(prBot);
assertTrue(pr.labelNames().contains("backport"));
assertFalse(pr.labelNames().contains("clean"));
assertLastCommentContains(pr, "can only mark [backport pull requests]");
assertLastCommentContains(pr, ", with an original hash, as clean");
}
}
}