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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d861a3e
SKARA-1079
erikj79 Sep 17, 2021
990ed8c
Adjusted for review comments
erikj79 Sep 21, 2021
ff720eb
Fix comment
erikj79 Sep 21, 2021
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
111 changes: 81 additions & 30 deletions bots/pr/src/main/java/org/openjdk/skara/bots/pr/IntegrateCommand.java
View file
Open in desktop
Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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.");
Expand All @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.");
Expand Down
112 changes: 105 additions & 7 deletions bots/pr/src/test/java/org/openjdk/skara/bots/pr/IntegrateTests.java
View file
Open in desktop
Expand Up @@ -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());
Expand All @@ -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);
Expand Down Expand Up @@ -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");
}
}

Expand Down Expand Up @@ -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");
}
}

Expand Down Expand Up @@ -1296,4 +1295,103 @@ 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
// 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
var integratorPr = integrator.pullRequest(authorPr.id());
integratorPr.addComment("/integrate auto");
TestBotRunner.runPeriodicItems(mergeBot);
var invalid = authorPr.comments().stream()
.filter(comment -> comment.body().contains("Only the author"))
.count();
assertEquals(1, 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);

// Author and committer should be the same
erikj79 marked this conversation as resolved.
Show resolved Hide resolved
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 label should have been removed
erikj79 marked this conversation as resolved.
Show resolved Hide resolved
assertFalse(authorPr.labelNames().contains("ready"));
assertFalse(authorPr.labelNames().contains("deferred"));
}
}
}
2 changes: 1 addition & 1 deletion bots/pr/src/test/java/org/openjdk/skara/bots/pr/PreIntegrateTests.java
View file
Open in desktop
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion bots/pr/src/test/java/org/openjdk/skara/bots/pr/ReviewersTests.java
View file
Open in desktop
Expand Up @@ -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");
Expand Down
3 changes: 2 additions & 1 deletion test/src/main/java/org/openjdk/skara/test/HostCredentials.java
View file
Open in desktop
Expand Up @@ -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) {
Expand Down