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

1079: Support ability to defer and delegate timing of an integration #1221

Closed
wants to merge 3 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
@@ -42,8 +42,15 @@ public class IntegrateCommand implements CommandHandler {
private static final String PRE_PUSH_MARKER = "<!-- prepush %s -->";
private static final Pattern PRE_PUSH_PATTERN = Pattern.compile("<!-- prepush ([0-9a-z]{40}) -->");

private enum Command {
auto,
manual,
defer,
undefer
}

private void showHelp(PrintWriter reply) {
reply.println("usage: `/integrate [auto|manual|<hash>]`");
reply.println("usage: `/integrate [auto|manual|defer|undefer|<hash>]`");
}

private Optional<String> checkProblem(Map<String, Check> performedChecks, String checkName, PullRequest pr) {
@@ -66,22 +73,9 @@ private Optional<String> checkProblem(Map<String, Check> performedChecks, String

@Override
public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInstance, Path scratchPath, CommandInvocation command, List<Comment> allComments, PrintWriter reply) {
if (!command.user().equals(pr.author()) && !command.user().equals(pr.repository().forge().currentUser())) {
reply.print("Only the author (@" + pr.author().username() + ") is allowed to issue the `integrate` command.");

// If the command author is allowed to sponsor this change, suggest that command
var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments);
if (readyHash.isPresent()) {
if (censusInstance.isCommitter(command.user())) {
reply.print(" As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the `/sponsor` command?");
return;
}
}
reply.println();
return;
}

// Parse any argument given
Hash targetHash = null;
Command commandArg = null;
if (!command.args().isEmpty()) {
var args = command.args().split(" ");
if (args.length != 1) {
@@ -90,19 +84,12 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

var arg = args[0].trim();
if (arg.equals("auto")) {
pr.addLabel("auto");
reply.println("This pull request will be automatically integrated when it is ready");
return;
} else if (arg.equals("manual")) {
if (pr.labelNames().contains("auto")) {
pr.removeLabel("auto");
for (Command value : Command.values()) {
if (value.name().equals(arg)) {
commandArg = value;
}
reply.println("This pull request will have to be integrated manually using the "+
"[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command.");
return;
} else {
// Validate the target hash if requested
}
if (commandArg == null) {
targetHash = new Hash(arg);
if (!targetHash.isValid()) {
reply.println("The given argument, `" + arg + "`, is not a valid hash.");
@@ -111,6 +98,60 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}
}

if (!command.user().equals(pr.author()) && !command.user().equals(pr.repository().forge().currentUser())) {
if (pr.labelNames().contains("deferred")) {
// Check that the command user is a committer
if (!censusInstance.isCommitter(command.user())) {
reply.print("Only project committers are allowed to issue the `integrate` command on a deferred pull request.");
return;
}
// Check that no extra arguments are added
if (!command.args().isEmpty()) {
reply.print("Only the author (@\" + pr.author().username() + \") is allowed to issue the `integrate` command with arguments.");
return;
}
} else {
reply.print("Only the author (@" + pr.author().username() + ") is allowed to issue the `integrate` command.");

// If the command author is allowed to sponsor this change, suggest that command
var readyHash = ReadyForSponsorTracker.latestReadyForSponsor(pr.repository().forge().currentUser(), allComments);
if (readyHash.isPresent()) {
if (censusInstance.isCommitter(command.user())) {
reply.print(" As this pull request is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the `/sponsor` command?");
return;
}
}
reply.println();
return;
}
}

if (commandArg == Command.auto) {
pr.addLabel("auto");
reply.println("This pull request will be automatically integrated when it is ready");
return;
} else if (commandArg == Command.manual) {
if (pr.labelNames().contains("auto")) {
pr.removeLabel("auto");
}
reply.println("This pull request will have to be integrated manually using the " +
"[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command.");
return;
} else if (commandArg == Command.defer) {
pr.addLabel("deferred");
reply.println("Integration of this pull request has been deferred and may be completed by any project committer using the " +
"[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command.");
return;
} else if (commandArg == Command.undefer) {
if (pr.labelNames().contains("deferred")) {
reply.println("Integration of this pull request is no longer deferred and may only be integrated by the author (@" + pr.author().username() + ")using the " +
"[/integrate](https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/integrate) pull request command.");
pr.removeLabel("deferred");
}
reply.println("This pull request may now only be integrated by the author");
return;
}

Optional<Hash> prepushHash = checkForPrePushHash(bot, pr, scratchPath, allComments, "integrate");
if (prepushHash.isPresent()) {
markIntegratedAndClosed(pr, prepushHash.get(), reply);
@@ -126,7 +167,7 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst

var labels = new HashSet<>(pr.labelNames());
if (!labels.contains("ready")) {
reply.println("This PR has not yet been marked as ready for integration.");
reply.println("This pull request has not yet been marked as ready for integration.");
return;
}

@@ -163,7 +204,14 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
}

var original = checkablePr.findOriginalBackportHash();
var localHash = checkablePr.commit(rebasedHash.get(), censusInstance.namespace(), censusInstance.configuration().census().domain(), null, original);
// If someone other than the author or the bot issued the /integrate command, then that person
// should be set as sponsor/integrator. Otherwise pass null to use the default author.
String committerId = null;
if (!command.user().equals(pr.author()) && !command.user().equals(pr.repository().forge().currentUser())) {
committerId = command.user().id();
}
var localHash = checkablePr.commit(rebasedHash.get(), censusInstance.namespace(),
censusInstance.configuration().census().domain(), committerId, original);

if (runJcheck(pr, censusInstance, allComments, reply, localRepo, checkablePr, localHash)) {
return;
@@ -274,6 +322,9 @@ static void markIntegratedAndClosed(PullRequest pr, Hash hash, PrintWriter reply
pr.addLabel("integrated");
pr.removeLabel("ready");
pr.removeLabel("rfr");
if (pr.labelNames().contains("deferred")) {
pr.removeLabel("deferred");
}
reply.println("Pushed as commit " + hash.hex() + ".");
reply.println();
reply.println(":bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.");
@@ -44,14 +44,13 @@ void simpleMerge(TestInfo testInfo) throws IOException {
var tempFolder = new TemporaryDirectory();
var pushedFolder = new TemporaryDirectory()) {

var botUser = credentials.getHostedRepository();
var author = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();
var censusBuilder = credentials.getCensusBuilder()
.addCommitter(author.forge().currentUser().id())
.addReviewer(integrator.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id());
var mergeBot = PullRequestBot.newBuilder().repo(integrator).censusRepo(censusBuilder.build()).build();
var mergeBot = PullRequestBot.newBuilder().repo(botUser).censusRepo(censusBuilder.build()).build();

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
@@ -65,8 +64,8 @@ void simpleMerge(TestInfo testInfo) throws IOException {
var pr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Approve it as another user
var approvalPr = integrator.pullRequest(pr.id());
approvalPr.addReview(Review.Verdict.APPROVED, "Approved");
var reviewerPr = reviewer.pullRequest(pr.id());
reviewerPr.addReview(Review.Verdict.APPROVED, "Approved");

// The bot should reply with integration message
TestBotRunner.runPeriodicItems(mergeBot);
@@ -234,7 +233,7 @@ void notReviewed(TestInfo testInfo) throws IOException {
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should reply with an error message
assertLastCommentContains(pr, "PR has not yet been marked as ready for integration");
assertLastCommentContains(pr, "pull request has not yet been marked as ready for integration");
}
}

@@ -651,7 +650,7 @@ void cannotRebase(TestInfo testInfo) throws IOException {

// The bot should reply with an error message
TestBotRunner.runPeriodicItems(mergeBot);
assertLastCommentContains(pr, "PR has not yet been marked as ready for integration");
assertLastCommentContains(pr, "pull request has not yet been marked as ready for integration");
}
}

@@ -1296,4 +1295,113 @@ void retryAfterInterruptExtraChange(TestInfo testInfo) throws IOException {
assertTrue(pr.labelNames().contains("integrated"), "integrated label not added");
}
}

@Test
void defer(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory();
var pushedFolder = new TemporaryDirectory()) {

var botUser = credentials.getHostedRepository();
var author = credentials.getHostedRepository();
var reviewer = credentials.getHostedRepository();
var badIntegrator = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();
var censusBuilder = credentials.getCensusBuilder()
.addCommitter(author.forge().currentUser().id())
.addReviewer(reviewer.forge().currentUser().id())
.addAuthor(badIntegrator.forge().currentUser().id())
.addCommitter(integrator.forge().currentUser().id());
var mergeBot = PullRequestBot.newBuilder().repo(botUser).censusRepo(censusBuilder.build()).build();

// Populate the projects repository
var localRepo = CheckableRepository.init(tempFolder.path(), author.repositoryType());
var masterHash = localRepo.resolve("master").orElseThrow();
assertFalse(CheckableRepository.hasBeenEdited(localRepo));
localRepo.push(masterHash, author.url(), "master", true);

// Make a change with a corresponding PR
var editHash = CheckableRepository.appendAndCommit(localRepo);
localRepo.push(editHash, author.url(), "refs/heads/edit", true);
var authorPr = credentials.createPullRequest(author, "master", "edit", "This is a pull request");

// Approve it as another user
var reviewerPr = reviewer.pullRequest(authorPr.id());
reviewerPr.addReview(Review.Verdict.APPROVED, "Approved");

// Issue /integrate defer command and verify the PR gets deferred
authorPr.addComment("/integrate defer");
TestBotRunner.runPeriodicItems(mergeBot);
var deferred = authorPr.comments().stream()
.filter(comment -> comment.body().contains("Integration of this pull request has been deferred"))
.count();
assertEquals(1, deferred, "Missing deferred message");
assertTrue(authorPr.labelNames().contains("deferred"));

// Try to integrate by non committer
var badIntegratorPr = badIntegrator.pullRequest(authorPr.id());
badIntegratorPr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);
var onlyCommitters = authorPr.comments().stream()
.filter(comment -> comment.body()
.contains("Only project committers are allowed to issue the `integrate` command on a deferred pull request."))
.count();
assertEquals(1, onlyCommitters, "Missing error about only committers can integrate");

// Issue /integrate undefer and verify the PR is no longer deferred
authorPr.addComment("/integrate undefer");
TestBotRunner.runPeriodicItems(mergeBot);
var undeferred = authorPr.comments().stream()
.filter(comment -> comment.body().contains("Integration of this pull request is no longer deferred and may only be integrated by the author"))
.count();
assertEquals(1, undeferred, "Missing undeferred message");
assertFalse(authorPr.labelNames().contains("deferred"));

erikj79 marked this conversation as resolved.
Show resolved Hide resolved
// Try integrating as another committer, which should fail since the PR is currently not deferred
var integratorPr = integrator.pullRequest(authorPr.id());
integratorPr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);
var nonAuthor = authorPr.comments().stream()
.filter(comment -> comment.body().contains("Only the author")
&& comment.body().contains("is allowed to issue the `integrate` command"))
.count();
assertEquals(1, nonAuthor, "Missing only author can integrate message");

// Defer again
authorPr.addComment("/integrate defer");
TestBotRunner.runPeriodicItems(mergeBot);
assertTrue(authorPr.labelNames().contains("deferred"));

// Try to issue /integrate with an invalid command for a non author
integratorPr.addComment("/integrate auto");
TestBotRunner.runPeriodicItems(mergeBot);
var invalid = authorPr.comments().stream()
.filter(comment -> comment.body().contains("Only the author"))
.count();
assertEquals(2, invalid, "Missing error message");

// Try to integrate by committer
integratorPr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);

// The change should now be present on the master branch
var pushedRepo = Repository.materialize(pushedFolder.path(), author.url(), "master");
assertTrue(CheckableRepository.hasBeenEdited(pushedRepo));

var headHash = pushedRepo.resolve("HEAD").orElseThrow();
var headCommit = pushedRepo.commits(headHash.hex() + "^.." + headHash.hex()).asList().get(0);

// Verify that the author and committer of the change are the correct users
// The number is implied from the order the add* methods of CensusBuilder were called above.
assertEquals("Generated Committer 1", headCommit.author().name());
assertEquals("integrationcommitter1@openjdk.java.net", headCommit.author().email());
assertEquals("Generated Committer 4", headCommit.committer().name());
assertEquals("integrationcommitter4@openjdk.java.net", headCommit.committer().email());
assertTrue(authorPr.labelNames().contains("integrated"));

// Ready and deferred labels should have been removed
assertFalse(authorPr.labelNames().contains("ready"));
assertFalse(authorPr.labelNames().contains("deferred"));
}
}
}
@@ -92,7 +92,7 @@ void integrateFollowup(TestInfo testInfo) throws IOException {
// Try to integrate it
followUpPr.addComment("/integrate");
TestBotRunner.runPeriodicItems(mergeBot);
assertLastCommentContains(followUpPr, "This PR has not yet been marked as ready for integration");
assertLastCommentContains(followUpPr, "This pull request has not yet been marked as ready for integration");

// Push something unrelated to the target
localRepo.checkout(masterHash, true);
@@ -179,7 +179,7 @@ void noIntegration(TestInfo testInfo) throws IOException {
// It should not be possible to integrate yet
pr.addComment("/integrate");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(reviewerPr,"PR has not yet been marked as ready for integration");
assertLastCommentContains(reviewerPr,"pull request has not yet been marked as ready for integration");

// Relax the requirement
reviewerPr.addComment("/reviewers 1");
@@ -187,7 +187,8 @@ private static class TestCredentials implements Credentials {
HostUser.create(1, "user1", "User Number 1"),
HostUser.create(2, "user2", "User Number 2"),
HostUser.create(3, "user3", "User Number 3"),
HostUser.create(4, "user4", "User Number 4")
HostUser.create(4, "user4", "User Number 4"),
HostUser.create(5, "user5", "User Number 5")
);

private TestHost createHost(int userIndex) {