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

Jump to bottom

983: Github commit links 404 #1136

Closed
wants to merge 3 commits into from
Closed

983: Github commit links 404 #1136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4304997
Stop abbreviating commit hash in web links
erikj79 Apr 22, 2021
52e7b97
Add test
erikj79 Apr 23, 2021
acc9f28
Dummy change to trigger actions
erikj79 Apr 23, 2021
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
6 changes: 4 additions & 2 deletions bots/notify/src/main/java/org/openjdk/skara/bots/notify/issue/IssueNotifier.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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