From 2dcb2a70d8c3b0b100245c7b79453c35df1f6e56 Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@arkins.net>
Date: Thu, 11 Jan 2018 11:49:01 +0100
Subject: [PATCH] feat: prHourlyLimit
Adds a feature to enforce an hourly limit on PR creations.
Closes #1363
---
lib/config/definitions.js | 7 ++++++
lib/platform/github/index.js | 2 ++
lib/platform/gitlab/index.js | 2 ++
lib/platform/vsts/index.js | 5 ++++
lib/workers/branch/index.js | 19 +++++++++++----
lib/workers/repository/write.js | 24 ++++++++++++++++++-
.../platform/__snapshots__/index.spec.js.snap | 3 +++
test/platform/vsts/index.spec.js | 6 ++++-
test/workers/branch/index.spec.js | 14 +++++++++++
test/workers/repository/write.spec.js | 17 +++++++++++++
.../2017-10-05-configuration-options.md | 19 +++++++++++++++
11 files changed, 112 insertions(+), 6 deletions(-)
diff --git a/lib/config/definitions.js b/lib/config/definitions.js
index 6bfa867981..c85207bce7 100644
--- a/lib/config/definitions.js
+++ b/lib/config/definitions.js
@@ -525,6 +525,13 @@ const options = [
// Must be at least 24 hours to give time for the unpublishSafe check to "complete".
default: 24,
},
+ {
+ name: 'prHourlyLimit',
+ description:
+ 'Rate limit PRs to maximum x created per hour. 0 (default) means no limit.',
+ type: 'integer',
+ default: 0, // no limit
+ },
// Automatic merging
{
name: 'automerge',
diff --git a/lib/platform/github/index.js b/lib/platform/github/index.js
index 7b0d6f0704..99b2fb8c4b 100644
--- a/lib/platform/github/index.js
+++ b/lib/platform/github/index.js
@@ -35,6 +35,7 @@ module.exports = {
ensureComment,
ensureCommentRemoval,
// PR
+ getPrList,
findPr,
createPr,
getPr,
@@ -617,6 +618,7 @@ async function getPrList() {
pr.state === 'closed' && pr.merged_at && pr.merged_at.length
? 'merged'
: pr.state,
+ createdAt: pr.created_at,
closed_at: pr.closed_at,
}));
logger.info({ length: config.prList.length }, 'Retrieved Pull Requests');
diff --git a/lib/platform/gitlab/index.js b/lib/platform/gitlab/index.js
index 27bc32595b..7664ee7ef3 100644
--- a/lib/platform/gitlab/index.js
+++ b/lib/platform/gitlab/index.js
@@ -31,6 +31,7 @@ module.exports = {
ensureComment,
ensureCommentRemoval,
// PR
+ getPrList,
findPr,
createPr,
getPr,
@@ -353,6 +354,7 @@ async function getPrList() {
branchName: pr.source_branch,
title: pr.title,
state: pr.state === 'opened' ? 'open' : pr.state,
+ createdAt: pr.created_at,
}));
}
return config.prList;
diff --git a/lib/platform/vsts/index.js b/lib/platform/vsts/index.js
index 3f5220efce..61f46f9105 100644
--- a/lib/platform/vsts/index.js
+++ b/lib/platform/vsts/index.js
@@ -32,6 +32,7 @@ module.exports = {
ensureComment,
ensureCommentRemoval,
// PR
+ getPrList,
findPr,
createPr,
getPr,
@@ -159,6 +160,10 @@ async function getFile(filePath, branchName = config.baseBranch) {
return f;
}
+function getPrList() {
+ return [];
+}
+
async function findPr(branchName, prTitle, state = 'all') {
logger.debug(`findPr(${branchName}, ${prTitle}, ${state})`);
let prsFiltered = [];
diff --git a/lib/workers/branch/index.js b/lib/workers/branch/index.js
index cac7713a42..fccbffcb26 100644
--- a/lib/workers/branch/index.js
+++ b/lib/workers/branch/index.js
@@ -25,7 +25,10 @@ async function processBranch(branchConfig) {
branch: config.branchName,
dependencies,
});
- logger.trace({ config }, 'processBranch');
+ logger.debug('processBranch()');
+ logger.trace({ config });
+ const branchExists = await platform.branchExists(config.branchName);
+ logger.debug(`branchExists=${branchExists}`);
try {
logger.info(`Branch has ${dependencies.length} upgrade(s)`);
@@ -58,7 +61,7 @@ async function processBranch(branchConfig) {
content +=
'\n\nIf this PR was closed by mistake or you changed your mind, you can simply reopen or rename it to reactivate Renovate for this dependency version.';
await platform.ensureComment(pr.number, subject, content);
- if (await platform.branchExists(config.branchName)) {
+ if (branchExists) {
await platform.deleteBranch(config.branchName);
}
return 'already-existed';
@@ -89,7 +92,7 @@ async function processBranch(branchConfig) {
// Check schedule
config.isScheduledNow = isScheduledNow(config);
if (!config.isScheduledNow) {
- if (!await platform.branchExists(config.branchName)) {
+ if (!branchExists) {
logger.info('Skipping branch creation as not within schedule');
return 'not-scheduled';
}
@@ -122,8 +125,13 @@ async function processBranch(branchConfig) {
} else {
logger.debug('No updated lock files in branch');
}
+
+ if (!branchExists && config.prHourlyLimitReached) {
+ logger.info('Reached PR creation limit - skipping branch creation');
+ return 'pr-hourly-limit-reached';
+ }
+
const committedFiles = await commitFilesToBranch(config);
- const branchExists = await platform.branchExists(config.branchName);
if (!(committedFiles || branchExists)) {
return 'no-work';
}
@@ -199,5 +207,8 @@ async function processBranch(branchConfig) {
logger.error({ err }, `Error ensuring PR: ${err.message}`);
// Don't throw here - we don't want to stop the other renovations
}
+ if (!branchExists) {
+ return 'pr-created';
+ }
return 'done';
}
diff --git a/lib/workers/repository/write.js b/lib/workers/repository/write.js
index 749dfd0d8a..83666438c3 100644
--- a/lib/workers/repository/write.js
+++ b/lib/workers/repository/write.js
@@ -1,3 +1,4 @@
+const moment = require('moment');
const tmp = require('tmp-promise');
const branchWorker = require('../branch');
@@ -14,13 +15,34 @@ async function writeUpdates(config) {
logger.info(`Processing ${branches.length} "pin" PRs first`);
}
const tmpDir = await tmp.dir({ unsafeCleanup: true });
+ let prsRemaining = 99;
+ if (config.prHourlyLimit) {
+ const prList = await platform.getPrList();
+ const currentHourStart = moment({
+ hour: moment().hour(),
+ });
+ try {
+ prsRemaining =
+ config.prHourlyLimit -
+ prList.filter(pr => moment(pr.createdAt).isAfter(currentHourStart))
+ .length;
+ logger.info(`PR creations remaining this hour: ${prsRemaining}`);
+ } catch (err) {
+ logger.error('Error checking PRs created per hour');
+ }
+ }
try {
for (const branch of branches) {
- const res = await branchWorker.processBranch({ ...branch, tmpDir });
+ const res = await branchWorker.processBranch({
+ ...branch,
+ tmpDir,
+ prHourlyLimitReached: prsRemaining <= 0,
+ });
if (res === 'pr-closed' || res === 'automerged') {
// Stop procesing other branches because base branch has been changed
return res;
}
+ prsRemaining -= res === 'pr-created' ? 1 : 0;
}
return 'done';
} finally {
diff --git a/test/platform/__snapshots__/index.spec.js.snap b/test/platform/__snapshots__/index.spec.js.snap
index d9e87ca47a..0543b2fb6e 100644
--- a/test/platform/__snapshots__/index.spec.js.snap
+++ b/test/platform/__snapshots__/index.spec.js.snap
@@ -23,6 +23,7 @@ Array [
"addReviewers",
"ensureComment",
"ensureCommentRemoval",
+ "getPrList",
"findPr",
"createPr",
"getPr",
@@ -58,6 +59,7 @@ Array [
"addReviewers",
"ensureComment",
"ensureCommentRemoval",
+ "getPrList",
"findPr",
"createPr",
"getPr",
@@ -93,6 +95,7 @@ Array [
"addReviewers",
"ensureComment",
"ensureCommentRemoval",
+ "getPrList",
"findPr",
"createPr",
"getPr",
diff --git a/test/platform/vsts/index.spec.js b/test/platform/vsts/index.spec.js
index 698c52a19c..ad5a490b85 100644
--- a/test/platform/vsts/index.spec.js
+++ b/test/platform/vsts/index.spec.js
@@ -286,7 +286,11 @@ describe('platform/vsts', () => {
});
*/
});
-
+ describe('getPrList()', () => {
+ it('returns empty array', () => {
+ expect(vsts.getPrList()).toEqual([]);
+ });
+ });
describe('getFileList', () => {
it('returns empty array if error', async () => {
await initRepo('some/repo', 'token');
diff --git a/test/workers/branch/index.spec.js b/test/workers/branch/index.spec.js
index b4d0ce42e3..7b2627f40a 100644
--- a/test/workers/branch/index.spec.js
+++ b/test/workers/branch/index.spec.js
@@ -98,6 +98,20 @@ describe('workers/branch', () => {
const res = await branchWorker.processBranch(config);
expect(res).not.toEqual('pr-edited');
});
+ it('returns if pr creation limit exceeded', async () => {
+ manager.getUpdatedPackageFiles.mockReturnValueOnce({
+ updatedPackageFiles: [],
+ });
+ lockFiles.getUpdatedLockFiles.mockReturnValueOnce({
+ lockFileError: false,
+ updatedLockFiles: [],
+ });
+ platform.branchExists.mockReturnValueOnce(false);
+ config.prHourlyLimitReached = true;
+ expect(await branchWorker.processBranch(config)).toEqual(
+ 'pr-hourly-limit-reached'
+ );
+ });
it('returns if no work', async () => {
manager.getUpdatedPackageFiles.mockReturnValueOnce({
updatedPackageFiles: [],
diff --git a/test/workers/repository/write.spec.js b/test/workers/repository/write.spec.js
index 6f8e335811..89435d9d12 100644
--- a/test/workers/repository/write.spec.js
+++ b/test/workers/repository/write.spec.js
@@ -1,5 +1,6 @@
const { writeUpdates } = require('../../../lib/workers/repository/write');
const branchWorker = require('../../../lib/workers/branch');
+const moment = require('moment');
branchWorker.processBranch = jest.fn();
@@ -11,6 +12,22 @@ beforeEach(() => {
describe('workers/repository/write', () => {
describe('writeUpdates()', () => {
+ it('calculates hourly limit remaining', async () => {
+ config.branches = [];
+ config.prHourlyLimit = 1;
+ platform.getPrList.mockReturnValueOnce([
+ { created_at: moment().format() },
+ ]);
+ const res = await writeUpdates(config);
+ expect(res).toEqual('done');
+ });
+ it('handles error in calculation', async () => {
+ config.branches = [];
+ config.prHourlyLimit = 1;
+ platform.getPrList.mockReturnValueOnce([{}, null]);
+ const res = await writeUpdates(config);
+ expect(res).toEqual('done');
+ });
it('runs pins first', async () => {
config.branches = [{ isPin: true }, {}, {}];
const res = await writeUpdates(config);
diff --git a/website/docs/_posts/2017-10-05-configuration-options.md b/website/docs/_posts/2017-10-05-configuration-options.md
index b826796848..cdf87ce07f 100644
--- a/website/docs/_posts/2017-10-05-configuration-options.md
+++ b/website/docs/_posts/2017-10-05-configuration-options.md
@@ -719,6 +719,25 @@ This setting tells Renovate when you would like it to raise PRs:
Renovate defaults to `immediate` but some like to change to `not-pending`. If you set to immediate, it means you will usually get GitHub notifications that a new PR is available but if you view it immediately then it will still have "pending" tests so you can't take any action. With `not-pending`, it means that when you receive the PR notification, you can see if it passed or failed and take action immediately. Therefore you can customise this setting if you wish to be notified a little later in order to reduce "noise".
+## prHourlyLimit
+
+Rate limit PRs to maximum x created per hour. 0 (default) means no limit.
+
+| name | value |
+| ------- | ------- |
+| type | integer |
+| default | 0 |
+
+This setting - if enabled - helps slow down Renovate, particularly during the onboarding phase. What may happen without this setting is:
+
+1. Onboarding PR is created
+2. User merges onboarding PR to activate Renovate
+3. Renovate creates a "Pin Dependencies" PR (if necessary)
+4. User merges Pin PR
+5. Renovate then creates every single upgrade PR necessary - potentially dozens
+
+The above can result in swamping CI systems, as well as a lot of retesting if branches need to be rebased every time one is merged. Instead, if `prHourlyLimit` is set to a value like 1 or 2, it will mean that Renovate creates at most that many new PRs within each hourly period (:00-:59). So the project should still result in all PRs created perhaps within the first 24 hours maximum, but at a rate that may allow users to merge them once they pass tests. It does not place a limit on the number of _concurrently open_ PRs - only on the rate they are created.
+
## prNotPendingHours
| name | value |
--
GitLab