Skip to content

Commit

Permalink
SKARA-2292
Browse files Browse the repository at this point in the history
  • Loading branch information
@zhaosongzs
zhaosongzs committed Jul 5, 2024
1 parent 273c362 commit 5835d99
Show file tree
Hide file tree
Showing 8 changed files with 170 additions and 37 deletions.
9 changes: 6 additions & 3 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/CommitFormatters.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@
import java.time.format.DateTimeFormatter;

public class CommitFormatters {
public static String toTextBrief(HostedRepository repository, Commit commit) {
public static String toTextBrief(HostedRepository repository, Commit commit, Branch branch) {
var writer = new StringWriter();
var printer = new PrintWriter(writer);

printer.println("Changeset: " + commit.hash().abbreviate());
if (branch != null) {
printer.println("Branch: " + branch.name());
}
printer.println("Author: " + commit.author().name() + " <" + commit.author().email() + ">");
if (!commit.author().equals(commit.committer())) {
printer.println("Committer: " + commit.committer().name() + " <" + commit.committer().email() + ">");
Expand All @@ -56,11 +59,11 @@ private static String patchToText(Patch patch) {
}
}

public static String toText(HostedRepository repository, Commit commit) {
public static String toText(HostedRepository repository, Commit commit, Branch branch) {
var writer = new StringWriter();
var printer = new PrintWriter(writer);

printer.print(toTextBrief(repository, commit));
printer.print(toTextBrief(repository, commit, branch));
printer.println();
printer.println(String.join("\n", commit.message()));
printer.println();
Expand Down
2 changes: 2 additions & 0 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestListener.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ default void onHeadChange(PullRequest pr, Path scratchPath, Hash oldHead) {
}
default void onStateChange(PullRequest pr, Path scratchPath, org.openjdk.skara.issuetracker.Issue.State oldState) {
}
default void onTargetBranchChange(PullRequest pr, Path scratchPath, Issue issue) {
}
String name();

default void initialize(HostedRepository repo) {
Expand Down
15 changes: 12 additions & 3 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,24 @@ class PullRequestState {
private final Hash commitId;
private final Hash head;
private final Issue.State state;
private final String targetBranch;

PullRequestState(PullRequest pr, Set<String> issueIds, Hash commitId, Hash head, Issue.State state) {
this.prId = pr.repository().id() + ":" + pr.id();
this.issueIds = issueIds;
this.commitId = commitId;
this.head = head;
this.state = state;
this.targetBranch = pr.targetRef();
}

PullRequestState(String prId, Set<String> issueIds, Hash commitId, Hash head, Issue.State state) {
PullRequestState(String prId, Set<String> issueIds, Hash commitId, Hash head, Issue.State state, String targetBranch) {
this.prId = prId;
this.issueIds = issueIds;
this.commitId = commitId;
this.head = head;
this.state = state;
this.targetBranch = targetBranch;
}

public String prId() {
Expand All @@ -71,6 +74,10 @@ public Issue.State state() {
return state;
}

public String targetBranch() {
return targetBranch;
}

@Override
public String toString() {
return "PullRequestState{" +
Expand All @@ -79,6 +86,7 @@ public String toString() {
", commitId=" + commitId +
", head=" + head +
", state=" + state +
", targetBranch=" + targetBranch +
'}';
}

Expand All @@ -95,11 +103,12 @@ public boolean equals(Object o) {
issueIds.equals(that.issueIds) &&
Objects.equals(commitId, that.commitId) &&
Objects.equals(head, that.head) &&
Objects.equals(state, that.state);
Objects.equals(state, that.state) &&
Objects.equals(targetBranch, that.targetBranch);
}

@Override
public int hashCode() {
return Objects.hash(prId, issueIds, commitId, head);
return Objects.hash(prId, issueIds, commitId, head, targetBranch);
}
}
21 changes: 18 additions & 3 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/PullRequestWorkItem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,15 @@ private Set<PullRequestState> deserializePrState(String current) {
}

var commit = obj.get("commit").isNull() ?
null : new Hash(obj.get("commit").asString());
null : new Hash(obj.get("commit").asString());
var state = obj.get("state").isNull() ?
null : org.openjdk.skara.issuetracker.Issue.State.valueOf(obj.get("state").asString());
var targetBranch = obj.get("targetBranch") == null ?
null : obj.get("targetBranch").asString();

return new PullRequestState(id, issues, commit, new Hash(obj.get("head").asString()), state);
return new PullRequestState(id, issues, commit, new Hash(obj.get("head").asString()), state, targetBranch);
})
.collect(Collectors.toSet());
.collect(Collectors.toSet());
}

private String serializePrState(Collection<PullRequestState> added, Set<PullRequestState> existing) {
Expand Down Expand Up @@ -133,6 +135,9 @@ private String serializePrState(Collection<PullRequestState> added, Set<PullRequ
} else {
ret.putNull("state");
}
if (pr.targetBranch() != null) {
ret.put("targetBranch", JSON.of(pr.targetBranch()));
}
return ret;
})
.map(JSONObject::toString)
Expand Down Expand Up @@ -176,6 +181,10 @@ private void notifyStateChange(org.openjdk.skara.issuetracker.Issue.State oldSta
listeners.forEach(c -> c.onStateChange(pr, scratchPath.resolve(c.name()), oldState));
}

private void notifyTargetBranchChange(String issueId, Path scratchPath) {
listeners.forEach(c -> c.onTargetBranchChange(pr, scratchPath.resolve(c.name()), new Issue(issueId, "")));
}

private boolean isOfInterest(PullRequest pr) {
var labels = new HashSet<>(pr.labelNames());
if (!(labels.contains("rfr") || labels.contains("integrated"))) {
Expand Down Expand Up @@ -277,6 +286,12 @@ public Collection<WorkItem> run(Path scratchPath) {
if (!storedState.get().state().equals(state.state())) {
notifyStateChange(storedState.get().state(), scratchPath);
}
var storedTargetBranch = storedState.get().targetBranch();
if (storedTargetBranch == null || !storedTargetBranch.equals(state.targetBranch())) {
storedIssues.stream()
.filter(issues::contains)
.forEach(issue -> notifyTargetBranchChange(issue, listenerScratchPath));
}
} else {
notifyNewPr(pr, listenerScratchPath);
issues.forEach(issue -> notifyNewIssue(issue, listenerScratchPath));
Expand Down
56 changes: 41 additions & 15 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,8 @@ public void onIntegratedPullRequest(PullRequest pr, Path scratchPath, Hash hash)
var issue = optionalIssue.get();

if (commitLink) {
var linkBuilder = Link.create(repository.webUrl(hash), "Commit")
.summary(repository.name() + "/" + hash.abbreviate());
var linkBuilder = Link.create(repository.webUrl(hash), "Commit(" + pr.targetRef() + ")")
.summary(repository.name() + "/" + hash.abbreviate());
if (commitIcon != null) {
linkBuilder.iconTitle("Commit");
linkBuilder.iconUrl(commitIcon);
Expand All @@ -224,6 +224,42 @@ public void onIntegratedPullRequest(PullRequest pr, Path scratchPath, Hash hash)
}
}

public void onTargetBranchChange(PullRequest pr, Path scratchPath, org.openjdk.skara.vcs.openjdk.Issue issue) {
var realIssue = issueProject.issue(issue.shortId());
if (realIssue.isEmpty()) {
log.warning("Pull request " + pr + " added unknown issue: " + issue.id());
return;
}

if (reviewLink) {
// Remove the previous link
removeReviewLink(pr, realIssue.get());
// Add a new link
addReviewLink(pr, realIssue.get());
}

log.info("Updating review link comment to issue " + realIssue.get().id());
PullRequestUtils.postPullRequestLinkComment(realIssue.get(), pr);
}

private void addReviewLink(PullRequest pr, IssueTrackerIssue realIssue) {
var linkBuilder = Link.create(pr.webUrl(), "Review(" + pr.targetRef() + ")")
.summary(pr.repository().name() + "/" + pr.id());
if (reviewIcon != null) {
linkBuilder.iconTitle("Review");
linkBuilder.iconUrl(reviewIcon);
}

log.info("Adding review link to issue " + realIssue.id());
realIssue.addLink(linkBuilder.build());
}

private void removeReviewLink(PullRequest pr, IssueTrackerIssue realIssue) {
log.info("Removing review links from issue " + realIssue.id());
var link = Link.create(pr.webUrl(), "").build();
realIssue.removeLink(link);
}

@Override
public void onNewIssue(PullRequest pr, Path scratchPath, org.openjdk.skara.vcs.openjdk.Issue issue) {
var realIssue = issueProject.issue(issue.shortId());
Expand All @@ -233,15 +269,7 @@ public void onNewIssue(PullRequest pr, Path scratchPath, org.openjdk.skara.vcs.o
}

if (reviewLink) {
var linkBuilder = Link.create(pr.webUrl(), "Review")
.summary(pr.repository().name() + "/" + pr.id());
if (reviewIcon != null) {
linkBuilder.iconTitle("Review");
linkBuilder.iconUrl(reviewIcon);
}

log.info("Adding review link to issue " + realIssue.get().id());
realIssue.get().addLink(linkBuilder.build());
addReviewLink(pr, realIssue.get());
}

log.info("Adding review link comment to issue " + realIssue.get().id());
Expand All @@ -256,9 +284,7 @@ public void onRemovedIssue(PullRequest pr, Path scratchPath, org.openjdk.skara.v
return;
}

log.info("Removing review links from issue " + realIssue.get().id());
var link = Link.create(pr.webUrl(), "").build();
realIssue.get().removeLink(link);
removeReviewLink(pr, realIssue.get());

PullRequestUtils.removePullRequestLinkComment(realIssue.get(), pr);
}
Expand All @@ -267,7 +293,7 @@ public void onRemovedIssue(PullRequest pr, Path scratchPath, org.openjdk.skara.v
public void onNewCommits(HostedRepository repository, Repository localRepository, Path scratchPath, List<Commit> commits, Branch branch) {
for (var commit : commits) {
var linkRepository = originalRepository != null ? originalRepository : repository;
var commitNotification = CommitFormatters.toTextBrief(linkRepository, commit);
var commitNotification = CommitFormatters.toTextBrief(linkRepository, commit, branch);
var commitMessage = CommitMessageParsers.v1.parse(commit);
var username = findIssueUsername(commit, scratchPath);

Expand Down
6 changes: 3 additions & 3 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/mailinglist/MailingListNotifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ private void sendCombinedCommits(HostedRepository repository, List<Commit> commi
var printer = new PrintWriter(writer);

for (var commit : commits) {
printer.println(CommitFormatters.toText(repository, commit));
printer.println(CommitFormatters.toText(repository, commit, branch));
}

var subject = commitsToSubject(repository, commits, branch);
Expand Down Expand Up @@ -256,7 +256,7 @@ public void onNewOpenJDKTagCommits(HostedRepository repository, Repository local
if (annotation != null) {
printer.println(tagAnnotationToText(repository, annotation));
}
printer.println(CommitFormatters.toTextBrief(repository, taggedCommit));
printer.println(CommitFormatters.toTextBrief(repository, taggedCommit, null));

printer.println("The following commits are included in " + tag.tag());
printer.println("========================================================");
Expand Down Expand Up @@ -302,7 +302,7 @@ public void onNewTagCommit(HostedRepository repository, Repository localReposito
if (annotation != null) {
printer.println(tagAnnotationToText(repository, annotation));
}
printer.println(CommitFormatters.toTextBrief(repository, commit));
printer.println(CommitFormatters.toTextBrief(repository, commit, null));

var subject = tagToSubject(repository, commit.hash(), tag);
var email = Email.create(subject, writer.toString())
Expand Down
80 changes: 77 additions & 3 deletions bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ void testIssueLinkIdempotence(TestInfo testInfo) throws IOException {
assertEquals(1, links.size());
var link = links.get(0);
assertEquals(commitIcon, link.iconUrl().orElseThrow());
assertEquals("Commit", link.title().orElseThrow());
assertEquals("Commit(master)", link.title().orElseThrow());
assertEquals(repo.webUrl(editHash), link.uri().orElseThrow());

// Wipe the history
Expand Down Expand Up @@ -1662,7 +1662,7 @@ void testIssueIdempotenceOldUrlFormat(TestInfo testInfo) throws IOException {

// Add a comment for the fix with the old url hash format
var lastCommit = localRepo.commits().stream().findFirst().orElseThrow();
issue.addComment(CommitFormatters.toTextBrief(repo, lastCommit).replace(editHash.toString(), editHash.abbreviate()));
issue.addComment(CommitFormatters.toTextBrief(repo, lastCommit, new Branch("master")).replace(editHash.toString(), editHash.abbreviate()));
TestBotRunner.runPeriodicItems(notifyBot);

// Verify that the planted comment is still the only one
Expand Down Expand Up @@ -2225,7 +2225,7 @@ void testFailedIssue(TestInfo testInfo) throws IOException {
assertEquals(1, links.size());
var link = links.get(0);
assertEquals(reviewIcon, link.iconUrl().orElseThrow());
assertEquals("Review", link.title().orElseThrow());
assertEquals("Review(master)", link.title().orElseThrow());
assertEquals(pr.webUrl(), link.uri().orElseThrow());

// Wipe the history
Expand Down Expand Up @@ -2498,4 +2498,78 @@ void testAvoidForwardportsShouldUseExistingBackport(TestInfo testInfo) throws IO
assertEquals(0, updatedBackport.labelNames().size());
}
}

@Test
void testTargetBranchUpdate(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var repo = credentials.getHostedRepository();
var repoFolder = tempFolder.path().resolve("repo");
var localRepo = CheckableRepository.init(repoFolder, repo.repositoryType());
credentials.commitLock(localRepo);
localRepo.pushAll(repo.authenticatedUrl());

var tagStorage = createTagStorage(repo);
var branchStorage = createBranchStorage(repo);
var prStateStorage = createPullRequestStateStorage(repo);
var storageFolder = tempFolder.path().resolve("storage");

var issueProject = credentials.getIssueProject();
var reviewIcon = URI.create("http://www.example.com/review.png");
var notifyBot = NotifyBot.newBuilder()
.repository(repo)
.storagePath(storageFolder)
.branches(Pattern.compile("master"))
.tagStorageBuilder(tagStorage)
.branchStorageBuilder(branchStorage)
.prStateStorageBuilder(prStateStorage)
.integratorId(repo.forge().currentUser().id())
.build();
var updater = IssueNotifier.newBuilder()
.issueProject(issueProject)
.reviewLink(true)
.reviewIcon(reviewIcon)
.build();
// Register a RepositoryListener to make history initialize on the first run
notifyBot.registerRepositoryListener(new NullRepositoryListener());
updater.attachTo(notifyBot);

// Initialize history
TestBotRunner.runPeriodicItems(notifyBot);

// Save the state
var historyState = localRepo.fetch(repo.authenticatedUrl(), "history").orElseThrow();

// Create an issue and commit a fix
var issue = issueProject.createIssue("This is an issue", List.of("Indeed"), Map.of("issuetype", JSON.of("Enhancement")));
var editHash = CheckableRepository.appendAndCommit(localRepo, "Another line", issue.id() + ": Fix that issue");
localRepo.push(editHash, repo.authenticatedUrl(), "master");
var pr = credentials.createPullRequest(repo, "master", "master", issue.id() + ": Fix that issue");
pr.addLabel("rfr");
pr.setBody("\n\n### Issue\n * [" + issue.id() + "](http://www.test.test/): The issue");
TestBotRunner.runPeriodicItems(notifyBot);

// There should be a review link
var links = issue.links();
assertEquals(1, links.size());
var link = links.get(0);
assertEquals(reviewIcon, link.iconUrl().orElseThrow());
assertEquals("Review(master)", link.title().orElseThrow());
assertEquals(pr.webUrl(), link.uri().orElseThrow());
assertTrue(issue.comments().getLast().body().contains("Branch: master"));

// Retarget the pr
pr.setTargetRef("jdk23");
TestBotRunner.runPeriodicItems(notifyBot);

// The review link should be updated
links = issue.links();
assertEquals(1, links.size());
link = links.get(0);
assertEquals(reviewIcon, link.iconUrl().orElseThrow());
assertEquals("Review(jdk23)", link.title().orElseThrow());
assertEquals(pr.webUrl(), link.uri().orElseThrow());
assertTrue(issue.comments().getLast().body().contains("Branch: jdk23"));
}
}
}
Loading

0 comments on commit 5835d99

Please sign in to comment.