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

Refactor and cleanup #1185

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
@@ -78,7 +78,7 @@ private static Optional<Commit> conflictCommit(PullRequest pr, Repository localR
}

try {
localRepo.merge(PullRequestUtils.targetHash(pr, localRepo));
localRepo.merge(PullRequestUtils.targetHash(localRepo));
// No problem means no conflict
return Optional.empty();
} catch (IOException e) {
@@ -35,7 +35,6 @@
import java.util.*;
import java.util.logging.Logger;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.*;

class CheckRun {
@@ -62,7 +61,6 @@ class CheckRun {
private static final String emptyPrBodyMarker = "<!--\nReplace this text with a description of your pull request (also remove the surrounding HTML comment markers).\n" +
"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;

@@ -138,7 +136,7 @@ private List<String> allowedTargetBranches() {
}

// Additional bot-specific checks that are not handled by JCheck
private List<String> botSpecificChecks(Hash finalHash) throws IOException {
private List<String> botSpecificChecks() {
var ret = new ArrayList<String>();

if (bodyWithoutStatus().isBlank()) {
@@ -291,19 +289,10 @@ private boolean updateClean(Commit commit) {
}

private Optional<HostedCommit> backportedFrom() {
var botUser = pr.repository().forge().currentUser();
var backportLines = pr.comments()
.stream()
.filter(c -> c.author().equals(botUser))
.flatMap(c -> Stream.of(c.body().split("\n")))
.map(l -> BACKPORT_PATTERN.matcher(l))
.filter(Matcher::find)
.collect(Collectors.toList());
if (backportLines.isEmpty()) {
var hash = checkablePullRequest.findOriginalBackportHash();
if (hash == null) {
return Optional.empty();
}

var hash = new Hash(backportLines.get(0).group(1));
var commit = pr.repository().forge().search(hash);
if (commit.isEmpty()) {
throw new IllegalStateException("Backport comment for PR " + pr.id() + " contains bad hash: " + hash.hex());
@@ -339,7 +328,7 @@ private String formatReviewer(HostUser reviewer) {
ret.append(contributor.fullName().orElse(contributor.username()));
if (censusLink.isPresent()) {
ret.append("](");
ret.append(censusLink.get().toString());
ret.append(censusLink.get());
ret.append(")");
}
ret.append(" (@");
@@ -475,7 +464,6 @@ 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("](");
@@ -594,16 +582,11 @@ private String updateStatusMessage(String message) {
}

private String verdictToString(Review.Verdict verdict) {
switch (verdict) {
case APPROVED:
return "changes are approved";
case DISAPPROVED:
return "more changes needed";
case NONE:
return "comment added";
default:
throw new RuntimeException("Unknown verdict: " + verdict);
}
return switch (verdict) {
case APPROVED -> "changes are approved";
case DISAPPROVED -> "more changes needed";
case NONE -> "comment added";
};
}

private void updateReviewedMessages(List<Comment> comments, List<Review> reviews) {
@@ -636,7 +619,7 @@ private String getMergeReadyComment(String commitMessage, List<Review> reviews)

try {
var hasContributingFile =
!localRepo.files(PullRequestUtils.targetHash(pr, localRepo), Path.of("CONTRIBUTING.md")).isEmpty();
!localRepo.files(checkablePullRequest.targetHash(), Path.of("CONTRIBUTING.md")).isEmpty();
if (hasContributingFile) {
message.append("\n\nℹ️ This project also has non-automated pre-integration requirements. Please see the file ");
message.append("[CONTRIBUTING.md](https://github.com/");
@@ -724,7 +707,6 @@ private String getMergeReadyComment(String commitMessage, List<Review> reviews)

if (!censusInstance.isCommitter(pr.author())) {
message.append("\n");
var contributor = censusInstance.namespace().get(pr.author().id());
message.append("As you do not have [Committer](https://openjdk.java.net/bylaws#committer) status in ");
message.append("[this project](https://openjdk.java.net/census#");
message.append(censusInstance.project().name());
@@ -811,7 +793,7 @@ private void updateMergeReadyComment(boolean isReady, String commitMessage, List
}
}

private void addSourceBranchWarningComment(List<Comment> comments) throws IOException {
private void addSourceBranchWarningComment(List<Comment> comments) {
var existing = findComment(comments, sourceBranchWarningMarker);
if (existing.isPresent()) {
// Only add the comment once per PR
@@ -822,7 +804,7 @@ private void addSourceBranchWarningComment(List<Comment> comments) throws IOExce
"a branch with the same name as the source branch for this pull request (`" + branch + "`) " +
"is present in the [target repository](" + pr.repository().nonTransformedWebUrl() + "). " +
"If you eventually integrate this pull request then the branch `" + branch + "` " +
"in your [personal fork](" + pr.sourceRepository().get().nonTransformedWebUrl() + ") will diverge once you sync " +
"in your [personal fork](" + pr.sourceRepository().orElseThrow().nonTransformedWebUrl() + ") will diverge once you sync " +
"your personal fork with the upstream repository.\n" +
"\n" +
"To avoid this situation, create a new branch for your changes and reset the `" + branch + "` branch. " +
@@ -911,18 +893,18 @@ private void checkStatus() {
additionalErrors = List.of(e.getMessage());
localHash = baseHash;
}
PullRequestCheckIssueVisitor visitor = checkablePullRequest.createVisitor(localHash);
PullRequestCheckIssueVisitor visitor = checkablePullRequest.createVisitor();
if (localHash.equals(baseHash)) {
if (additionalErrors.isEmpty()) {
additionalErrors = List.of("This PR contains no changes");
}
} else if (localHash.equals(PullRequestUtils.targetHash(pr, localRepo))) {
} else if (localHash.equals(checkablePullRequest.targetHash())) {
additionalErrors = List.of("This PR only contains changes already present in the target");
} else {
// Determine current status
var additionalConfiguration = AdditionalConfiguration.get(localRepo, localHash, pr.repository().forge().currentUser(), comments);
checkablePullRequest.executeChecks(localHash, censusInstance, visitor, additionalConfiguration);
additionalErrors = botSpecificChecks(localHash);
additionalErrors = botSpecificChecks();
}
updateCheckBuilder(checkBuilder, visitor, additionalErrors);
updateReadyForReview(visitor, additionalErrors);
@@ -32,10 +32,14 @@

import java.io.*;
import java.util.*;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class CheckablePullRequest {
private static final Pattern BACKPORT_PATTERN = Pattern.compile("<!-- backport ([0-9a-z]{40}) -->");

private final PullRequest pr;
private final Repository localRepo;
private final boolean ignoreStaleReviews;
@@ -121,8 +125,6 @@ private String commitMessage(Hash head, List<Review> activeReviews, Namespace na
/**
* The Review list is in chronological order, the latest one from a particular reviewer is the
* one that is "active".
* @param allReviews
* @return
*/
static List<Review> filterActiveReviews(List<Review> allReviews) {
var reviewPerUser = new LinkedHashMap<HostUser, Review>();
@@ -142,7 +144,7 @@ Hash commit(Hash finalHead, Namespace namespace, String censusDomain, String spo
throw new CommitFailure("Merge PRs can only be created by known OpenJDK authors.");
}

var head = localRepo.lookup(pr.headHash()).get();
var head = localRepo.lookup(pr.headHash()).orElseThrow();
author = head.author();
} else {
author = new Author(contributor.fullName().orElseThrow(), contributor.username() + "@" + censusDomain);
@@ -172,8 +174,8 @@ Hash amendManualReviewers(Hash commit, Namespace namespace, Hash original) throw
}
}

PullRequestCheckIssueVisitor createVisitor(Hash localHash) throws IOException {
var checks = JCheck.checksFor(localRepo, PullRequestUtils.targetHash(pr, localRepo));
PullRequestCheckIssueVisitor createVisitor() throws IOException {
var checks = JCheck.checksFor(localRepo, targetHash());
return new PullRequestCheckIssueVisitor(checks);
}

@@ -182,10 +184,10 @@ void executeChecks(Hash localHash, CensusInstance censusInstance, PullRequestChe
if (confOverride != null) {
conf = JCheck.parseConfiguration(confOverride, additionalConfiguration);
} else {
conf = JCheck.parseConfiguration(localRepo, PullRequestUtils.targetHash(pr, localRepo), additionalConfiguration);
conf = JCheck.parseConfiguration(localRepo, targetHash(), additionalConfiguration);
}
if (conf.isEmpty()) {
throw new RuntimeException("Failed to parse jcheck configuration at: " + PullRequestUtils.targetHash(pr, localRepo) + " with extra: " + additionalConfiguration);
throw new RuntimeException("Failed to parse jcheck configuration at: " + targetHash() + " with extra: " + additionalConfiguration);
}
try (var issues = JCheck.check(localRepo, censusInstance.census(), CommitMessageParsers.v1, localHash,
conf.get())) {
@@ -201,8 +203,8 @@ List<CommitMetadata> divergingCommits() {

private List<CommitMetadata> divergingCommits(Hash commitHash) {
try {
var updatedBase = localRepo.mergeBase(PullRequestUtils.targetHash(pr, localRepo), commitHash);
return localRepo.commitMetadata(updatedBase, PullRequestUtils.targetHash(pr, localRepo));
var updatedBase = localRepo.mergeBase(targetHash(), commitHash);
return localRepo.commitMetadata(updatedBase, targetHash());
} catch (IOException e) {
throw new RuntimeException(e);
}
@@ -227,7 +229,7 @@ Optional<Hash> mergeTarget(PrintWriter reply) {
.forEach(c -> reply.println(" * " + c.hash().hex() + ": " + c.message().get(0)));
if (divergingCommits.size() > 10) {
try {
var baseHash = localRepo.mergeBase(PullRequestUtils.targetHash(pr, localRepo), pr.headHash());
var baseHash = localRepo.mergeBase(targetHash(), pr.headHash());
reply.println(" * ... and " + (divergingCommits.size() - 10) + " more: " +
pr.repository().webUrl(baseHash.hex(), pr.targetRef()));
} catch (IOException e) {
@@ -240,11 +242,11 @@ Optional<Hash> mergeTarget(PrintWriter reply) {
localRepo.checkout(pr.headHash(), true);
Hash hash;
try {
localRepo.merge(PullRequestUtils.targetHash(pr, localRepo));
localRepo.merge(targetHash());
hash = localRepo.commit("Automatic merge with latest target", "duke", "duke@openjdk.org");
} catch (IOException e) {
localRepo.abortMerge();
localRepo.rebase(PullRequestUtils.targetHash(pr, localRepo), "duke", "duke@openjdk.org");
localRepo.rebase(targetHash(), "duke", "duke@openjdk.org");
hash = localRepo.head();
}
reply.println();
@@ -263,4 +265,25 @@ Optional<Hash> mergeTarget(PrintWriter reply) {
}
}

Hash findOriginalBackportHash() {
var botUser = pr.repository().forge().currentUser();
var backportLines = pr.comments()
.stream()
.filter(c -> c.author().equals(botUser))
.flatMap(c -> Stream.of(c.body().split("\n")))
.map(BACKPORT_PATTERN::matcher)
.filter(Matcher::find)
.collect(Collectors.toList());
return backportLines.isEmpty() ? null : new Hash(backportLines.get(0).group(1));
}

// Lazily initiated
private Hash targetHash;

public Hash targetHash() throws IOException {
if (targetHash == null) {
targetHash = PullRequestUtils.targetHash(localRepo);
}
return targetHash;
}
}