From deac76b015cb5e25196d2431ff997ae08e005cda Mon Sep 17 00:00:00 2001
From: Rhys Arkins <rhys@keylocation.sg>
Date: Mon, 28 Aug 2017 11:37:09 +0200
Subject: [PATCH] feat: support timeout for pr creation = not-pending (#748)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
We should not leave the PR unopened forever if the branch remains in not-pending state too long. Some status checks may leave the status as “pending†instead of “failedâ€. Defaults to 12 hours but is configurable.
Closes #747
---
docs/configuration.md | 9 ++++
lib/api/github.js | 20 ++++++++-
lib/api/gitlab.js | 13 ++++++
lib/config/definitions.js | 6 +++
lib/workers/pr/index.js | 20 ++++++++-
test/api/__snapshots__/github.spec.js.snap | 2 +
test/api/__snapshots__/gitlab.spec.js.snap | 2 +
test/api/github.spec.js | 26 +++++++++++
test/api/gitlab.spec.js | 45 +++++++++++++++++++
.../package/__snapshots__/index.spec.js.snap | 4 ++
test/workers/pr/index.spec.js | 11 +++++
11 files changed, 154 insertions(+), 4 deletions(-)
diff --git a/docs/configuration.md b/docs/configuration.md
index 77a6461b37..9dc387908b 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -98,6 +98,7 @@ $ node renovate --help
--rebase-stale-prs [boolean] Rebase stale PRs (GitHub only)
--unpublish-safe [boolean] Set a status check for unpublish-safe upgrades
--pr-creation <string> When to create the PR for a branch. Values: immediate, not-pending, status-success.
+ --pr-not-pending-hours <integer> Timeout in hours for when prCreation=not-pending
--automerge [boolean] Whether to automerge branches/PRs automatically, without human intervention
--automerge-type <string> How to automerge - "branch-merge-commit", "branch-push" or "pr". Branch support is GitHub-only
--lazy-grouping [boolean] Use group names only when multiple dependencies upgraded
@@ -515,6 +516,14 @@ Obviously, you can't set repository or package file location with this method.
<td>`RENOVATE_PR_CREATION`</td>
<td>`--pr-creation`<td>
</tr>
+<tr>
+ <td>`prNotPendingHours`</td>
+ <td>Timeout in hours for when prCreation=not-pending</td>
+ <td>integer</td>
+ <td><pre>12</pre></td>
+ <td>`RENOVATE_PR_NOT_PENDING_HOURS`</td>
+ <td>`--pr-not-pending-hours`<td>
+</tr>
<tr>
<td>`automerge`</td>
<td>Whether to automerge branches/PRs automatically, without human intervention</td>
diff --git a/lib/api/github.js b/lib/api/github.js
index 415a299555..ff4f35659d 100644
--- a/lib/api/github.js
+++ b/lib/api/github.js
@@ -24,6 +24,7 @@ module.exports = {
setBranchStatus,
deleteBranch,
mergeBranch,
+ getBranchLastCommitTime,
// issue
addAssignees,
addReviewers,
@@ -44,6 +45,8 @@ module.exports = {
getFileJson,
// Commits
getCommitMessages,
+ getBranchCommit,
+ getCommitDetails,
};
// Get all installations for a GitHub app
@@ -411,6 +414,18 @@ async function mergeBranch(branchName, mergeType) {
await deleteBranch(branchName);
}
+async function getBranchLastCommitTime(branchName) {
+ try {
+ const res = await ghGotRetry(
+ `repos/${config.repoName}/commits?sha=${branchName}`
+ );
+ return new Date(res.body[0].commit.committer.date);
+ } catch (err) {
+ logger.error({ err }, `getBranchLastCommitTime error`);
+ return new Date();
+ }
+}
+
// Issue
async function addAssignees(issueNo, assignees) {
@@ -741,9 +756,10 @@ async function createBlob(fileContents) {
// Return the commit SHA for a branch
async function getBranchCommit(branchName) {
- return (await ghGotRetry(
+ const res = await ghGotRetry(
`repos/${config.repoName}/git/refs/heads/${branchName}`
- )).body.object.sha;
+ );
+ return res.body.object.sha;
}
async function getCommitDetails(commit) {
diff --git a/lib/api/gitlab.js b/lib/api/gitlab.js
index c5391f4e6b..3d78ba50cf 100644
--- a/lib/api/gitlab.js
+++ b/lib/api/gitlab.js
@@ -18,6 +18,7 @@ module.exports = {
getBranchStatusCheck,
setBranchStatus,
deleteBranch,
+ getBranchLastCommitTime,
// issue
addAssignees,
addReviewers,
@@ -270,6 +271,18 @@ async function deleteBranch(branchName) {
);
}
+async function getBranchLastCommitTime(branchName) {
+ try {
+ const res = await glGot(
+ `projects/${config.repoName}/repository/commits?ref_name=${branchName}`
+ );
+ return new Date(res.body[0].committed_date);
+ } catch (err) {
+ logger.error({ err }, `getBranchLastCommitTime error`);
+ return new Date();
+ }
+}
+
// Issue
async function addAssignees(prNo, assignees) {
diff --git a/lib/config/definitions.js b/lib/config/definitions.js
index b61be3c58a..146f2ec50b 100644
--- a/lib/config/definitions.js
+++ b/lib/config/definitions.js
@@ -376,6 +376,12 @@ const options = [
type: 'string',
default: 'immediate',
},
+ {
+ name: 'prNotPendingHours',
+ description: 'Timeout in hours for when prCreation=not-pending',
+ type: 'integer',
+ default: 12,
+ },
// Automatic merging
{
name: 'automerge',
diff --git a/lib/workers/pr/index.js b/lib/workers/pr/index.js
index 0526dd03f2..63879f19c3 100644
--- a/lib/workers/pr/index.js
+++ b/lib/workers/pr/index.js
@@ -45,8 +45,24 @@ async function ensurePr(prConfig) {
} else if (config.prCreation === 'not-pending') {
logger.debug('Checking branch combined status');
if (branchStatus === 'pending' || branchStatus === 'running') {
- logger.debug(`Branch status is "${branchStatus}" - not creating PR`);
- return null;
+ logger.debug(`Branch status is "${branchStatus}" - checking timeout`);
+ const lastCommitTime = await config.api.getBranchLastCommitTime(
+ branchName
+ );
+ const currentTime = new Date();
+ const millisecondsPerHour = 1000 * 60 * 60;
+ const elapsedHours = Math.round(
+ (currentTime.getTime() - lastCommitTime.getTime()) / millisecondsPerHour
+ );
+ if (elapsedHours < config.prNotPendingHours) {
+ logger.debug(
+ `Branch is ${elapsedHours} hours old - skipping PR creation`
+ );
+ return null;
+ }
+ logger.debug(
+ `prNotPendingHours=${config.prNotPendingHours} threshold hit - creating PR`
+ );
}
logger.debug('Branch status success');
}
diff --git a/test/api/__snapshots__/github.spec.js.snap b/test/api/__snapshots__/github.spec.js.snap
index 868b799125..bec271a52c 100644
--- a/test/api/__snapshots__/github.spec.js.snap
+++ b/test/api/__snapshots__/github.spec.js.snap
@@ -689,6 +689,8 @@ Array [
]
`;
+exports[`api/github getBranchLastCommitTime should return a Date 1`] = `2011-04-14T16:00:49.000Z`;
+
exports[`api/github getBranchPr(branchName) should return null if no PR exists 1`] = `
Array [
Array [
diff --git a/test/api/__snapshots__/gitlab.spec.js.snap b/test/api/__snapshots__/gitlab.spec.js.snap
index 6cf51c50dc..67c1586b73 100644
--- a/test/api/__snapshots__/gitlab.spec.js.snap
+++ b/test/api/__snapshots__/gitlab.spec.js.snap
@@ -195,6 +195,8 @@ Array [
exports[`api/gitlab getBranch returns a branch 1`] = `"foo"`;
+exports[`api/gitlab getBranchLastCommitTime should return a Date 1`] = `2012-09-20T08:50:22.000Z`;
+
exports[`api/gitlab getBranchPr(branchName) should return null if no PR exists 1`] = `
Array [
Array [
diff --git a/test/api/github.spec.js b/test/api/github.spec.js
index 58d1d40ae9..aa9282fe32 100644
--- a/test/api/github.spec.js
+++ b/test/api/github.spec.js
@@ -1044,6 +1044,32 @@ describe('api/github', () => {
expect(ghGot.delete.mock.calls).toMatchSnapshot();
});
});
+ describe('getBranchLastCommitTime', () => {
+ it('should return a Date', async () => {
+ await initRepo('some/repo', 'token');
+ ghGot.mockReturnValueOnce({
+ body: [
+ {
+ commit: {
+ committer: {
+ date: '2011-04-14T16:00:49Z',
+ },
+ },
+ },
+ ],
+ });
+ const res = await github.getBranchLastCommitTime('some-branch');
+ expect(res).toMatchSnapshot();
+ });
+ it('handles error', async () => {
+ await initRepo('some/repo', 'token');
+ ghGot.mockReturnValueOnce({
+ body: [],
+ });
+ const res = await github.getBranchLastCommitTime('some-branch');
+ expect(res).toBeDefined();
+ });
+ });
describe('addAssignees(issueNo, assignees)', () => {
it('should add the given assignees to the issue', async () => {
await initRepo('some/repo', 'token');
diff --git a/test/api/gitlab.spec.js b/test/api/gitlab.spec.js
index 10ac368182..581ef8b846 100644
--- a/test/api/gitlab.spec.js
+++ b/test/api/gitlab.spec.js
@@ -399,6 +399,51 @@ describe('api/gitlab', () => {
expect(glGot.delete.mock.calls.length).toBe(1);
});
});
+ describe('getBranchLastCommitTime', () => {
+ it('should return a Date', async () => {
+ await initRepo('some/repo', 'token');
+ glGot.mockReturnValueOnce({
+ body: [
+ {
+ id: 'ed899a2f4b50b4370feeea94676502b42383c746',
+ short_id: 'ed899a2f4b5',
+ title: 'Replace sanitize with escape once',
+ author_name: 'Dmitriy Zaporozhets',
+ author_email: 'dzaporozhets@sphereconsultinginc.com',
+ authored_date: '2012-09-20T11:50:22+03:00',
+ committer_name: 'Administrator',
+ committer_email: 'admin@example.com',
+ committed_date: '2012-09-20T11:50:22+03:00',
+ created_at: '2012-09-20T11:50:22+03:00',
+ message: 'Replace sanitize with escape once',
+ parent_ids: ['6104942438c14ec7bd21c6cd5bd995272b3faff6'],
+ },
+ {
+ id: '6104942438c14ec7bd21c6cd5bd995272b3faff6',
+ short_id: '6104942438c',
+ title: 'Sanitize for network graph',
+ author_name: 'randx',
+ author_email: 'dmitriy.zaporozhets@gmail.com',
+ committer_name: 'Dmitriy',
+ committer_email: 'dmitriy.zaporozhets@gmail.com',
+ created_at: '2012-09-20T09:06:12+03:00',
+ message: 'Sanitize for network graph',
+ parent_ids: ['ae1d9fb46aa2b07ee9836d49862ec4e2c46fbbba'],
+ },
+ ],
+ });
+ const res = await gitlab.getBranchLastCommitTime('some-branch');
+ expect(res).toMatchSnapshot();
+ });
+ it('handles error', async () => {
+ await initRepo('some/repo', 'token');
+ glGot.mockReturnValueOnce({
+ body: [],
+ });
+ const res = await gitlab.getBranchLastCommitTime('some-branch');
+ expect(res).toBeDefined();
+ });
+ });
describe('addAssignees(issueNo, assignees)', () => {
it('should add the given assignees to the issue', async () => {
await initRepo('some/repo', 'token');
diff --git a/test/workers/package/__snapshots__/index.spec.js.snap b/test/workers/package/__snapshots__/index.spec.js.snap
index 86a756298c..ec40674aae 100644
--- a/test/workers/package/__snapshots__/index.spec.js.snap
+++ b/test/workers/package/__snapshots__/index.spec.js.snap
@@ -13,6 +13,7 @@ Array [
"rebaseStalePrs",
"unpublishSafe",
"prCreation",
+ "prNotPendingHours",
"automerge",
"automergeType",
"requiredStatusChecks",
@@ -47,6 +48,7 @@ Array [
"rebaseStalePrs",
"unpublishSafe",
"prCreation",
+ "prNotPendingHours",
"automerge",
"automergeType",
"requiredStatusChecks",
@@ -204,6 +206,7 @@ Please make sure the following warnings are safe to ignore:
This {{#if isGitHub}}PR{{else}}MR{{/if}} has been generated by [Renovate Bot](https://renovateapp.com).",
"prCreation": "immediate",
+ "prNotPendingHours": 12,
"prTitle": "{{#if isPin}}Pin{{else}}{{#if isRollback}}Roll back{{else}}Update{{/if}}{{/if}} dependency {{depName}} to {{#if isRange}}{{newVersion}}{{else}}{{#if isMajor}}v{{newVersionMajor}}{{else}}v{{newVersion}}{{/if}}{{/if}}",
"rebaseStalePrs": false,
"recreateClosed": false,
@@ -357,6 +360,7 @@ Please make sure the following warnings are safe to ignore:
This {{#if isGitHub}}PR{{else}}MR{{/if}} has been generated by [Renovate Bot](https://renovateapp.com).",
"prCreation": "immediate",
+ "prNotPendingHours": 12,
"prTitle": "{{#if isPin}}Pin{{else}}{{#if isRollback}}Roll back{{else}}Update{{/if}}{{/if}} dependency {{depName}} to {{#if isRange}}{{newVersion}}{{else}}{{#if isMajor}}v{{newVersionMajor}}{{else}}v{{newVersion}}{{/if}}{{/if}}",
"rebaseStalePrs": false,
"recreateClosed": false,
diff --git a/test/workers/pr/index.spec.js b/test/workers/pr/index.spec.js
index 14b97df3b8..07a0b758ec 100644
--- a/test/workers/pr/index.spec.js
+++ b/test/workers/pr/index.spec.js
@@ -139,10 +139,21 @@ describe('workers/pr', () => {
});
it('should return null if waiting for not pending', async () => {
config.api.getBranchStatus = jest.fn(() => 'pending');
+ config.api.getBranchLastCommitTime = jest.fn(() => new Date());
config.prCreation = 'not-pending';
const pr = await prWorker.ensurePr(config);
expect(pr).toBe(null);
});
+ it('should create PR if pending timeout hit', async () => {
+ config.api.getBranchStatus = jest.fn(() => 'pending');
+ config.api.getBranchLastCommitTime = jest.fn(
+ () => new Date('2017-01-01')
+ );
+ config.prCreation = 'not-pending';
+ config.api.getBranchPr = jest.fn();
+ const pr = await prWorker.ensurePr(config);
+ expect(pr).toMatchObject({ displayNumber: 'New Pull Request' });
+ });
it('should create PR if no longer pending', async () => {
config.api.getBranchStatus = jest.fn(() => 'failed');
config.api.getBranchPr = jest.fn();
--
GitLab