Skip to content

Delete or Update Editorial bot comments when violations are fixed #546

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/io/quarkus/bot/CheckIssueEditorialRules.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class CheckIssueEditorialRules {
private static final Logger LOG = Logger.getLogger(CheckIssueEditorialRules.class);

private static final String ZULIP_URL = "https://quarkusio.zulipchat.com/";
public static final String ZULIP_WARNING = Strings.commentByBot(
public static final String ZULIP_WARNING = Strings.editorialCommentByBot(
"You added a link to a Zulip discussion, please make sure the description of the issue is comprehensive and doesn't require accessing Zulip");

@Inject
Expand Down
172 changes: 59 additions & 113 deletions src/main/java/io/quarkus/bot/CheckPullRequestEditorialRules.java
Original file line number Diff line number Diff line change
@@ -1,43 +1,42 @@
package io.quarkus.bot;

import static io.quarkus.bot.util.Strings.EDITORIAL_RULES_COMMENT_MARKER;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import java.util.regex.Pattern;
import java.util.Optional;
import java.util.StringJoiner;

import jakarta.inject.Inject;

import org.jboss.logging.Logger;
import org.kohsuke.github.GHEventPayload;
import org.kohsuke.github.GHIssueComment;
import org.kohsuke.github.GHPullRequest;

import io.quarkiverse.githubapp.ConfigFile;
import io.quarkiverse.githubapp.event.PullRequest;
import io.quarkus.bot.config.Feature;
import io.quarkus.bot.config.QuarkusGitHubBotConfig;
import io.quarkus.bot.config.QuarkusGitHubBotConfigFile;
import io.quarkus.bot.util.GHPullRequests;
import io.quarkus.bot.util.PullRequestFilesMatcher;
import io.quarkus.bot.util.Strings;
import io.quarkus.bot.violation.EditorialViolation;
import io.quarkus.bot.violation.ViolationDetectorManager;

class CheckPullRequestEditorialRules {

private static final Logger LOG = Logger.getLogger(CheckPullRequestEditorialRules.class);

private static final Pattern SPACE_PATTERN = Pattern.compile("\\s+");
private static final Pattern ISSUE_PATTERN = Pattern.compile("#[0-9]+");
private static final Pattern FIX_FEAT_CHORE = Pattern.compile("^(fix|chore|feat|docs|refactor)[(:].*");
public static final String HEADER = """
Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

private static final List<String> UPPER_CASE_EXCEPTIONS = Arrays.asList("gRPC");
private static final List<String> BOMS = List.of("bom/application/pom.xml");
private static final List<String> DOC_CHANGES = List.of("docs/src/main/asciidoc/", "README.md", "LICENSE",
"CONTRIBUTING.md");
""";

@Inject
QuarkusGitHubBotConfig quarkusBotConfig;
GitHubBotActions gitHubBotActions;

@Inject
ViolationDetectorManager violationManager;

void checkPullRequestEditorialRules(@PullRequest.Opened GHEventPayload.PullRequest pullRequestPayload,
@ConfigFile("quarkus-github-bot.yml") QuarkusGitHubBotConfigFile quarkusBotConfigFile) throws IOException {
Expand All @@ -48,122 +47,69 @@ void checkPullRequestEditorialRules(@PullRequest.Opened GHEventPayload.PullReque
String baseBranch = pullRequestPayload.getPullRequest().getBase().getRef();

GHPullRequest pullRequest = pullRequestPayload.getPullRequest();
String body = pullRequest.getBody();
normalizeTitle(pullRequest, baseBranch);

processViolations(pullRequest, false);
}

void checkPullRequestEditorialRulesOnEdit(@PullRequest.Edited GHEventPayload.PullRequest pullRequestPayload,
@ConfigFile("quarkus-github-bot.yml") QuarkusGitHubBotConfigFile quarkusBotConfigFile) throws IOException {
if (!Feature.CHECK_EDITORIAL_RULES.isEnabled(quarkusBotConfigFile)) {
return;
}
processViolations(pullRequestPayload.getPullRequest(), true);
}

private void normalizeTitle(GHPullRequest pullRequest, String baseBranch) {
String originalTitle = pullRequest.getTitle();
String normalizedTitle = GHPullRequests.normalizeTitle(originalTitle, baseBranch);

if (!originalTitle.equals(normalizedTitle)) {
pullRequest.setTitle(normalizedTitle);
gitHubBotActions.setPullRequestTitle(pullRequest, normalizedTitle);
}
}

// we remove the potential version prefix before checking the editorial rules
String title = GHPullRequests.dropVersionSuffix(normalizedTitle, baseBranch);

List<String> titleErrorMessages = getTitleErrorMessages(title);
List<String> bodyErrorMessages = getBodyErrorMessages(body, pullRequest);
private void processViolations(GHPullRequest pullRequest, boolean isEdit) throws IOException {
List<EditorialViolation> violations = violationManager.detectViolations(pullRequest);
List<String> titleErrorMessages = violationManager.getTitleViolationMessages(violations);
List<String> bodyErrorMessages = violationManager.getBodyViolationMessages(violations);

if (titleErrorMessages.isEmpty() && bodyErrorMessages.isEmpty()) {
// No violations - remove existing comment if this is an edit
if (isEdit) {
gitHubBotActions.findBotComment(pullRequest, EDITORIAL_RULES_COMMENT_MARKER)
.ifPresent(comment -> gitHubBotActions.deleteComment(comment, pullRequest.getNumber()));
}
return;
}

StringBuilder comment = new StringBuilder("""
Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?
String violationComment = buildViolationComment(titleErrorMessages, bodyErrorMessages);

""");
for (String errorMessage : titleErrorMessages) {
comment.append("- ").append(errorMessage).append("\n");
}
for (String errorMessage : bodyErrorMessages) {
comment.append("- ").append(errorMessage).append("\n");
}

if (!quarkusBotConfig.isDryRun()) {
pullRequest.comment(Strings.commentByBot(comment.toString()));
if (isEdit) {
processCommentUpdate(pullRequest, EDITORIAL_RULES_COMMENT_MARKER, violationComment, true);
} else {
LOG.info("Pull request #" + pullRequest.getNumber() + " - Add comment " + comment.toString());
}
}

private static List<String> getTitleErrorMessages(String title) {
List<String> errorMessages = new ArrayList<>();

if (title == null || title.isEmpty()) {
return Collections.singletonList("title should not be empty");
}

if (title.endsWith(".")) {
errorMessages.add("title should not end up with dot");
}
if (title.endsWith("…")) {
errorMessages.add("title should not end up with ellipsis (make sure the title is complete)");
}
if (SPACE_PATTERN.split(title.trim()).length < 2) {
errorMessages.add("title should count at least 2 words to describe the change properly");
}
if (!Character.isDigit(title.codePointAt(0)) && !Character.isUpperCase(title.codePointAt(0))
&& !isUpperCaseException(title)) {
errorMessages.add("title should preferably start with an uppercase character (if it makes sense!)");
gitHubBotActions.addComment(pullRequest, violationComment, true);
}
if (ISSUE_PATTERN.matcher(title).find()) {
errorMessages.add("title should not contain an issue number (use `Fix #1234` in the description instead)");
}
if (FIX_FEAT_CHORE.matcher(title).matches()) {
errorMessages.add("title should not start with chore/docs/feat/fix/refactor but be a proper sentence");
}

return errorMessages;
}

private static List<String> getBodyErrorMessages(String body, GHPullRequest pullRequest) throws IOException {
List<String> errorMessages = new ArrayList<>();

if ((body == null || body.isBlank()) && isMeaningfulPullRequest(pullRequest)) {
return List.of(
"description should not be empty, describe your intent or provide links to the issues this PR is fixing (using `Fixes #NNNNN`) or changelogs");
}

return errorMessages;
private String buildViolationComment(List<String> titleErrors, List<String> bodyErrors) {
StringJoiner comment = new StringJoiner("\n", HEADER, "");
titleErrors.forEach(error -> comment.add("- " + error));
bodyErrors.forEach(error -> comment.add("- " + error));
return comment.toString();
}

private static boolean isUpperCaseException(String title) {
for (String exception : UPPER_CASE_EXCEPTIONS) {
if (title.toLowerCase(Locale.ROOT).startsWith(exception.toLowerCase(Locale.ROOT))) {
return true;
public void processCommentUpdate(GHPullRequest pr, String marker, String newContent, boolean isEditorial) {
try {
Optional<GHIssueComment> existingComment = gitHubBotActions.findBotComment(pr, marker);
if (existingComment.isPresent()) {
gitHubBotActions.updateComment(existingComment.get(), newContent, pr.getNumber(), isEditorial);
} else {
gitHubBotActions.addComment(pr, newContent, isEditorial);
}
} catch (IOException e) {
LOG.error("Failed to process comment update for PR #" + pr.getNumber(), e);
}

return false;
}

private static boolean isMeaningfulPullRequest(GHPullRequest pullRequest) throws IOException {
// Note: these rules will have to be adjusted depending on how it goes
// we don't want to annoy people fixing a typo or require a description for a one liner explained in the title

// if we have more than one commit, then it's meaningful
if (pullRequest.getCommits() > 1) {
return true;
}

PullRequestFilesMatcher filesMatcher = new PullRequestFilesMatcher(pullRequest);

// for changes to the BOM, we are stricter
if (filesMatcher.changedFilesMatch(BOMS)) {
return true;
}

// for one liner/two liners, let's be a little more lenient
if (pullRequest.getAdditions() <= 2 && pullRequest.getDeletions() <= 2) {
return false;
}

// let's be a little more flexible for doc changes
if (filesMatcher.changedFilesOnlyMatch(DOC_CHANGES)
&& pullRequest.getAdditions() <= 10 && pullRequest.getDeletions() <= 10) {
return false;
}

return true;
}
}
78 changes: 78 additions & 0 deletions src/main/java/io/quarkus/bot/GHIssueCommentService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
package io.quarkus.bot;

import java.io.IOException;
import java.util.List;
import java.util.Optional;

import jakarta.inject.Singleton;

import org.jboss.logging.Logger;
import org.kohsuke.github.GHIssue;
import org.kohsuke.github.GHIssueComment;

import io.quarkus.bot.util.Strings;

@Singleton
public class GHIssueCommentService {
private static final Logger LOG = Logger.getLogger(GHIssueCommentService.class);

public void updateComment(GHIssueComment comment, String newText, int issueNumber, boolean isEditorialComment,
boolean isDryRun) {
if (!isDryRun) {
try {
String formattedComment = isEditorialComment ? Strings.editorialCommentByBot(newText)
: Strings.commentByBot(newText);
comment.update(formattedComment);
LOG.debug("Pull request #" + issueNumber + " - Updated comment");
} catch (IOException e) {
LOG.error(String.format("Pull Request #%s - Failed to update comment %d",
issueNumber, comment.getId()), e);
}
} else {
LOG.info(String.format("Pull Request #%s - Update comment %d with: %s",
issueNumber, comment.getId(), newText));
}
}

public void deleteComment(GHIssueComment comment, int issueNumber, boolean isDryRun) {
if (!isDryRun) {
try {
comment.delete();
LOG.debug("Pull request #" + issueNumber + " - Deleted comment");
} catch (IOException e) {
LOG.error(String.format("Pull Request #%s - Failed to delete comment %d",
issueNumber, comment.getId()), e);
}
} else {
LOG.info(String.format("Pull Request #%s - Delete comment %d",
issueNumber, comment.getId()));
}
}

public Optional<GHIssueComment> findBotCommentInIssue(GHIssue GhIssue, String marker) throws IOException {
List<GHIssueComment> comments = GhIssue.getComments();
if (comments == null || comments.isEmpty()) {
return Optional.empty();
}

return comments.stream()
.filter(comment -> comment.getBody() != null)
.filter(comment -> comment.getBody().contains(marker))
.findFirst();
}

public void addComment(GHIssue ghIssue, String comment, boolean isEditorialComment, boolean isDryRun) {
if (!isDryRun) {
try {
String formattedComment = isEditorialComment ? Strings.editorialCommentByBot(comment)
: Strings.commentByBot(comment);
ghIssue.comment(formattedComment);
LOG.debugf("Pull request #%d - Added new comment", ghIssue.getNumber());
} catch (IOException e) {
LOG.errorf(e, "Pull Request #%d - Failed to add comment", ghIssue.getNumber());
}
} else {
LOG.infof("Pull request #%d - Add comment (dry-run): %s", ghIssue.getNumber(), comment);
}
}
}
30 changes: 30 additions & 0 deletions src/main/java/io/quarkus/bot/GHIssueService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package io.quarkus.bot;

import java.io.IOException;

import jakarta.inject.Singleton;

import org.jboss.logging.Logger;
import org.kohsuke.github.GHIssue;

import io.quarkus.bot.util.Strings;

@Singleton
public class GHIssueService {

private static final Logger LOG = Logger.getLogger(GHIssueService.class);

public void setIssueTitle(GHIssue issue, String newTitle, boolean isDryRun) {
if (!isDryRun) {
try {
issue.setTitle(newTitle);
LOG.debugf("Pull request #%d - Updated title to: %s", issue.getNumber(), newTitle);
} catch (IOException e) {
LOG.errorf(e, "Pull Request #%d - Failed to update title", issue.getNumber());
}
} else {
LOG.infof("Pull request #%d - Update title to (dry-run): %s", issue.getNumber(), newTitle);
}
}

}
47 changes: 47 additions & 0 deletions src/main/java/io/quarkus/bot/GitHubBotActions.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package io.quarkus.bot;

import java.io.IOException;
import java.util.Optional;

import jakarta.inject.Inject;
import jakarta.inject.Singleton;

import org.kohsuke.github.GHIssueComment;
import org.kohsuke.github.GHPullRequest;

import io.quarkus.bot.config.QuarkusGitHubBotConfig;

/**
* This class handles all GitHub operations related to bot actions.
* It provides methods for adding, updating, and removing comments on pull requests,
* as well as setting pull request titles.
*/
@Singleton
class GitHubBotActions {
@Inject
QuarkusGitHubBotConfig quarkusBotConfig;
@Inject
GHIssueService issueService;
@Inject
GHIssueCommentService commentService;

public void addComment(GHPullRequest pullRequest, String comment, boolean isEditorialComment) {
commentService.addComment(pullRequest, comment, isEditorialComment, quarkusBotConfig.isDryRun());
}

public void updateComment(GHIssueComment comment, String newText, int pullRequestNumber, boolean isEditorialComment) {
commentService.updateComment(comment, newText, pullRequestNumber, isEditorialComment, quarkusBotConfig.isDryRun());
}

public void deleteComment(GHIssueComment comment, int pullRequestNumber) {
commentService.deleteComment(comment, pullRequestNumber, quarkusBotConfig.isDryRun());
}

public void setPullRequestTitle(GHPullRequest pullRequest, String newTitle) {
issueService.setIssueTitle(pullRequest, newTitle, quarkusBotConfig.isDryRun());
}

public Optional<GHIssueComment> findBotComment(GHPullRequest pullRequest, String marker) throws IOException {
return commentService.findBotCommentInIssue(pullRequest, marker);
}
}
Loading