From 6807c31183b5b1c1d1d98dd3fea00c3150835859 Mon Sep 17 00:00:00 2001 From: Marc Salles Date: Tue, 16 Jun 2020 14:27:33 +0200 Subject: [PATCH 1/5] Adds new ApiRateLimitChecker that ignores rate_limits --- .../ApiRateLimitChecker.java | 13 +++++++++++- .../github_branch_source/Messages.properties | 1 + .../ApiRateLimitCheckerTest.java | 21 +++++++++++++++++++ 3 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java index f8f7375b0..d566daf90 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java @@ -107,7 +107,18 @@ public void checkApiRateLimit(@NonNull TaskListener listener, GitHub github) thr waitUntilRateLimit(listener, github, rateLimit, expiration); } } - }; + }, + /** + * Ignore GitHub API Rate limit. Useful for GitHub Enterprise instances that might not have a limit set up. + */ + ThrottleNone(Messages.ApiRateLimitChecker_ThrottleNone()) { + + @Override + public void checkApiRateLimit(@NonNull TaskListener listener, GitHub github) throws IOException, InterruptedException { + // Nothing needed + } + } + ; private static final double MILLIS_PER_HOUR = TimeUnit.HOURS.toMillis(1); diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties index f17d0ebd7..8cbe6c2db 100644 --- a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties @@ -23,6 +23,7 @@ GitHubBuildStatusNotification.CommitStatusSet=GitHub has been notified of this c ApiRateLimitChecker.ThrottleForNormalize=Normalize API requests ApiRateLimitChecker.ThrottleOnOver=Throttle at/near rate limit +ApiRateLimitChecker.ThrottleNone=Ignore rate limit GitHubLink.DisplayName=GitHub diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java index 9465e3851..1924a04c0 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java @@ -168,6 +168,27 @@ public void ThrottleOnNormalizeTestWithQuota() throws Exception { assertEquals(100, getRequestCount(githubApi)); } + /** + * Verify that "ThrottleNone" does not throttle in any case. + * + * @author Marc Salles Navarro + */ + @Test + public void ThrottleNoneTestShouldNotThrottle() throws Exception { + // set up scenarios + List scenarios = new ArrayList<>(); + int limit = 5000; + // Keep decreasing quota until there's none left + for (int i = 500; i >= 0; i--) { + scenarios.add(new RateLimit(limit, i, soon)); + } + setupStubs(scenarios); + ApiRateLimitChecker.ThrottleNone.checkApiRateLimit(listener, github); + + + assertEquals(0, countOfOutputLinesContaining("Sleeping")); + } + /** * Verify exactly when the throttle is occurring in "OnOver" * From 7c3c9a1c1c32fe3e21136f827327e56f7baf0a39 Mon Sep 17 00:00:00 2001 From: Marc Salles Date: Wed, 15 Jul 2020 16:54:54 +0200 Subject: [PATCH 2/5] Rename ThrottleNone to NoThrottle --- .../plugins/github_branch_source/ApiRateLimitChecker.java | 2 +- .../plugins/github_branch_source/Messages.properties | 2 +- .../github_branch_source/ApiRateLimitCheckerTest.java | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java index d566daf90..6d84cb1f2 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java @@ -111,7 +111,7 @@ public void checkApiRateLimit(@NonNull TaskListener listener, GitHub github) thr /** * Ignore GitHub API Rate limit. Useful for GitHub Enterprise instances that might not have a limit set up. */ - ThrottleNone(Messages.ApiRateLimitChecker_ThrottleNone()) { + NoThrottle(Messages.ApiRateLimitChecker_NoThrottle()) { @Override public void checkApiRateLimit(@NonNull TaskListener listener, GitHub github) throws IOException, InterruptedException { diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties index 8cbe6c2db..544c132a4 100644 --- a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties @@ -23,7 +23,7 @@ GitHubBuildStatusNotification.CommitStatusSet=GitHub has been notified of this c ApiRateLimitChecker.ThrottleForNormalize=Normalize API requests ApiRateLimitChecker.ThrottleOnOver=Throttle at/near rate limit -ApiRateLimitChecker.ThrottleNone=Ignore rate limit +ApiRateLimitChecker.NoThrottle=Ignore rate limit GitHubLink.DisplayName=GitHub diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java index 1924a04c0..869c03d0b 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java @@ -169,12 +169,12 @@ public void ThrottleOnNormalizeTestWithQuota() throws Exception { } /** - * Verify that "ThrottleNone" does not throttle in any case. + * Verify that "NoThrottle" does not throttle in any case. * * @author Marc Salles Navarro */ @Test - public void ThrottleNoneTestShouldNotThrottle() throws Exception { + public void NoThrottleTestShouldNotThrottle() throws Exception { // set up scenarios List scenarios = new ArrayList<>(); int limit = 5000; @@ -183,7 +183,7 @@ public void ThrottleNoneTestShouldNotThrottle() throws Exception { scenarios.add(new RateLimit(limit, i, soon)); } setupStubs(scenarios); - ApiRateLimitChecker.ThrottleNone.checkApiRateLimit(listener, github); + ApiRateLimitChecker.NoThrottle.checkApiRateLimit(listener, github); assertEquals(0, countOfOutputLinesContaining("Sleeping")); From a99e7a3b09ffd9a81cd371eac1e543236a339b78 Mon Sep 17 00:00:00 2001 From: Marc Salles Date: Wed, 15 Jul 2020 22:30:31 +0200 Subject: [PATCH 3/5] Improve test for ApiRateLimitChecker.NoThrottle --- .../github_branch_source/ApiRateLimitCheckerTest.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java index 869c03d0b..3a49fac1d 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java @@ -2,6 +2,7 @@ import com.github.tomakehurst.wiremock.WireMockServer; import com.github.tomakehurst.wiremock.client.ScenarioMappingBuilder; +import com.github.tomakehurst.wiremock.http.RequestMethod; import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder; import com.github.tomakehurst.wiremock.stubbing.Scenario; import hudson.util.LogTaskListener; @@ -24,6 +25,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.resetAllScenarios; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; public class ApiRateLimitCheckerTest extends AbstractGitHubWireMockTest { @@ -169,7 +171,7 @@ public void ThrottleOnNormalizeTestWithQuota() throws Exception { } /** - * Verify that "NoThrottle" does not throttle in any case. + * Verify that "NoThrottle" does not contact the GitHub api nor output any logs * * @author Marc Salles Navarro */ @@ -184,9 +186,10 @@ public void NoThrottleTestShouldNotThrottle() throws Exception { } setupStubs(scenarios); ApiRateLimitChecker.NoThrottle.checkApiRateLimit(listener, github); - - - assertEquals(0, countOfOutputLinesContaining("Sleeping")); + // there should be no output + assertEquals(0, countOfOutputLines(m -> m.matches("[sS]leeping"))); + // github rate_limit endpoint should not be contacted + assertEquals(0, getRequestCount(githubApi)); } /** From b88408fe373e104938f37939d68cdddb2e515ccc Mon Sep 17 00:00:00 2001 From: sirstrahd Date: Mon, 20 Jul 2020 08:16:38 +0200 Subject: [PATCH 4/5] More explicit warning on selector for ApiRateLimitChecker.NoThrottle Co-authored-by: Liam Newman --- .../jenkinsci/plugins/github_branch_source/Messages.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties index 544c132a4..786b17748 100644 --- a/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties +++ b/src/main/resources/org/jenkinsci/plugins/github_branch_source/Messages.properties @@ -23,7 +23,7 @@ GitHubBuildStatusNotification.CommitStatusSet=GitHub has been notified of this c ApiRateLimitChecker.ThrottleForNormalize=Normalize API requests ApiRateLimitChecker.ThrottleOnOver=Throttle at/near rate limit -ApiRateLimitChecker.NoThrottle=Ignore rate limit +ApiRateLimitChecker.NoThrottle=Never check rate limit (NOT RECOMMENDED) GitHubLink.DisplayName=GitHub From 7b39521cb53cf8a427fddaa64c31e97a17096a53 Mon Sep 17 00:00:00 2001 From: Marc Salles Date: Mon, 20 Jul 2020 11:23:46 +0200 Subject: [PATCH 5/5] NoThrottle will now redirect to ThrottleOnOver if GitHub.com api detected --- .../ApiRateLimitChecker.java | 8 +++++ .../ApiRateLimitCheckerTest.java | 32 +++++++++++++++++-- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java b/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java index 6d84cb1f2..e8bb2ffbc 100644 --- a/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java +++ b/src/main/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitChecker.java @@ -3,6 +3,7 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.Util; import hudson.model.TaskListener; +import org.jenkinsci.plugins.github.config.GitHubServerConfig; import org.kohsuke.github.GHRateLimit; import org.kohsuke.github.GitHub; @@ -115,6 +116,13 @@ public void checkApiRateLimit(@NonNull TaskListener listener, GitHub github) thr @Override public void checkApiRateLimit(@NonNull TaskListener listener, GitHub github) throws IOException, InterruptedException { + if (GitHubServerConfig.GITHUB_URL.equals(github.getApiUrl())) { + + // If the GitHub public API is being used, this will fallback to ThrottleOnOver + listener.getLogger().println(GitHubConsoleNote.create(System.currentTimeMillis(), + "GitHub throttling is disabled, which is not allowed for public GitHub usage, so ThrottleOnOver will be used instead. To configure a different rate limiting strategy, go to \"GitHub API usage\" under \"Configure System\" in the Jenkins settings.")); + ThrottleOnOver.checkApiRateLimit(listener, github); + } // Nothing needed } } diff --git a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java index 3a49fac1d..c200adf45 100644 --- a/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github_branch_source/ApiRateLimitCheckerTest.java @@ -7,9 +7,12 @@ import com.github.tomakehurst.wiremock.stubbing.Scenario; import hudson.util.LogTaskListener; import hudson.util.RingBufferLogHandler; +import org.jenkinsci.plugins.github.config.GitHubServerConfig; import org.junit.Test; import org.junit.Before; import org.kohsuke.github.GitHub; +import org.mockito.Mock; +import org.mockito.Mockito; import java.time.LocalDateTime; import java.time.ZoneId; @@ -187,11 +190,34 @@ public void NoThrottleTestShouldNotThrottle() throws Exception { setupStubs(scenarios); ApiRateLimitChecker.NoThrottle.checkApiRateLimit(listener, github); // there should be no output - assertEquals(0, countOfOutputLines(m -> m.matches("[sS]leeping"))); + assertEquals(0, countOfOutputLines(m -> m.matches(".*[sS]leeping.*"))); // github rate_limit endpoint should not be contacted assertEquals(0, getRequestCount(githubApi)); } + /** + * Verify that "NoThrottle" falls back to "ThrottleOnOver" if using GitHub.com + * + * @author Marc Salles Navarro + */ + @Test + public void NoThrottleTestShouldFallbackToThrottleOnOverForGitHubDotCom() throws Exception { + GitHub spy = Mockito.spy(github); + Mockito.when(spy.getApiUrl()).thenReturn(GitHubServerConfig.GITHUB_URL).thenReturn(github.getApiUrl()); + // set up scenarios + List scenarios = new ArrayList<>(); + int limit = 5000; + int buffer = ApiRateLimitChecker.calculateBuffer(limit); + scenarios.add(new RateLimit(limit, buffer -1, soon)); + scenarios.add(new RateLimit(limit, limit, new Date(soon.getTime() + 2000))); + setupStubs(scenarios); + ApiRateLimitChecker.NoThrottle.checkApiRateLimit(listener, spy); + + assertEquals(1, countOfOutputLines(m -> m.matches(".*[sS]leeping.*"))); + // github rate_limit endpoint should be contacted by ThrottleOnOver + assertEquals(3, getRequestCount(githubApi)); + } + /** * Verify exactly when the throttle is occurring in "OnOver" * @@ -222,7 +248,7 @@ public void ThrottleOnOverTest() throws Exception { } //should be no output - assertEquals(0, countOfOutputLines(m -> m.matches("[sS]leeping"))); + assertEquals(0, countOfOutputLines(m -> m.matches(".*[sS]leeping.*"))); assertEquals(11, getRequestCount(githubApi)); @@ -515,7 +541,7 @@ public void NormalizeExpectedIdealOverTime() throws Exception { // Making sure the budgets are correct assertEquals(12, countOfOutputLinesContaining("0 under budget")); // no occurrences of sleeping - assertEquals(0, countOfOutputLines(m -> m.matches("[sS]leeping"))); + assertEquals(0, countOfOutputLines(m -> m.matches(".*[sS]leeping.*"))); } /**