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

[JENKINS-63078] Allow ignoring rate limits #313

Merged
merged 7 commits into from
Aug 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -107,7 +108,25 @@ 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.
*/
NoThrottle(Messages.ApiRateLimitChecker_NoThrottle()) {

@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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If pointing at https://github.com this setting should pass through to ThrottleOnOver. There is literally never a reason to not check rate limits when interacting with github.com.

Copy link
Contributor

@car-roll car-roll Aug 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we block the selection of NoThrottle in the config page if the api uri is github.com?
edit: actually scratch that. For some reason I forgot you can add mulitple servers

}
}
;


private static final double MILLIS_PER_HOUR = TimeUnit.HOURS.toMillis(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.NoThrottle=Never check rate limit (NOT RECOMMENDED)

GitHubLink.DisplayName=GitHub

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@

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;
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;
Expand All @@ -24,6 +28,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 {

Expand Down Expand Up @@ -168,6 +173,51 @@ public void ThrottleOnNormalizeTestWithQuota() throws Exception {
assertEquals(100, getRequestCount(githubApi));
}

/**
* Verify that "NoThrottle" does not contact the GitHub api nor output any logs
*
* @author Marc Salles Navarro
*/
@Test
public void NoThrottleTestShouldNotThrottle() throws Exception {
// set up scenarios
List<RateLimit> 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.NoThrottle.checkApiRateLimit(listener, github);
// 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));
}

/**
* 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<RateLimit> 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"
*
Expand Down Expand Up @@ -198,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));

Expand Down Expand Up @@ -491,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.*")));
}

/**
Expand Down