diff --git a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/State.java b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/State.java index e09a81689..4a2e0431d 100644 --- a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/State.java +++ b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/State.java @@ -23,8 +23,11 @@ package org.openjdk.skara.bots.tester; import org.openjdk.skara.forge.PullRequest; +import org.openjdk.skara.host.HostUser; import org.openjdk.skara.issuetracker.Comment; +import java.util.function.Predicate; + class State { private final Stage stage; private final Comment requested; @@ -77,7 +80,7 @@ Comment finished() { return finished; } - static State from(PullRequest pr, String approverGroupId) { + static State from(PullRequest pr, Predicate validate) { Comment requested = null; Comment pending = null; Comment approval = null; @@ -127,7 +130,7 @@ static State from(PullRequest pr, String approverGroupId) { } } else if (body.equals("/test approve")) { approval = comment; - if (host.isMemberOf(approverGroupId, author)) { + if (validate.test(author)) { isApproved = true; } } else if (body.equals("/test cancel")) { @@ -135,7 +138,7 @@ static State from(PullRequest pr, String approverGroupId) { cancelled = comment; } } else if (body.startsWith("/test")) { - if (host.isMemberOf(approverGroupId, author)) { + if (validate.test(author)) { isApproved = true; } } diff --git a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBot.java b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBot.java index 2331b4329..25b9415d3 100644 --- a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBot.java +++ b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBot.java @@ -47,6 +47,7 @@ private static class Observation { private final Logger log = Logger.getLogger("org.openjdk.skara.bots");; private final ContinuousIntegration ci; private final String approversGroupId; + private final Set allowlist; private final List availableJobs; private final List defaultJobs; private final String name; @@ -57,6 +58,7 @@ private static class Observation { TestBot(ContinuousIntegration ci, String approversGroupId, + Set allowlist, List availableJobs, List defaultJobs, String name, @@ -64,6 +66,7 @@ private static class Observation { HostedRepository repo) { this.ci = ci; this.approversGroupId = approversGroupId; + this.allowlist = allowlist; this.availableJobs = availableJobs; this.defaultJobs = defaultJobs; this.name = name; @@ -83,6 +86,7 @@ public List getPeriodicItems() { if (cache.needsUpdate(pr)) { ret.add(new TestWorkItem(ci, approversGroupId, + allowlist, availableJobs, defaultJobs, name, @@ -121,6 +125,7 @@ public List getPeriodicItems() { if (shouldUpdate) { ret.add(new TestWorkItem(ci, approversGroupId, + allowlist, availableJobs, defaultJobs, name, diff --git a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBotFactory.java b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBotFactory.java index 9ce76b6eb..ce9b108f3 100644 --- a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBotFactory.java +++ b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestBotFactory.java @@ -52,13 +52,14 @@ public List create(BotConfiguration configuration) { var specific = configuration.specific(); var approvers = specific.get("approvers").asString(); + var allowlist = specific.get("allowlist").stream().map(JSONValue::asString).collect(Collectors.toSet()); var availableJobs = specific.get("availableJobs").stream().map(JSONValue::asString).collect(Collectors.toList()); var defaultJobs = specific.get("defaultJobs").stream().map(JSONValue::asString).collect(Collectors.toList()); var name = specific.get("name").asString(); var ci = configuration.continuousIntegration(specific.get("ci").asString()); for (var repo : specific.get("repositories").asArray()) { var hostedRepo = configuration.repository(repo.asString()); - ret.add(new TestBot(ci, approvers, availableJobs, defaultJobs, name, storage, hostedRepo)); + ret.add(new TestBot(ci, approvers, allowlist, availableJobs, defaultJobs, name, storage, hostedRepo)); } return ret; diff --git a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestWorkItem.java b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestWorkItem.java index 565310c95..36a86111e 100644 --- a/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestWorkItem.java +++ b/bots/tester/src/main/java/org/openjdk/skara/bots/tester/TestWorkItem.java @@ -26,6 +26,7 @@ import org.openjdk.skara.ci.*; import org.openjdk.skara.forge.*; import org.openjdk.skara.vcs.*; +import org.openjdk.skara.host.HostUser; import java.io.*; import java.net.*; @@ -34,11 +35,13 @@ import java.util.*; import java.util.logging.Logger; import java.util.stream.*; +import java.util.function.Predicate; public class TestWorkItem implements WorkItem { private final Logger log = Logger.getLogger("org.openjdk.skara.bots");; private final ContinuousIntegration ci; private final String approversGroupId; + private final Set allowlist; private final List availableJobs; private final List defaultJobs; private final String name; @@ -46,10 +49,11 @@ public class TestWorkItem implements WorkItem { private final HostedRepository repository; private final PullRequest pr; - TestWorkItem(ContinuousIntegration ci, String approversGroupId, List availableJobs, + TestWorkItem(ContinuousIntegration ci, String approversGroupId, Set allowlist, List availableJobs, List defaultJobs, String name, Path storage, PullRequest pr) { this.ci = ci; this.approversGroupId = approversGroupId; + this.allowlist = allowlist; this.availableJobs = availableJobs; this.defaultJobs = defaultJobs; this.name = name; @@ -246,9 +250,14 @@ private String display(Job job) { return sb.toString(); } + private boolean validate(HostUser u) { + var forge = pr.repository().forge(); + return forge.isMemberOf(approversGroupId, u) || allowlist.contains(u.id()); + } + @Override public Collection run(Path scratchPath) { - var state = State.from(pr, approversGroupId); + var state = State.from(pr, this::validate); var stage = state.stage(); if (stage == Stage.NA || stage == Stage.ERROR || stage == Stage.PENDING || stage == Stage.FINISHED) { // nothing to do diff --git a/bots/tester/src/test/java/org/openjdk/skara/bots/tester/StateTests.java b/bots/tester/src/test/java/org/openjdk/skara/bots/tester/StateTests.java index 2f1d54eb2..dd9e7dc80 100644 --- a/bots/tester/src/test/java/org/openjdk/skara/bots/tester/StateTests.java +++ b/bots/tester/src/test/java/org/openjdk/skara/bots/tester/StateTests.java @@ -48,7 +48,7 @@ void noCommentsShouldEqualNA() { pr.author = duke; pr.comments = List.of(); - var state = State.from(pr, "0"); + var state = State.from(pr, u -> host.isMemberOf("0", u)); assertEquals(Stage.NA, state.stage()); assertEquals(null, state.requested()); assertEquals(null, state.pending()); @@ -78,7 +78,7 @@ void testCommentFromNotApprovedUserShouldEqualRequested() { var approvers = "0"; host.groups = Map.of(approvers, Set.of()); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.REQUESTED, state.stage()); assertEquals(comment, state.requested()); assertEquals(null, state.pending()); @@ -108,7 +108,7 @@ void testCommentFromApprovedUserShouldEqualApproved() { var approvers = "0"; host.groups = Map.of(approvers, Set.of(duke)); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.APPROVED, state.stage()); assertEquals(comment, state.requested()); assertEquals(null, state.pending()); @@ -143,7 +143,7 @@ void testApprovalNeededCommentShouldResultInPending() { pr.comments = List.of(testComment, pendingComment); host.groups = Map.of("0", Set.of()); - var state = State.from(pr, "0"); + var state = State.from(pr, u -> host.isMemberOf("0", u)); assertEquals(Stage.PENDING, state.stage()); assertEquals(testComment, state.requested()); assertEquals(pendingComment, state.pending()); @@ -191,7 +191,7 @@ void testStartedCommentShouldResultInRunning() { var approvers = "0"; host.groups = Map.of(approvers, Set.of(member)); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.STARTED, state.stage()); assertEquals(testComment, state.requested()); assertEquals(pendingComment, state.pending()); @@ -222,7 +222,7 @@ void cancelCommentFromAuthorShouldEqualCancelled() { var approvers = "0"; host.groups = Map.of(approvers, Set.of()); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.CANCELLED, state.stage()); assertEquals(testComment, state.requested()); assertEquals(cancelComment, state.cancelled()); @@ -256,7 +256,7 @@ void cancelCommentFromAnotherUserShouldHaveNoEffect() { var approvers = "0"; host.groups = Map.of(approvers, Set.of()); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.REQUESTED, state.stage()); assertEquals(testComment, state.requested()); assertEquals(null, state.cancelled()); @@ -289,7 +289,7 @@ void multipleTestCommentsShouldOnlyCareAboutLast() { var approvers = "0"; host.groups = Map.of(approvers, Set.of()); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.REQUESTED, state.stage()); assertEquals(test3Comment, state.requested()); assertEquals(null, state.cancelled()); @@ -326,7 +326,7 @@ void errorAfterRequestedShouldBeError() { var approvers = "0"; host.groups = Map.of(approvers, Set.of()); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.ERROR, state.stage()); assertEquals(testComment, state.requested()); assertEquals(null, state.pending()); @@ -381,7 +381,7 @@ void testFinishedCommentShouldResultInFinished() { var approvers = "0"; host.groups = Map.of(approvers, Set.of(member)); - var state = State.from(pr, approvers); + var state = State.from(pr, u -> host.isMemberOf(approvers, u)); assertEquals(Stage.FINISHED, state.stage()); assertEquals(testComment, state.requested()); assertEquals(pendingComment, state.pending()); diff --git a/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestBotTests.java b/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestBotTests.java index d8ca2c4a7..7884e93aa 100644 --- a/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestBotTests.java +++ b/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestBotTests.java @@ -49,7 +49,7 @@ void noTestCommentShouldDoNothing(TestInfo testInfo) throws IOException { var storage = tmp.path().resolve("storage"); var ci = new InMemoryContinuousIntegration(); - var bot = new TestBot(ci, "0", List.of(), List.of(), "", storage, upstreamHostedRepo); + var bot = new TestBot(ci, "0", Set.of(), List.of(), List.of(), "", storage, upstreamHostedRepo); var runner = new TestBotRunner(); runner.runPeriodicItems(bot); diff --git a/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestWorkItemTests.java b/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestWorkItemTests.java index d0df8299e..2c1bd6ac9 100644 --- a/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestWorkItemTests.java +++ b/bots/tester/src/test/java/org/openjdk/skara/bots/tester/TestWorkItemTests.java @@ -64,7 +64,7 @@ void noTestCommentsShouldDoNothing() throws IOException { pr.author = duke; pr.comments = List.of(); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); var comments = pr.comments(); @@ -99,7 +99,7 @@ void topLevelTestApproveShouldDoNothing() throws IOException { var testApproveComment = new Comment("0", "/test approve", duke, now, now); pr.comments = List.of(testApproveComment); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); var comments = pr.comments(); @@ -135,7 +135,7 @@ void topLevelTestCancelShouldDoNothing() throws IOException { var testApproveComment = new Comment("0", "/test cancel", duke, now, now); pr.comments = List.of(testApproveComment); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); var comments = pr.comments(); @@ -173,7 +173,7 @@ void testCommentWithMadeUpJobShouldBeError() throws IOException { var comment = new Comment("0", "/test foobar", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); var comments = pr.comments(); @@ -220,7 +220,7 @@ void testCommentFromUnapprovedUserShouldBePending() throws IOException { var comment = new Comment("0", "/test foobar", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); // Non-existing test group should result in error item.run(scratch); @@ -301,7 +301,7 @@ void cancelAtestCommentShouldBeCancel() throws IOException { var cancelComment = new Comment("1", "/test cancel", duke, now, now); pr.comments = new ArrayList<>(List.of(testComment, cancelComment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -342,7 +342,7 @@ void cancellingAPendingTestCommentShouldWork() throws IOException { var comment = new Comment("0", "/test tier1", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -426,7 +426,7 @@ void cancellingApprovedPendingRequestShouldBeCancelled() throws IOException { var comment = new Comment("0", "/test tier1", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -513,7 +513,7 @@ void approvedPendingRequestShouldBeStarted() throws IOException { var comment = new Comment("0", "/test tier1", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -621,7 +621,7 @@ void cancellingApprovedPendingRequestShouldBeCancel() throws IOException { var comment = new Comment("0", "/test tier1", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -746,7 +746,7 @@ void errorWhenCreatingTestJobShouldResultInError() throws IOException { var comment = new Comment("0", "/test tier1", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -834,7 +834,7 @@ void finishedJobShouldResultInFinishedComment() throws IOException { var comment = new Comment("0", "/test tier1", duke, now, now); pr.comments = new ArrayList<>(List.of(comment)); - var item = new TestWorkItem(ci, approvers, available, defaultJobs, name, storage, pr); + var item = new TestWorkItem(ci, approvers, Set.of(), available, defaultJobs, name, storage, pr); item.run(scratch); @@ -945,4 +945,82 @@ void finishedJobShouldResultInFinishedComment() throws IOException { assertEquals("8 jobs passed, 0 jobs with failures, 0 jobs not run", summaryLines[11]); } } + + @Test + void userOnApprovelistDoesNotNeedApproval() throws IOException { + try (var tmp = new TemporaryDirectory()) { + var localRepoDir = tmp.path().resolve("repository.git"); + var localRepo = Repository.init(localRepoDir, VCS.GIT); + var readme = localRepoDir.resolve("README"); + Files.writeString(readme, "Hello\n"); + localRepo.add(readme); + var head = localRepo.commit("Add README", "duke", "duke@openjdk.org"); + + var ci = new InMemoryContinuousIntegration(); + var approvers = "0"; + var available = List.of("tier1", "tier2", "tier3"); + var defaultJobs = List.of("tier1"); + var name = "test"; + var storage = tmp.path().resolve("storage"); + var scratch = tmp.path().resolve("storage"); + + var bot = new HostUser(1, "bot", "openjdk [bot]"); + var host = new InMemoryHost(); + host.currentUserDetails = bot; + host.groups = Map.of(approvers, Set.of()); + + var repo = new InMemoryHostedRepository(); + repo.host = host; + repo.webUrl = URI.create("file://" + localRepoDir.toAbsolutePath()); + repo.url = URI.create("file://" + localRepoDir.toAbsolutePath()); + repo.id = 1337L; + + var pr = new InMemoryPullRequest(); + pr.repository = repo; + pr.id = "17"; + + var duke = new HostUser(0, "duke", "Duke"); + pr.author = duke; + pr.headHash = head; + + var now = ZonedDateTime.now(); + var comment = new Comment("0", "/test tier1", duke, now, now); + pr.comments = new ArrayList<>(List.of(comment)); + + var item = new TestWorkItem(ci, approvers, Set.of("0"), available, defaultJobs, name, storage, pr); + + var expectedJobId = "null-1337-17-0"; + var expectedJob = new InMemoryJob(); + expectedJob.status = new Job.Status(0, 1, 7); + ci.jobs.put(expectedJobId, expectedJob); + + item.run(scratch); + + var comments = pr.comments(); + assertEquals(2, comments.size()); + assertEquals(comment, comments.get(0)); + var secondComment = comments.get(1); + assertEquals(bot, secondComment.author()); + + var lines = secondComment.body().split("\n"); + assertEquals("", lines[0]); + assertEquals("", lines[1]); + assertEquals("", lines[2]); + assertEquals("A test job has been started with id: " + expectedJobId, lines[3]); + + assertEquals(1, ci.submissions.size()); + var submission = ci.submissions.get(0); + assertTrue(submission.source.startsWith(storage)); + assertEquals(List.of("tier1"), submission.jobs); + assertEquals(expectedJobId, submission.id); + + var checks = pr.checks(pr.headHash()); + assertEquals(1, checks.keySet().size()); + var check = checks.get("test"); + assertEquals("Summary", check.title().get()); + assertTrue(check.summary() + .get() + .contains("0 jobs completed, 1 job running, 7 jobs not yet started")); + } + } }