Skip to content
Permalink
Browse files
768: Skara bot should prevent attempt to use JBS backport issue ID in PR
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Nov 5, 2020
1 parent 5045eeb commit 4d7471a4c2ee26f68562fb8a18ae878fae4090ed
Showing 5 changed files with 13 additions and 68 deletions.
@@ -62,6 +62,7 @@ class CheckRun {
"If in doubt, feel free to delete everything in this edit box first, the bot will restore the progress section as needed.\n-->";
private static final String fullNameWarningMarker = "<!-- PullRequestBot full name warning comment -->";
private static final Pattern BACKPORT_PATTERN = Pattern.compile("<!-- backport ([0-9a-z]{40}) -->");
private final static Set<String> primaryTypes = Set.of("Bug", "New Feature", "Enhancement", "Task", "Sub-task");
private final Set<String> newLabels;

private Duration expiresIn;
@@ -107,10 +108,6 @@ 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()) {
@@ -126,22 +123,6 @@ 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()
@@ -171,20 +152,6 @@ 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);
}

for (var blocker : workItem.bot.blockingCheckLabels().entrySet()) {
if (labels.contains(blocker.getKey())) {
ret.add(blocker.getValue());
@@ -577,6 +544,7 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
try {
var iss = issueProject.issue(currentIssue.shortId());
if (iss.isPresent()) {
var properties = iss.get().properties();
progressBody.append("[");
progressBody.append(iss.get().id());
progressBody.append("](");
@@ -593,6 +561,10 @@ private String getStatusMessage(List<Comment> comments, List<Review> reviews, Pu
}
continue;
}
if (properties.containsKey("issuetype") && !primaryTypes.contains(properties.get("issuetype").asString())) {
progressBody.append(" ⚠️ Unexpected issue type `").append(properties.get("issuetype").asString()).append("`.");
setExpiration(Duration.ofMinutes(10));
}
progressBody.append("\n");
} else {
progressBody.append("⚠️ Failed to retrieve information on issue `");
@@ -52,7 +52,6 @@ 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 HostedRepository confOverrideRepo;
@@ -73,7 +72,7 @@ class PullRequestBot implements Bot {
Map<String, String> blockingCheckLabels, Set<String> readyLabels,
Set<String> twoReviewersLabels, Set<String> twentyFourHoursLabels,
Map<String, Pattern> readyComments, IssueProject issueProject,
boolean ignoreStaleReviews, Set<String> allowedIssueTypes, Pattern allowedTargetBranches,
boolean ignoreStaleReviews, Pattern allowedTargetBranches,
Path seedStorage, HostedRepository confOverrideRepo, String confOverrideName,
String confOverrideRef, String censusLink, List<HostUser> commitCommandUsers) {
remoteRepo = repo;
@@ -88,7 +87,6 @@ class PullRequestBot implements Bot {
this.issueProject = issueProject;
this.readyComments = readyComments;
this.ignoreStaleReviews = ignoreStaleReviews;
this.allowedIssueTypes = allowedIssueTypes;
this.allowedTargetBranches = allowedTargetBranches;
this.seedStorage = seedStorage;
this.confOverrideRepo = confOverrideRepo;
@@ -265,10 +263,6 @@ boolean ignoreStaleReviews() {
return ignoreStaleReviews;
}

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

Pattern allowedTargetBranches() {
return allowedTargetBranches;
}
@@ -45,7 +45,6 @@ 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;
private HostedRepository confOverrideRepo = null;
@@ -117,11 +116,6 @@ 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;
@@ -160,7 +154,7 @@ public PullRequestBotBuilder commitCommandUsers(List<HostUser> commitCommandUser
public PullRequestBot build() {
return new PullRequestBot(repo, censusRepo, censusRef, labelConfiguration, externalCommands,
blockingCheckLabels, readyLabels, twoReviewersLabels, twentyFourHoursLabels,
readyComments, issueProject, ignoreStaleReviews, allowedIssueTypes,
readyComments, issueProject, ignoreStaleReviews,
allowedTargetBranches, seedStorage, confOverrideRepo, confOverrideName,
confOverrideRef, censusLink, commitCommandUsers);
}
@@ -118,12 +118,6 @@ 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());
}
@@ -1442,14 +1442,13 @@ void allowedIssueTypes(TestInfo testInfo) throws IOException {
.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")));
Map.of("issuetype", JSON.of("Backport")));

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
@@ -1479,18 +1478,10 @@ void allowedIssueTypes(TestInfo testInfo) throws IOException {

// 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"));
assertTrue(featurePR.body().contains(feature.id()));
assertTrue(featurePR.body().contains("My first feature"));
assertTrue(featurePR.body().contains("## Issue\n"));
assertTrue(featurePR.body().contains("Unexpected issue type"));
}
}

1 comment on commit 4d7471a

@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented on 4d7471a Nov 5, 2020

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.