Skip to content

Commit

Permalink
2159: CheckRun repeats search for backport commit on each evaluation
Browse files Browse the repository at this point in the history
Reviewed-by: erikj
  • Loading branch information
zhaosongzs committed Jan 29, 2024
1 parent 63018e9 commit f300ab2
Show file tree
Hide file tree
Showing 11 changed files with 70 additions and 34 deletions.
10 changes: 9 additions & 1 deletion bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,15 @@ private Optional<HostedCommit> backportedFrom() {
if (hash == null) {
return Optional.empty();
}
var commit = pr.repository().forge().search(hash, true);
var repoName = checkablePullRequest.findOriginalBackportRepo();
if (repoName == null) {
repoName = pr.repository().forge().search(hash).orElseThrow();
}
var repo = pr.repository().forge().repository(repoName);
if (repo.isEmpty()) {
throw new IllegalStateException("Backport comment for PR " + pr.id() + " contains bad repo name: " + repoName);
}
var commit = repo.get().commit(hash, true);
if (commit.isEmpty()) {
throw new IllegalStateException("Backport comment for PR " + pr.id() + " contains bad hash: " + hash.hex());
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -524,9 +524,11 @@ public Collection<WorkItem> prRun(ScratchArea scratchArea) {
} catch (IOException e) {
throw new UncheckedIOException(e);
}
var metadata = pr.repository().forge().search(hash);
if (metadata.isPresent()) {
var message = CommitMessageParsers.v1.parse(metadata.get().message());
var forge = pr.repository().forge();
var repoName = forge.search(hash);
if (repoName.isPresent()) {
var commit = forge.repository(repoName.get()).flatMap(repository -> repository.commit(hash));
var message = CommitMessageParsers.v1.parse(commit.orElseThrow().message());
var issues = message.issues();
var comment = new ArrayList<String>();
if (issues.isEmpty()) {
Expand All @@ -550,6 +552,7 @@ public Collection<WorkItem> prRun(ScratchArea scratchArea) {
}
pr.setTitle(id + ": " + issue.get().title());
comment.add("<!-- backport " + hash.hex() + " -->\n");
comment.add("<!-- repo " + repoName.get() + " -->\n");
for (var additionalIssue : issues.subList(1, issues.size())) {
comment.add(SolvesTracker.setSolvesMarker(additionalIssue));
}
Expand All @@ -565,13 +568,12 @@ public Collection<WorkItem> prRun(ScratchArea scratchArea) {
if (!summary.isEmpty()) {
text += " and summary";
}
text += " from the original [commit](" + metadata.get().url() + ").";
text += " from the original [commit](" + commit.get().url() + ").";
comment.add(text);
pr.addComment(String.join("\n", comment));
pr.addLabel("backport");
return List.of(CheckWorkItem.fromWorkItem(bot, prId, errorHandler, triggerUpdatedAt));
} else {
var botUser = pr.repository().forge().currentUser();
var text = "<!-- backport error -->\n" +
":warning: @" + pr.author().username() + " could not find any commit with hash `" +
hash.hex() + "`. Please update the title with the hash for an existing commit.";
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -41,6 +41,7 @@

public class CheckablePullRequest {
private static final Pattern BACKPORT_PATTERN = Pattern.compile("<!-- backport ([0-9a-z]{40}) -->");
private static final Pattern BACKPORT_REPO_PATTERN = Pattern.compile("<!-- repo (.+) -->");

private final PullRequest pr;
private final Repository localRepo;
Expand Down Expand Up @@ -311,6 +312,10 @@ Hash findOriginalBackportHash() {
return findOriginalBackportHash(pr, comments);
}

String findOriginalBackportRepo() {
return findOriginalBackportRepo(pr, comments);
}

static Hash findOriginalBackportHash(PullRequest pr, List<Comment> comments) {
var botUser = pr.repository().forge().currentUser();
return comments
Expand All @@ -324,6 +329,19 @@ static Hash findOriginalBackportHash(PullRequest pr, List<Comment> comments) {
.orElse(null);
}

static String findOriginalBackportRepo(PullRequest pr, List<Comment> comments) {
var botUser = pr.repository().forge().currentUser();
return comments
.stream()
.filter(c -> c.author().equals(botUser))
.flatMap(c -> Stream.of(c.body().split("\n")))
.map(BACKPORT_REPO_PATTERN::matcher)
.filter(Matcher::find)
.reduce((first, second) -> second)
.map(l -> l.group(1))
.orElse(null);
}

// Lazily initiated
private Hash targetHash;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public boolean isMemberOf(String groupId, HostUser user) {
}

@Override
public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
public Optional<String> search(Hash hash, boolean includeDiffs) {
return Optional.empty();
}

Expand Down
8 changes: 4 additions & 4 deletions forge/src/main/java/org/openjdk/skara/forge/Forge.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -47,10 +47,10 @@ public interface Forge extends Host {
* Search the whole host for a commit by hash.
* @param hash Hash to search for
* @param includeDiffs Set to true to include parent diffs in Commit, default false
* @return Commit instance if found, otherwise empty
* @return Repository name if found, otherwise empty
*/
Optional<HostedCommit> search(Hash hash, boolean includeDiffs);
default Optional<HostedCommit> search(Hash hash) {
Optional<String> search(Hash hash, boolean includeDiffs);
default Optional<String> search(Hash hash) {
return search(hash, false);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -79,7 +79,7 @@ public Optional<HostedRepository> repository(String name) {
}

@Override
public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
public Optional<String> search(Hash hash, boolean includeDiffs) {
throw new UnsupportedOperationException();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -443,7 +443,7 @@ public boolean isMemberOf(String groupId, HostUser user) {
}

@Override
public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
public Optional<String> search(Hash hash, boolean includeDiffs) {
var orgsToSearch = orgs.stream().map(o -> "org:" + o).collect(Collectors.joining(" "));
if (!orgsToSearch.isEmpty()) {
orgsToSearch = " " + orgsToSearch;
Expand All @@ -457,9 +457,8 @@ public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
// There is no good way of knowing for sure which repository we would rather
// get the commit from, but a reasonable default is to go by the shortest
// name as that is most likely the main repository of the project.
var shortestName = items.stream()
return items.stream()
.map(o -> o.get("repository").get("full_name").asString())
.min(Comparator.comparing(String::length));
return shortestName.flatMap(this::repository).flatMap(r -> r.commit(hash, includeDiffs));
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -34,7 +34,6 @@
import java.nio.charset.StandardCharsets;
import java.util.*;
import java.util.logging.Logger;
import java.util.stream.Collectors;

public class GitLabHost implements Forge {
private final String name;
Expand Down Expand Up @@ -235,8 +234,7 @@ public boolean isMemberOf(String groupId, HostUser user) {
}

@Override
public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
var hex = hash.hex();
public Optional<String> search(Hash hash, boolean includeDiffs) {
for (var group : groups) {
var ids = request.get("groups/" + group + "/projects")
.execute()
Expand All @@ -247,12 +245,12 @@ public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
// path name as that is most likely the main repository of the project.
.sorted(Comparator.comparing(o -> o.get("path").asString().length()))
.map(o -> o.get("id").asInt())
.collect(Collectors.toList());
.toList();
for (var id : ids) {
var project = new GitLabRepository(this, id);
var commit = project.commit(hash, includeDiffs);
if (commit.isPresent()) {
return commit;
return Optional.of(project.name());
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ void testClosedBy() {
}

@Test
void test() {
void testGeneratingUrl() {
var username = settings.getProperty("github.user");
var token = settings.getProperty("github.pat");
var credential = new Credential(username, token);
Expand Down Expand Up @@ -293,8 +293,13 @@ void testBackportCleanIgnoreCopyRight() {
var gitHubRepo = githubHost.repository(settings.getProperty("github.repository")).orElseThrow();

var pr = gitHubRepo.pullRequest(settings.getProperty("github.prId"));
var commit = pr.repository().forge().search(new Hash(settings.getProperty("github.commitHash")), true);
var backportDiff = commit.get().parentDiffs().get(0);
var hash = new Hash(settings.getProperty("github.commitHash"));
var repoName = pr.repository().forge().search(hash, true);
assertTrue(repoName.isPresent());
var repository = pr.repository().forge().repository(repoName.get());
assertTrue(repository.isPresent());
var commit = repository.get().commit(hash, true);
var backportDiff = commit.orElseThrow().parentDiffs().get(0);
var prDiff = pr.diff();
var isClean = DiffComparator.areFuzzyEqual(backportDiff, prDiff);
assertTrue(isClean);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2022, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2022, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -24,6 +24,7 @@

import java.nio.file.Path;
import java.time.ZonedDateTime;
import java.util.Arrays;
import java.util.Properties;
import java.util.Set;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -61,7 +62,7 @@ void setupGitLab() throws IOException {
var token = settings.getProperty("gitlab.pat");
var credential = new Credential(username, token);
var uri = URIBuilder.base(settings.getProperty("gitlab.uri")).build();
gitLabHost = new GitLabHost("gitlab", uri, false, credential, List.of());
gitLabHost = new GitLabHost("gitlab", uri, false, credential, Arrays.asList(settings.getProperty("gitlab.group").split(",")));
}

@Test
Expand Down Expand Up @@ -253,7 +254,12 @@ void testBackportCleanIgnoreCopyRight() {
var gitLabRepo = gitLabHost.repository(settings.getProperty("gitlab.repository")).orElseThrow();

var pr = gitLabRepo.pullRequest(settings.getProperty("gitlab.prId"));
var commit = pr.repository().forge().search(new Hash(settings.getProperty("gitlab.commitHash")), true);
var hash = new Hash(settings.getProperty("gitlab.commitHash"));
var repoName = pr.repository().forge().search(hash, true);
assertTrue(repoName.isPresent());
var repository = pr.repository().forge().repository(repoName.get());
assertTrue(repository.isPresent());
var commit = repository.get().commit(hash, true);
var backportDiff = commit.orElseThrow().parentDiffs().get(0);
var prDiff = pr.diff();
var isClean = DiffComparator.areFuzzyEqual(backportDiff, prDiff);
Expand Down
6 changes: 3 additions & 3 deletions test/src/main/java/org/openjdk/skara/test/TestHost.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2023, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2024, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -184,12 +184,12 @@ public boolean isMemberOf(String groupId, HostUser user) {
}

@Override
public Optional<HostedCommit> search(Hash hash, boolean includeDiffs) {
public Optional<String> search(Hash hash, boolean includeDiffs) {
for (var key : data.repositories.keySet()) {
var repo = repository(key).orElseThrow();
var commit = repo.commit(hash, includeDiffs);
if (commit.isPresent()) {
return commit;
return Optional.of(repo.name());
}
}
return Optional.empty();
Expand Down

1 comment on commit f300ab2

@openjdk-notifier
Copy link

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.