Skip to content

Commit

Permalink
983: Github commit links 404
Browse files Browse the repository at this point in the history 
Reviewed-by: tbell, rwestberg, ehelin, kcr
  • Loading branch information
@erikj79
erikj79 committed Apr 27, 2021
1 parent 4d271a0 commit 75a8a62
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 16 deletions.
6 changes: 4 additions & 2 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java
View file
Expand Up @@ -257,9 +257,11 @@ public void onNewCommits(HostedRepository repository, Repository localRepository

var existingComments = issue.comments();
var hashUrl = repository.webUrl(commit.hash()).toString();
// We used to store URLs with just the abbreviated hash, so need to check for both
var shortHashUrl = repository.webUrl(new Hash(commit.hash().abbreviate())).toString();
var alreadyPostedComment = existingComments.stream()
.filter(comment -> comment.author().equals(issueProject.issueTracker().currentUser()))
.anyMatch(comment -> comment.body().contains(hashUrl));
.filter(comment -> comment.author().equals(issueProject.issueTracker().currentUser()))
.anyMatch(comment -> comment.body().contains(hashUrl) || comment.body().contains(shortHashUrl));
if (!alreadyPostedComment) {
issue.addComment(commitNotification);
}
Expand Down
85 changes: 73 additions & 12 deletions bots/notify/src/test/java/org/openjdk/skara/bots/notify/issue/IssueNotifierTests.java
View file
Expand Up @@ -23,6 +23,7 @@
package org.openjdk.skara.bots.notify.issue;

import org.junit.jupiter.api.*;
import org.openjdk.skara.bots.notify.CommitFormatters;
import org.openjdk.skara.bots.notify.NotifyBot;
import org.openjdk.skara.forge.HostedRepository;
import org.openjdk.skara.issuetracker.*;
Expand Down Expand Up @@ -415,15 +416,15 @@ void testMultipleIssues(TestInfo testInfo) throws IOException {
var comments1 = updatedIssue1.comments();
assertEquals(1, comments1.size());
var comment1 = comments1.get(0);
assertTrue(comment1.body().contains(editHash.abbreviate()));
assertTrue(comment1.body().contains(editHash.toString()));
var comments2 = updatedIssue2.comments();
assertEquals(1, comments2.size());
var comment2 = comments2.get(0);
assertTrue(comment2.body().contains(editHash.abbreviate()));
assertTrue(comment2.body().contains(editHash.toString()));
var comments3 = updatedIssue3.comments();
assertEquals(1, comments3.size());
var comment3 = comments3.get(0);
assertTrue(comment3.body().contains(editHash.abbreviate()));
assertTrue(comment3.body().contains(editHash.toString()));

// As well as a fixVersion and a resolved in build
assertEquals(Set.of("0.1"), fixVersions(updatedIssue1));
Expand Down Expand Up @@ -472,7 +473,7 @@ void testIssue(TestInfo testInfo) throws IOException {
var comments = updatedIssue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion and a resolved in build
assertEquals(Set.of("0.1"), fixVersions(updatedIssue));
Expand Down Expand Up @@ -530,7 +531,7 @@ void testIssueBuildAfterMerge(TestInfo testInfo) throws IOException {
var comments = updatedIssue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion and a resolved in build
assertEquals(Set.of("0.1"), fixVersions(updatedIssue));
Expand Down Expand Up @@ -618,7 +619,7 @@ void testIssueBuildAfterTag(TestInfo testInfo) throws IOException {
var comments = updatedIssue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion and a resolved in build
assertEquals(Set.of("0.1"), fixVersions(updatedIssue));
Expand Down Expand Up @@ -699,12 +700,12 @@ void testIssueBuildAfterTagMultipleBranches(TestInfo testInfo) throws IOExceptio
var comments = updatedIssue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

var backportComments = backportIssue.comments();
assertEquals(1, backportComments.size());
var backportComment = backportComments.get(0);
assertTrue(backportComment.body().contains(editHash.abbreviate()));
assertTrue(backportComment.body().contains(editHash.toString()));

// As well as a fixVersion and a resolved in build
assertEquals(Set.of("16.0.2"), fixVersions(updatedIssue));
Expand Down Expand Up @@ -789,7 +790,7 @@ void testIssueRetryTag(TestInfo testInfo) throws IOException {
var comments = updatedIssue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion and a resolved in build
assertEquals(Set.of("0.1"), fixVersions(updatedIssue));
Expand Down Expand Up @@ -886,7 +887,7 @@ void testIssueOtherDomain(TestInfo testInfo) throws IOException {
var comments = updatedIssue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion
assertEquals(Set.of("0.1"), fixVersions(updatedIssue));
Expand Down Expand Up @@ -925,7 +926,7 @@ void testIssueNoVersion(TestInfo testInfo) throws IOException {
var comments = issue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// But not in the fixVersion
assertEquals(Set.of(), fixVersions(issue));
Expand Down Expand Up @@ -960,7 +961,7 @@ void testIssueConfiguredVersionNoCommit(TestInfo testInfo) throws IOException {
var comments = issue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.abbreviate()));
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion - but not the one from the repo
assertEquals(Set.of("2.0"), fixVersions(issue));
Expand Down Expand Up @@ -1002,6 +1003,66 @@ void testIssueIdempotence(TestInfo testInfo) throws IOException {
var comments = issue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
assertTrue(comment.body().contains(editHash.toString()));

// As well as a fixVersion
assertEquals(Set.of("0.1"), fixVersions(issue));

// Wipe the history
localRepo.push(historyState, repo.url(), "history", true);

// Run it again
TestBotRunner.runPeriodicItems(notifyBot);

// There should be no new comments or fixVersions
var updatedIssue = issueProject.issue(issue.id()).orElseThrow();
assertEquals(1, updatedIssue.comments().size());
assertEquals(Set.of("0.1"), fixVersions(updatedIssue));
}
}

/**
* The format used for commit URLs in bug comments and elsewhere was changed
* to use the full hash instead of an abbreviated hash. This test verifies
* that the idempotence of the IssueNotifier holds true even when
* encountering old bug comments containing the old commit URL format.
*/
@Test
void testIssueIdempotenceOldUrlFormat(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.url());

var storageFolder = tempFolder.path().resolve("storage");
var issueProject = credentials.getIssueProject();
var jbsNotifierConfig = JSON.object().put("fixversions", JSON.object());
var notifyBot = testBotBuilder(repo, issueProject, storageFolder, jbsNotifierConfig).create("notify", JSON.object());

// Initialize history
TestBotRunner.runPeriodicItems(notifyBot);

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

// 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.url(), "master");

// 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()));
TestBotRunner.runPeriodicItems(notifyBot);

// Verify that the planted comment is still the only one
var comments = issue.comments();
assertEquals(1, comments.size());
var comment = comments.get(0);
// We expect the abbreviated hash in the planted comment
assertTrue(comment.body().contains(editHash.abbreviate()));

// As well as a fixVersion
Expand Down
2 changes: 1 addition & 1 deletion forge/src/main/java/org/openjdk/skara/forge/github/GitHubRepository.java
View file
Expand Up @@ -208,7 +208,7 @@ public URI nonTransformedWebUrl() {

@Override
public URI webUrl(Hash hash) {
var endpoint = "/" + repository + "/commit/" + hash.abbreviate();
var endpoint = "/" + repository + "/commit/" + hash;
return gitHubHost.getWebURI(endpoint);
}

Expand Down
2 changes: 1 addition & 1 deletion forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabRepository.java
View file
Expand Up @@ -199,7 +199,7 @@ public URI nonTransformedWebUrl() {
@Override
public URI webUrl(Hash hash) {
return URIBuilder.base(gitLabHost.getUri())
.setPath("/" + projectName + "/commit/" + hash.abbreviate())
.setPath("/" + projectName + "/commit/" + hash)
.build();
}

Expand Down

1 comment on commit 75a8a62

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.