Skip to content
Permalink
Browse files
542: Adjust label (cc) command logic
Reviewed-by: ehelin
  • Loading branch information
rwestberg committed Aug 28, 2020
1 parent 8a2d11c commit aca3439640f72b6c3a6da5b8789ce10cbe7bd6d1
Showing 8 changed files with 192 additions and 42 deletions.
@@ -213,7 +213,8 @@ public Collection<WorkItem> run(Path scratchPath) {
var nextCommand = nextCommand(pr, comments);
if (nextCommand.isEmpty()) {
log.info("No new non-external PR commands found, stopping further processing");
return List.of();
// When all commands are processed, it's time to check labels
return List.of(new LabelerWorkItem(bot, pr, errorHandler));
}

var census = CensusInstance.create(bot.censusRepo(), bot.censusRef(), scratchPath.resolve("census"), pr);
@@ -49,15 +49,6 @@ private void showHelp(LabelConfiguration labelConfiguration, PrintWriter reply)
labelConfiguration.allowed().forEach(label -> reply.println(" * `" + label + "`"));
}

private Set<String> automaticLabels(PullRequestBot bot, PullRequest pr, Path scratchPath) throws IOException {
var path = scratchPath.resolve("pr").resolve("labelcommand").resolve(pr.repository().name());
var seedPath = bot.seedStorage().orElse(scratchPath.resolve("seeds"));
var hostedRepositoryPool = new HostedRepositoryPool(seedPath);
var localRepo = PullRequestUtils.materialize(hostedRepositoryPool, pr, path);
var files = PullRequestUtils.changedFiles(pr, localRepo);
return bot.labelConfiguration().fromChanges(files);
}

@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()) && (!censusInstance.isCommitter(command.user()))) {
@@ -88,28 +79,21 @@ public void handle(PullRequestBot bot, PullRequest pr, CensusInstance censusInst
for (var label : labels) {
if (!currentLabels.contains(label)) {
pr.addLabel(label);
reply.println(LabelTracker.addLabelMarker(label));
reply.println("The `" + label + "` label was successfully added.");
} else {
reply.println("The `" + label + "` label was already applied.");
}
}
} else if (argumentMatcher.group(1).equals("remove")) {
try {
var automaticLabels = automaticLabels(bot, pr, scratchPath);
for (var label : labels) {
if (currentLabels.contains(label)) {
if (automaticLabels.contains(label)) {
reply.println("The `" + label + "` label was automatically added and cannot be removed.");
} else {
pr.removeLabel(label);
reply.println("The `" + label + "` label was successfully removed.");
}
} else {
reply.println("The `" + label + "` label was not set.");
}
for (var label : labels) {
if (currentLabels.contains(label)) {
pr.removeLabel(label);
reply.println(LabelTracker.removeLabelMarker(label));
reply.println("The `" + label + "` label was successfully removed.");
} else {
reply.println("The `" + label + "` label was not set.");
}
} catch (IOException e) {
reply.println("An error occurred when trying to check automatically added labels");
}
}
}
@@ -87,21 +87,22 @@ public Set<String> fromChanges(Set<Path> changes) {
}
}

// If the current labels matches at least two members of a group, the group is also included
var ret = new HashSet<>(labels);
// If the current labels matches at least two members of a group, use the group instead
for (var group : groups.entrySet()) {
var count = 0;
for (var groupEntry : group.getValue()) {
if (labels.contains(groupEntry)) {
if (ret.contains(groupEntry)) {
count++;
if (count == 2) {
labels.add(group.getKey());
ret.add(group.getKey());
ret.removeAll(group.getValue());
break;
}
}
}
}

return labels;
return ret;
}

public Set<String> allowed() {
@@ -0,0 +1,88 @@
/*
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
package org.openjdk.skara.bots.pr;

import org.openjdk.skara.host.HostUser;
import org.openjdk.skara.issuetracker.Comment;

import java.util.*;
import java.util.regex.*;
import java.util.stream.Collectors;

public class LabelTracker {
private final static String addMarker = "<!-- added label: '%s' -->";
private final static String removeMarker = "<!-- removed label: '%s' -->";
private final static Pattern labelMarkerPattern = Pattern.compile("<!-- (added|removed) label: '(.*?)' -->");

static String addLabelMarker(String label) {
return String.format(addMarker, label);
}

static String removeLabelMarker(String label) {
return String.format(removeMarker, label);
}

// Return all manually added labels, but filter any explicitly removed ones
static Set<String> currentAdded(HostUser botUser, List<Comment> comments) {
var labelActions = comments.stream()
.filter(comment -> comment.author().equals(botUser))
.flatMap(comment -> comment.body().lines())
.map(labelMarkerPattern::matcher)
.filter(Matcher::find)
.collect(Collectors.toList());

var ret = new HashSet<String>();
for (var actionMatch : labelActions) {
var action = actionMatch.group(1);
if (action.equals("added")) {
ret.add(actionMatch.group(2));
} else {
ret.remove(actionMatch.group(2));
}
}

return Collections.unmodifiableSet(ret);
}

// Return all manually removed labels, but filter any explicitly added ones
static Set<String> currentRemoved(HostUser botUser, List<Comment> comments) {
var labelActions = comments.stream()
.filter(comment -> comment.author().equals(botUser))
.flatMap(comment -> comment.body().lines())
.map(labelMarkerPattern::matcher)
.filter(Matcher::find)
.collect(Collectors.toList());

var ret = new HashSet<String>();
for (var actionMatch : labelActions) {
var action = actionMatch.group(1);
if (action.equals("removed")) {
ret.add(actionMatch.group(2));
} else {
ret.remove(actionMatch.group(2));
}
}

return Collections.unmodifiableSet(ret);
}
}
@@ -62,14 +62,26 @@ public Collection<WorkItem> run(Path scratchPath) {
.filter(key -> bot.labelConfiguration().allowed().contains(key))
.collect(Collectors.toSet());

// Add all labels not already set
var comments = pr.comments();
var manuallyAdded = LabelTracker.currentAdded(pr.repository().forge().currentUser(), comments);
var manuallyRemoved = LabelTracker.currentRemoved(pr.repository().forge().currentUser(), comments);

// If a manual label command has been issued before we have done any labeling,
// that is considered to be a request to override any automatic labelling
if (bot.currentLabels().isEmpty() && !(manuallyAdded.isEmpty() && manuallyRemoved.isEmpty())) {
return List.of();
}

// Add all labels not already set that are not manually removed
newLabels.stream()
.filter(label -> !currentLabels.contains(label))
.filter(label -> !manuallyRemoved.contains(label))
.forEach(pr::addLabel);

// Remove set labels no longer present
// Remove set labels no longer present unless it has been manually added
currentLabels.stream()
.filter(label -> !newLabels.contains(label))
.filter(label -> !manuallyAdded.contains(label))
.forEach(pr::removeLabel);

bot.currentLabels().put(pr.headHash(), Boolean.TRUE);
@@ -119,7 +119,6 @@ private List<WorkItem> getWorkItems(List<PullRequest> pullRequests) {
}

ret.add(new CheckWorkItem(this, pr, e -> updateCache.invalidate(pr)));
ret.add(new LabelerWorkItem(this, pr, e -> updateCache.invalidate(pr)));
}
}

@@ -57,6 +57,6 @@ void group() {

assertEquals(Set.of("1"), config.fromChanges(Set.of(Path.of("a.cpp"))));
assertEquals(Set.of("2"), config.fromChanges(Set.of(Path.of("a.hpp"))));
assertEquals(Set.of("1", "2", "both"), config.fromChanges(Set.of(Path.of("a.cpp"), Path.of("a.hpp"))));
assertEquals(Set.of("both"), config.fromChanges(Set.of(Path.of("a.cpp"), Path.of("a.hpp"))));
}
}
@@ -125,7 +125,7 @@ void simple(TestInfo testInfo) throws IOException {
}

@Test
void removeAutoApplied(TestInfo testInfo) throws IOException {
void adjustAutoApplied(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var author = credentials.getHostedRepository();
@@ -165,23 +165,88 @@ void removeAutoApplied(TestInfo testInfo) throws IOException {
// It will refuse to remove it
pr.addComment("/label remove 2");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "The `2` label was automatically added and cannot be removed.");
assertLastCommentContains(pr, "The `2` label was successfully removed.");

// Add another file to trigger a group match
Files.writeString(localRepoFolder.resolve("test.cpp"), "Hello there");
localRepo.add(Path.of("test.cpp"));
editHash = localRepo.commit("Another one", "duke", "duke@openjdk.org");
localRepo.push(editHash, author.url(), "edit");

// The bot should have applied more labels automatically
// The bot should have applied more labels automatically (but not the manually removed)
TestBotRunner.runPeriodicItems(prBot);
assertEquals(Set.of("1", "2", "group", "rfr"), new HashSet<>(pr.labels()));
assertEquals(Set.of("group", "rfr"), new HashSet<>(pr.labels()));

// Remove one of these as well
pr.addComment("/label remove group");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "The `group` label was successfully removed.");
assertEquals(Set.of("rfr"), new HashSet<>(pr.labels()));

// It will refuse to remove these as well
pr.addComment("/label remove group, 1");
// Adding them back again is fine
pr.addComment("/label add group 1");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "The `1` label was automatically added and cannot be removed.");
assertLastCommentContains(pr, "The `group` label was automatically added and cannot be removed.");
assertLastCommentContains(pr, "The `group` label was successfully added.");
assertLastCommentContains(pr, "The `1` label was successfully added.");
assertEquals(Set.of("1", "group", "rfr"), new HashSet<>(pr.labels()));
}
}

@Test
void overrideAutoApplied(TestInfo testInfo) throws IOException {
try (var credentials = new HostCredentials(testInfo);
var tempFolder = new TemporaryDirectory()) {
var author = credentials.getHostedRepository();
var integrator = credentials.getHostedRepository();

var censusBuilder = credentials.getCensusBuilder()
.addReviewer(integrator.forge().currentUser().id())
.addCommitter(author.forge().currentUser().id());
var labelConfiguration = LabelConfiguration.newBuilder()
.addMatchers("1", List.of(Pattern.compile("cpp$")))
.addMatchers("2", List.of(Pattern.compile("hpp$")))
.addGroup("group", List.of("1", "2"))
.addExtra("extra")
.build();
var prBot = PullRequestBot.newBuilder()
.repo(integrator)
.censusRepo(censusBuilder.build())
.labelConfiguration(labelConfiguration)
.build();

// Populate the projects repository
var localRepoFolder = tempFolder.path().resolve("localrepo");
var localRepo = CheckableRepository.init(localRepoFolder, author.repositoryType(), Path.of("test.hpp"));
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(), "edit", true);
var pr = credentials.createPullRequest(author, "master", "edit", "123: This is a pull request");
pr.setBody("/cc 1");

// The bot will not add any label automatically
TestBotRunner.runPeriodicItems(prBot);
assertEquals(Set.of("1", "rfr"), new HashSet<>(pr.labels()));

// Add another file to trigger a group match
Files.writeString(localRepoFolder.resolve("test.cpp"), "Hello there");
localRepo.add(Path.of("test.cpp"));
editHash = localRepo.commit("Another one", "duke", "duke@openjdk.org");
localRepo.push(editHash, author.url(), "edit");

// The bot will still not do any automatic labelling
TestBotRunner.runPeriodicItems(prBot);
assertEquals(Set.of("1", "rfr"), new HashSet<>(pr.labels()));

// Adding manually is still fine
pr.addComment("/label add group 2");
TestBotRunner.runPeriodicItems(prBot);
assertLastCommentContains(pr, "The `group` label was successfully added.");
assertLastCommentContains(pr, "The `2` label was successfully added.");
assertEquals(Set.of("1", "2", "group", "rfr"), new HashSet<>(pr.labels()));
}
}

1 comment on commit aca3439

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented on aca3439 Aug 28, 2020

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.