Skip to content
Permalink
Browse files
1079: Support ability to defer and delegate timing of an integration
Reviewed-by: kcr, ihse
  • Loading branch information
erikj79 committed Sep 30, 2021
1 parent acda266 commit 620f889dfdc6d92ab5237be2088730c7012abed4
@@ -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"));

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

1 comment on commit 620f889

@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented on 620f889 Sep 30, 2021

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.