diff --git a/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent.java b/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent.java index 12047fb6..8badb81e 100644 --- a/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent.java +++ b/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent.java @@ -2,14 +2,21 @@ import com.github.kostyasha.github.integration.generic.GitHubPRDecisionContext; import hudson.Extension; +import hudson.model.Job; import hudson.model.TaskListener; +import hudson.scheduler.CronTab; +import hudson.scheduler.CronTabList; +import hudson.scheduler.Hash; import org.jenkinsci.Symbol; import org.jenkinsci.plugins.github.pullrequest.GitHubPRCause; import org.jenkinsci.plugins.github.pullrequest.GitHubPRPullRequest; +import org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger; +import org.jenkinsci.plugins.github.pullrequest.GitHubPRTriggerMode; import org.jenkinsci.plugins.github.pullrequest.events.GitHubPREvent; import org.jenkinsci.plugins.github.pullrequest.events.GitHubPREventDescriptor; import org.jenkinsci.plugins.github.pullrequest.restrictions.GitHubPRUserRestriction; import org.kohsuke.github.GHIssueComment; +import org.kohsuke.github.GHIssueState; import org.kohsuke.github.GHPullRequest; import org.kohsuke.stapler.DataBoundConstructor; import org.slf4j.Logger; @@ -18,6 +25,10 @@ import edu.umd.cs.findbugs.annotations.NonNull; import java.io.IOException; import java.io.PrintStream; +import java.util.ArrayList; +import java.util.Calendar; +import java.util.Date; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -32,6 +43,7 @@ public class GitHubPRCommentEvent extends GitHubPREvent { private static final String DISPLAY_NAME = "Comment matched to pattern"; private static final Logger LOG = LoggerFactory.getLogger(GitHubPRCommentEvent.class); + private static final long CLOSED_PR_COMMENT_GRACE_MILLIS = 10 * 1000L; private String comment = ""; @@ -53,8 +65,18 @@ public GitHubPRCause check(@NonNull GitHubPRDecisionContext prDecisionContext) { final GitHubPRUserRestriction prUserRestriction = prDecisionContext.getPrUserRestriction(); GitHubPRCause cause = null; + final boolean isClosedWithoutLocalState = isNull(localPR) + && GHIssueState.CLOSED.equals(remotePR.getState()); + final Date closedPrCommentCutoff = + resolveClosedPrCommentCutoff(prDecisionContext, llog, isClosedWithoutLocalState); try { for (GHIssueComment issueComment : remotePR.getComments()) { + if (isClosedWithoutLocalState) { + Date commentUpdatedAt = resolveCommentUpdatedAt(issueComment); + if (isNull(commentUpdatedAt) || commentUpdatedAt.before(closedPrCommentCutoff)) { + continue; + } + } if (isNull(localPR) // test all comments for trigger word even if we never saw PR before || isNull(localPR.getLastCommentCreatedAt()) // PR was created but had no comments // don't check comments that we saw before @@ -81,6 +103,129 @@ public GitHubPRCause check(@NonNull GitHubPRDecisionContext prDecisionContext) { return cause; } + private Date resolveClosedPrCommentCutoff(GitHubPRDecisionContext prDecisionContext, + PrintStream llog, + boolean shouldResolve) { + if (!shouldResolve) { + return null; + } + long pollingIntervalMillis = resolvePollingIntervalMillis(prDecisionContext, llog); + long cutoffMillis = System.currentTimeMillis() - pollingIntervalMillis - CLOSED_PR_COMMENT_GRACE_MILLIS; + Date cutoff = new Date(cutoffMillis); + llog.println(DISPLAY_NAME + ": closed PR scan limited to comments updated since " + cutoff); + return cutoff; + } + + private long resolvePollingIntervalMillis(GitHubPRDecisionContext prDecisionContext, PrintStream llog) { + GitHubPRTrigger trigger = prDecisionContext.getTrigger(); + if (isNull(trigger)) { + return 0L; + } + + GitHubPRTriggerMode triggerMode = trigger.getTriggerMode(); + if (triggerMode == GitHubPRTriggerMode.HEAVY_HOOKS + || triggerMode == GitHubPRTriggerMode.LIGHT_HOOKS) { + return 0L; + } + + String spec = trigger.getSpec(); + if (isNull(spec) || spec.trim().isEmpty()) { + llog.println(DISPLAY_NAME + ": empty cron spec, using 0s polling window for closed PR scan"); + return 0L; + } + + Job job = trigger.getJob(); + String seed = job == null || job.getFullName() == null ? "github-pullrequest-trigger" : job.getFullName(); + List tabs = parseCronTabs(spec, Hash.from(seed), llog); + if (tabs.isEmpty()) { + llog.println(DISPLAY_NAME + ": no valid cron entries, using 0s polling window"); + return 0L; + } + try { + Calendar next = nextScheduledAfter(tabs, System.currentTimeMillis() + 1000L); + if (next == null) { + llog.println(DISPLAY_NAME + ": unable to resolve cron interval, using 0s polling window"); + return 0L; + } + Calendar nextAfter = nextScheduledAfter(tabs, next.getTimeInMillis() + 1000L); + if (nextAfter == null) { + llog.println(DISPLAY_NAME + ": unable to resolve cron interval, using 0s polling window"); + return 0L; + } + long intervalMillis = nextAfter.getTimeInMillis() - next.getTimeInMillis(); + if (intervalMillis <= 0L) { + llog.println(DISPLAY_NAME + ": non-positive cron interval, using 0s polling window"); + return 0L; + } + return intervalMillis; + } catch (IllegalArgumentException e) { + LOG.warn("Invalid cron spec '{}' while resolving polling interval for closed PR scan", spec, e); + llog.println(DISPLAY_NAME + ": invalid cron spec, using 0s polling window"); + return 0L; + } + } + + private List parseCronTabs(String spec, Hash hash, PrintStream llog) { + List tabs = new ArrayList<>(); + String timezone = null; + int lineNumber = 0; + for (String line : spec.split("\\r?\\n")) { + lineNumber++; + String trimmed = line.trim(); + if (lineNumber == 1 && trimmed.startsWith("TZ=")) { + String tz = trimmed.replace("TZ=", ""); + timezone = CronTabList.getValidTimezone(tz); + if (timezone == null) { + llog.println(DISPLAY_NAME + ": invalid cron timezone, using 0s polling window"); + return new ArrayList<>(); + } + continue; + } + if (trimmed.isEmpty() || trimmed.startsWith("#")) { + continue; + } + try { + tabs.add(new CronTab(trimmed, lineNumber, hash, timezone)); + } catch (IllegalArgumentException e) { + LOG.warn("Invalid cron entry '{}' while resolving polling interval for closed PR scan", trimmed, e); + llog.println(DISPLAY_NAME + ": invalid cron entry, using 0s polling window"); + return new ArrayList<>(); + } + } + return tabs; + } + + private static Calendar nextScheduledAfter(List tabs, long baseMillis) { + Calendar next = null; + for (CronTab tab : tabs) { + Calendar base = calendarFor(tab, baseMillis); + Calendar candidate = tab.ceil(base); + if (candidate == null) { + continue; + } + if (next == null || candidate.before(next)) { + next = candidate; + } + } + return next; + } + + private static Calendar calendarFor(CronTab tab, long baseMillis) { + Calendar calendar = tab.getTimeZone() == null + ? Calendar.getInstance() + : Calendar.getInstance(tab.getTimeZone()); + calendar.setTimeInMillis(baseMillis); + return calendar; + } + + private static Date resolveCommentUpdatedAt(GHIssueComment issueComment) throws IOException { + Date updatedAt = issueComment.getUpdatedAt(); + if (nonNull(updatedAt)) { + return updatedAt; + } + return issueComment.getCreatedAt(); + } + private GitHubPRCause checkComment(GitHubPRDecisionContext prDecisionContext, GHIssueComment issueComment, GitHubPRUserRestriction userRestriction, diff --git a/github-pullrequest-plugin/src/main/resources/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent/help.html b/github-pullrequest-plugin/src/main/resources/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent/help.html index 2d814773..150b9ec3 100644 --- a/github-pullrequest-plugin/src/main/resources/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent/help.html +++ b/github-pullrequest-plugin/src/main/resources/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent/help.html @@ -1,4 +1,6 @@
Triggers when a comment matching the pattern is posted in a pull request.
+ For closed or merged pull requests without local state, only comments updated within the polling interval + (or 0 for hook-only triggers) plus 10 seconds are evaluated to avoid reprocessing historic comments.
For example Test ([A-Za-z0-9 ,!]+) tags please.
diff --git a/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEventTest.java b/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEventTest.java index feeaf312..c9cab0f5 100644 --- a/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEventTest.java +++ b/github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEventTest.java @@ -1,10 +1,15 @@ package org.jenkinsci.plugins.github.pullrequest.events.impl; +import hudson.model.Job; import hudson.model.TaskListener; +import hudson.scheduler.CronTab; +import hudson.scheduler.CronTabList; +import hudson.scheduler.Hash; import org.jenkinsci.plugins.github.pullrequest.GitHubPRCause; import org.jenkinsci.plugins.github.pullrequest.GitHubPRLabel; import org.jenkinsci.plugins.github.pullrequest.GitHubPRPullRequest; import org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger; +import org.jenkinsci.plugins.github.pullrequest.GitHubPRTriggerMode; import org.junit.Test; import org.junit.runner.RunWith; import org.kohsuke.github.GHCommitPointer; @@ -21,7 +26,9 @@ import java.io.IOException; import java.io.PrintStream; import java.util.ArrayList; +import java.util.Calendar; import java.util.Date; +import java.util.List; import java.util.Set; import static com.github.kostyasha.github.integration.generic.GitHubPRDecisionContext.newGitHubPRDecisionContext; @@ -41,6 +48,7 @@ */ @RunWith(MockitoJUnitRunner.class) public class GitHubPRCommentEventTest { + private static final long CLOSED_PR_COMMENT_GRACE_MILLIS = 10 * 1000L; @Mock private GHPullRequest remotePr; @@ -214,6 +222,104 @@ public void testNullLocalPR() throws IOException { assertNotNull(cause); } + @Test + public void testClosedPrSkipsHistoricComments() throws IOException { + commonExpectations(emptySet()); + when(remotePr.getState()).thenReturn(GHIssueState.CLOSED); + when(trigger.getTriggerMode()).thenReturn(GitHubPRTriggerMode.HEAVY_HOOKS); + + Date oldCommentDate = new Date(System.currentTimeMillis() - CLOSED_PR_COMMENT_GRACE_MILLIS - 20000L); + when(comment.getCreatedAt()).thenReturn(oldCommentDate); + when(comment.getUpdatedAt()).thenReturn(oldCommentDate); + when(comment.getBody()).thenReturn("test foo, bar tags please."); + + final ArrayList ghIssueComments = new ArrayList<>(); + ghIssueComments.add(comment); + when(remotePr.getComments()).thenReturn(ghIssueComments); + + GitHubPRCause cause = new GitHubPRCommentEvent("test ([A-Za-z0-9 ,!]+) tags please.") + .check(newGitHubPRDecisionContext() + .withPrTrigger(trigger) + .withRemotePR(remotePr) + .withListener(listener) + .build() + ); // localPR is null and PR is closed + + assertNull(cause); + } + + @Test + public void testClosedPrMatchesLatestCommentUpdate() throws IOException { + commonExpectations(emptySet()); + causeCreationExpectations(); + when(remotePr.getState()).thenReturn(GHIssueState.CLOSED); + when(trigger.getTriggerMode()).thenReturn(GitHubPRTriggerMode.HEAVY_HOOKS); + + Date commentDate = new Date(System.currentTimeMillis() - 1000L); + when(comment.getCreatedAt()).thenReturn(new Date(System.currentTimeMillis() - CLOSED_PR_COMMENT_GRACE_MILLIS)); + when(comment.getUpdatedAt()).thenReturn(commentDate); + + final String body = "test foo, bar tags please."; + when(comment.getBody()).thenReturn(body); + + final ArrayList ghIssueComments = new ArrayList<>(); + ghIssueComments.add(comment); + when(remotePr.getComments()).thenReturn(ghIssueComments); + + GitHubPRCause cause = new GitHubPRCommentEvent("test ([A-Za-z0-9 ,!]+) tags please.") + .check(newGitHubPRDecisionContext() + .withPrTrigger(trigger) + .withRemotePR(remotePr) + .withListener(listener) + .build() + ); // localPR is null and PR is closed + + assertThat(cause.getCommentAuthorName(), is("commentOwnerName")); + assertThat(cause.getCommentAuthorEmail(), is("commentOwner@email.com")); + assertThat(cause.getCommentBody(), is(body)); + assertThat(cause.getCommentBodyMatch(), is("foo, bar")); + assertNotNull(cause); + } + + @Test + public void testClosedPrCronWindowAcceptsRecentComment() throws IOException { + commonExpectations(emptySet()); + causeCreationExpectations(); + when(remotePr.getState()).thenReturn(GHIssueState.CLOSED); + when(trigger.getTriggerMode()).thenReturn(GitHubPRTriggerMode.CRON); + when(trigger.getSpec()).thenReturn("H/5 * * * *"); + + Job job = mock(Job.class); + when(job.getFullName()).thenReturn("pr-comment-cron-window"); + when(trigger.getJob()).thenReturn(job); + + long intervalMillis = computeCronIntervalMillis("H/5 * * * *", "pr-comment-cron-window"); + Date recentCommentDate = new Date(System.currentTimeMillis() - intervalMillis + 2000L); + when(comment.getCreatedAt()).thenReturn(new Date(System.currentTimeMillis() - intervalMillis - 60000L)); + when(comment.getUpdatedAt()).thenReturn(recentCommentDate); + + final String body = "test foo, bar tags please."; + when(comment.getBody()).thenReturn(body); + + final ArrayList ghIssueComments = new ArrayList<>(); + ghIssueComments.add(comment); + when(remotePr.getComments()).thenReturn(ghIssueComments); + + GitHubPRCause cause = new GitHubPRCommentEvent("test ([A-Za-z0-9 ,!]+) tags please.") + .check(newGitHubPRDecisionContext() + .withPrTrigger(trigger) + .withRemotePR(remotePr) + .withListener(listener) + .build() + ); // localPR is null and PR is closed + + assertThat(cause.getCommentAuthorName(), is("commentOwnerName")); + assertThat(cause.getCommentAuthorEmail(), is("commentOwner@email.com")); + assertThat(cause.getCommentBody(), is(body)); + assertThat(cause.getCommentBodyMatch(), is("foo, bar")); + assertNotNull(cause); + } + private void commonExpectations(Set localLabels) throws IOException { when(labels.getLabelsSet()).thenReturn(localLabels); when(localPR.getLabels()).thenReturn(localLabels); @@ -238,4 +344,57 @@ private void causeCreationExpectations() throws IOException { when(remotePr.getHead()).thenReturn(mockPointer); when(remotePr.getBase()).thenReturn(mockPointer); } + + private long computeCronIntervalMillis(String spec, String seed) { + List tabs = parseCronTabs(spec, Hash.from(seed)); + Calendar next = nextScheduledAfter(tabs, System.currentTimeMillis() + 1000L); + Calendar nextAfter = nextScheduledAfter(tabs, next.getTimeInMillis() + 1000L); + return nextAfter.getTimeInMillis() - next.getTimeInMillis(); + } + + private List parseCronTabs(String spec, Hash hash) { + List tabs = new ArrayList<>(); + String timezone = null; + int lineNumber = 0; + for (String line : spec.split("\\r?\\n")) { + lineNumber++; + String trimmed = line.trim(); + if (lineNumber == 1 && trimmed.startsWith("TZ=")) { + String tz = trimmed.replace("TZ=", ""); + timezone = CronTabList.getValidTimezone(tz); + if (timezone == null) { + throw new IllegalArgumentException("Invalid cron timezone"); + } + continue; + } + if (trimmed.isEmpty() || trimmed.startsWith("#")) { + continue; + } + tabs.add(new CronTab(trimmed, lineNumber, hash, timezone)); + } + return tabs; + } + + private Calendar nextScheduledAfter(List tabs, long baseMillis) { + Calendar next = null; + for (CronTab tab : tabs) { + Calendar base = calendarFor(tab, baseMillis); + Calendar candidate = tab.ceil(base); + if (candidate == null) { + continue; + } + if (next == null || candidate.before(next)) { + next = candidate; + } + } + return next; + } + + private Calendar calendarFor(CronTab tab, long baseMillis) { + Calendar calendar = tab.getTimeZone() == null + ? Calendar.getInstance() + : Calendar.getInstance(tab.getTimeZone()); + calendar.setTimeInMillis(baseMillis); + return calendar; + } }