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

411: Warn when a PR issue is not of a primary type #665

Closed
wants to merge 2 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
@@ -26,6 +26,7 @@
import org.openjdk.skara.forge.*;
import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.Comment;
import org.openjdk.skara.issuetracker.IssueProject;
import org.openjdk.skara.vcs.*;
import org.openjdk.skara.vcs.openjdk.Issue;

@@ -89,6 +90,41 @@ private boolean isTargetBranchAllowed() {
return matcher.matches();
}

private Set<String> allowedIssueTypes() {
return workItem.bot.allowedIssueTypes();
}

private List<Issue> issues() {
var issue = Issue.fromStringRelaxed(pr.title());
if (issue.isPresent()) {
var issues = new ArrayList<Issue>();
issues.add(issue.get());
issues.addAll(SolvesTracker.currentSolved(pr.repository().forge().currentUser(), comments));
return issues;
}
return List.of();
}

private IssueProject issueProject() {
return workItem.bot.issueProject();
}

private List<org.openjdk.skara.issuetracker.Issue> issuesOfDisallowedType() {
var issueProject = issueProject();
var allowed = allowedIssueTypes();
if (issueProject != null && allowed != null) {
return issues().stream()
.filter(i -> i.project().equals(Optional.of(issueProject.name())))
.map(i -> issueProject.issue(i.shortId()))
.filter(Optional::isPresent)
.map(Optional::get)
.filter(i -> i.properties().containsKey("issuetype"))
.filter(i -> !allowed.contains(i.properties().get("issuetype").asString()))
.collect(Collectors.toList());
}
return List.of();
}

private List<String> allowedTargetBranches() {
return pr.repository()
.branches()
@@ -135,6 +171,20 @@ private List<String> botSpecificChecks(Hash finalHash) throws IOException {
ret.add(error);
}

var disallowedIssues = issuesOfDisallowedType();
if (!disallowedIssues.isEmpty()) {
var s = disallowedIssues.size() > 1 ? "s " : " ";
var are = disallowedIssues.size() > 1 ? "are" : "is";
var links = disallowedIssues.stream()
.map(i -> "[" + i.id() + "](" + i.webUrl() + ")")
.collect(Collectors.toList());
var error = "The issue" + s + String.join(",", links) + " " + are + " not of the expected type. The allowed issue types are:\n" +
allowedIssueTypes().stream()
.map(name -> " - " + name)
.collect(Collectors.joining("\n"));
ret.add(error);
}

var headHash = pr.headHash();
var originalCommits = localRepo.commitMetadata(baseHash, headHash);

@@ -313,18 +363,15 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
progressBody.append(getAdditionalErrorsList(allAdditionalErrors));
}

var issue = Issue.fromStringRelaxed(pr.title());
var issueProject = workItem.bot.issueProject();
if (issueProject != null && issue.isPresent()) {
var allIssues = new ArrayList<Issue>();
allIssues.add(issue.get());
allIssues.addAll(SolvesTracker.currentSolved(pr.repository().forge().currentUser(), comments));
var issues = issues();
var issueProject = issueProject();
if (issueProject != null && !issues.isEmpty()) {
progressBody.append("\n\n### Issue");
if (allIssues.size() > 1) {
if (issues.size() > 1) {
progressBody.append("s");
}
progressBody.append("\n");
for (var currentIssue : allIssues) {
for (var currentIssue : issues) {
progressBody.append(" * ");
if (currentIssue.project().isPresent() && !currentIssue.project().get().equals(issueProject.name())) {
progressBody.append("⚠️ Issue `");
@@ -45,6 +45,7 @@ class PullRequestBot implements Bot {
private final Map<String, Pattern> readyComments;
private final IssueProject issueProject;
private final boolean ignoreStaleReviews;
private final Set<String> allowedIssueTypes;
private final Pattern allowedTargetBranches;
private final Path seedStorage;
private final ConcurrentMap<Hash, Boolean> currentLabels;
@@ -55,7 +56,7 @@ class PullRequestBot implements Bot {
LabelConfiguration labelConfiguration, Map<String, String> externalCommands,
Map<String, String> blockingCheckLabels, Set<String> readyLabels,
Map<String, Pattern> readyComments, IssueProject issueProject, boolean ignoreStaleReviews,
Pattern allowedTargetBranches, Path seedStorage) {
Set<String> allowedIssueTypes, Pattern allowedTargetBranches, Path seedStorage) {
remoteRepo = repo;
this.censusRepo = censusRepo;
this.censusRef = censusRef;
@@ -66,6 +67,7 @@ class PullRequestBot implements Bot {
this.issueProject = issueProject;
this.readyComments = readyComments;
this.ignoreStaleReviews = ignoreStaleReviews;
this.allowedIssueTypes = allowedIssueTypes;
this.allowedTargetBranches = allowedTargetBranches;
this.seedStorage = seedStorage;

@@ -185,6 +187,10 @@ boolean ignoreStaleReviews() {
return ignoreStaleReviews;
}

Set<String> allowedIssueTypes() {
return allowedIssueTypes;
}

Pattern allowedTargetBranches() {
return allowedTargetBranches;
}
@@ -40,6 +40,7 @@ public class PullRequestBotBuilder {
private Map<String, Pattern> readyComments = Map.of();
private IssueProject issueProject = null;
private boolean ignoreStaleReviews = false;
private Set<String> allowedIssueTypes = null;
private Pattern allowedTargetBranches = Pattern.compile(".*");
private Path seedStorage = null;

@@ -96,6 +97,11 @@ public PullRequestBotBuilder ignoreStaleReviews(boolean ignoreStaleReviews) {
return this;
}

public PullRequestBotBuilder allowedIssueTypes(Set<String> allowedIssueTypes) {
this.allowedIssueTypes = allowedIssueTypes;
return this;
}

public PullRequestBotBuilder allowedTargetBranches(String allowedTargetBranches) {
this.allowedTargetBranches = Pattern.compile(allowedTargetBranches);
return this;
@@ -107,8 +113,9 @@ public PullRequestBotBuilder seedStorage(Path seedStorage) {
}

public PullRequestBot build() {
return new PullRequestBot(repo, censusRepo, censusRef, labelConfiguration, externalCommands, blockingCheckLabels,
readyLabels, readyComments, issueProject, ignoreStaleReviews, allowedTargetBranches,
return new PullRequestBot(repo, censusRepo, censusRef, labelConfiguration, externalCommands,
blockingCheckLabels, readyLabels, readyComments, issueProject,
ignoreStaleReviews, allowedIssueTypes, allowedTargetBranches,
seedStorage);
}
}
}
@@ -118,6 +118,12 @@ public List<Bot> create(BotConfiguration configuration) {
if (repo.value().contains("ignorestale")) {
botBuilder.ignoreStaleReviews(repo.value().get("ignorestale").asBoolean());
}
if (repo.value().contains("issuetypes")) {
var types = repo.value().get("issuetypes").stream()
.map(JSONValue::asString)
.collect(Collectors.toSet());
botBuilder.allowedIssueTypes(types);
}
if (repo.value().contains("targetbranches")) {
botBuilder.allowedTargetBranches(repo.value().get("targetbranches").asString());
}
@@ -24,6 +24,7 @@

import org.junit.jupiter.api.*;
import org.openjdk.skara.forge.*;
import org.openjdk.skara.json.JSON;
import org.openjdk.skara.test.*;

import java.io.IOException;
@@ -1384,4 +1385,67 @@ void targetBranchPattern(TestInfo testInfo) throws IOException {
}
}

@Test
void allowedIssueTypes(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var author = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();
var issues = credentials.getIssueProject();

var censusBuilder = credentials.getCensusBuilder()
.addAuthor(author.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id());
var checkBot = PullRequestBot.newBuilder().repo(author).censusRepo(censusBuilder.build())
.allowedIssueTypes(Set.of("Bug"))
.issueProject(issues)
.build();

var bug = issues.createIssue("My first bug", List.of("A bug"),
Map.of("issuetype", JSON.of("Bug")));
var feature = issues.createIssue("My first feature", List.of("A feature"),
Map.of("issuetype", JSON.of("Enhancement")));

// 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);

// Make a change with a corresponding PR
var bugHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(bugHash, author.url(), "bug", true);
var bugPR = credentials.createPullRequest(author, "master", "bug",
bug.id() + ": My first bug", true);

// Check the status
TestBotRunner.runPeriodicItems(checkBot);

// Verify that the check passed
var bugChecks = bugPR.checks(bugHash);
assertEquals(1, bugChecks.size());
var bugCheck = bugChecks.get("jcheck");
assertEquals(CheckStatus.SUCCESS, bugCheck.status());

// Make a change with a corresponding PR
var featureHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(featureHash, author.url(), "feature", true);
var featurePR = credentials.createPullRequest(author, "master", "feature",
feature.id() + ": My first feature", true);

// Check the status
TestBotRunner.runPeriodicItems(checkBot);

// Verify that the check failed for the feature PR
var featureChecks = featurePR.checks(featureHash);
assertEquals(1, featureChecks.size());
var featureCheck = featureChecks.get("jcheck");
assertEquals(CheckStatus.FAILURE, featureCheck.status());
var link = "[" + feature.id() + "](" + feature.webUrl() + ")";
assertTrue(featureCheck.summary()
.orElseThrow()
.contains("The issue " + link + " is not of the expected type. " +
"The allowed issue types are:\n" +
" - Bug\n"));
}
}
}