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

394: Merge bot conflict resolution interaction requires separate credentials #618

Closed
wants to merge 2 commits into from

Conversation

@rwestberg
Copy link
Member

@rwestberg rwestberg commented May 8, 2020

Hi all,

Please review this change that allows the pull request bot to recognize specially formatted command comments made by "itself" (most likely another bot using the same credentials).

Best regards,
Robin


Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Issue

  • SKARA-394: Merge bot conflict resolution interaction requires separate credentials

Reviewers

Download

$ git fetch https://git.openjdk.java.net/skara pull/618/head:pull/618
$ git checkout pull/618

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 8, 2020

👋 Welcome back rwestberg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 8, 2020

Webrevs

Copy link
Member

@JornVernee JornVernee left a comment

Looks good. Some minor comments left inline. The one about the test bot running early in the test might be the only problematic thing (but I don't think it matters in practice).

private final String commandReplyMarker = "<!-- Jmerge command reply message (%s) -->";
private final Pattern commandReplyPattern = Pattern.compile("<!-- Jmerge command reply message \\((\\S+)\\) -->");
private final String selfCommandMarker = "<!-- Valid self-command -->";
Copy link
Member

@JornVernee JornVernee May 8, 2020

FWIW, these fields could also be made static as well.

Copy link
Member Author

@rwestberg rwestberg May 11, 2020

Makes sense, will fix.

.map(comment -> new AbstractMap.SimpleEntry<>(comment, commandPattern.matcher(comment.body())))
.filter(entry -> !entry.getKey().author().equals(self) || entry.getKey().body().endsWith(selfCommandMarker))
Copy link
Member

@JornVernee JornVernee May 8, 2020

Could you move this above the .map(...) call, so that you can replace entry.getKey() with comment? (just like before)

Copy link
Member Author

@rwestberg rwestberg May 11, 2020

Ah, good catch, at first I intended to match the override marker with the regular expression, but it didn't end up that way, so can revert this.

// Issue an command using the bot account
var botPr = integrator.pullRequest(pr.id());
botPr.addComment("/help");
TestBotRunner.runPeriodicItems(mergeBot);

// The bot should not reply
assertEquals(1, pr.comments().size());
TestBotRunner.runPeriodicItems(mergeBot);
assertEquals(1, pr.comments().size());
Copy link
Member

@JornVernee JornVernee May 8, 2020

Should to runPeriodicItems call right after adding the "/help" comment be there? Asking because it looks like you want to check the number of comments before and after running the bot below that (but at that point the bot has already run).

Copy link
Member Author

@rwestberg rwestberg May 11, 2020

Right, I put the asserts around the bot execution to make it clear that the number doesn't change, but the first invocation certainly obfuscates that.. Will remove it.

var help = pr.comments().stream()
.filter(comment -> comment.body().contains("Available commands"))
.filter(comment -> comment.body().contains("help"))
.count();
Copy link
Member

@JornVernee JornVernee May 8, 2020

Spurious whitespace on last line

Suggested change
var help = pr.comments().stream()
.filter(comment -> comment.body().contains("Available commands"))
.filter(comment -> comment.body().contains("help"))
.count();
var help = pr.comments().stream()
.filter(comment -> comment.body().contains("Available commands"))
.filter(comment -> comment.body().contains("help"))
.count();

Copy link
Member Author

@rwestberg rwestberg May 11, 2020

Will fix.

@openjdk
Copy link

@openjdk openjdk bot commented May 8, 2020

@rwestberg This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

394: Merge bot conflict resolution interaction requires separate credentials

Reviewed-by: jvernee
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

There are currently no new commits on the master branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate eb9b6b1e81be1181182d39b59b1e1aee579307ff.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 8, 2020
@rwestberg
Copy link
Member Author

@rwestberg rwestberg commented May 11, 2020

/integrate

@openjdk openjdk bot closed this May 11, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels May 11, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 11, 2020

@rwestberg
Pushed as commit 9573e79.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants