From f980fea6ca5bbf3fd39ed49f61ae69c74b8d4800 Mon Sep 17 00:00:00 2001 From: Rhys Arkins <rhys@keylocation.sg> Date: Wed, 18 Oct 2017 15:28:51 +0200 Subject: [PATCH] feat: comment in closed PR when it is blocking an update (github) (#981) This feature adds a new behaviour to Renovate, where it will now add a comment to any existing closed PRs that are "blocking" currently valid updates. A new API function "ensureComment" has been added, its purpose is so we can ensure there exists only one comment with a certain subject/heading. This lets us prevent duplicates as well as update existing, without having to keep state about it. ensureComment needs porting to gitlab, but should be quite easy. --- lib/api/github.js | 50 ++++++++++++++++++++++ lib/api/gitlab.js | 5 +++ lib/workers/branch/check-existing.js | 19 ++++---- lib/workers/branch/index.js | 12 +++++- test/api/__snapshots__/github.spec.js.snap | 32 ++++++++++++++ test/api/github.spec.js | 28 ++++++++++++ test/api/gitlab.spec.js | 5 +++ test/workers/branch/check-existing.spec.js | 14 +++--- test/workers/branch/index.spec.js | 16 +++++-- 9 files changed, 159 insertions(+), 22 deletions(-) diff --git a/lib/api/github.js b/lib/api/github.js index 5f901a568d..0bfc19de08 100644 --- a/lib/api/github.js +++ b/lib/api/github.js @@ -29,6 +29,10 @@ module.exports = { addAssignees, addReviewers, addLabels, + getComments, + addComment, + editComment, + ensureComment, // PR getPrList, findPr, @@ -471,6 +475,52 @@ async function addLabels(issueNo, labels) { }); } +async function getComments(issueNo) { + // GET /repos/:owner/:repo/issues/:number/comments + logger.debug(`Getting comments for #${issueNo}`); + const url = `repos/${config.repoName}/issues/${issueNo}/comments?per_page=100`; + const comments = (await get(url, { paginate: true })).body; + logger.debug(`Found ${comments.length} comments`); + return comments; +} + +async function addComment(issueNo, body) { + // POST /repos/:owner/:repo/issues/:number/comments + await get.post(`repos/${config.repoName}/issues/${issueNo}/comments`, { + body: { body }, + }); +} + +async function editComment(commentId, body) { + // PATCH /repos/:owner/:repo/issues/comments/:id + await get.patch(`repos/${config.repoName}/issues/comments/${commentId}`, { + body: { body }, + }); +} + +async function ensureComment(issueNo, topic, content) { + logger.debug(`Ensuring comment "${topic}" in #${issueNo}`); + const body = `### ${topic}\n\n${content}`; + const comments = await getComments(issueNo); + let commentId; + let commentNeedsUpdating; + comments.forEach(comment => { + if (comment.body.startsWith(`### ${topic}\n\n`)) { + commentId = comment.id; + commentNeedsUpdating = comment.body !== body; + } + }); + if (!commentId) { + await addComment(issueNo, body); + logger.info({ repository: config.repoName, issueNo }, 'Added comment'); + } else if (commentNeedsUpdating) { + await editComment(commentId, body); + logger.info({ repository: config.repoName, issueNo }, 'Updated comment'); + } else { + logger.debug('Comment is already update-to-date'); + } +} + // Pull Request async function getPrList() { diff --git a/lib/api/gitlab.js b/lib/api/gitlab.js index 4ff6a291b9..f30a6c1712 100644 --- a/lib/api/gitlab.js +++ b/lib/api/gitlab.js @@ -23,6 +23,7 @@ module.exports = { addAssignees, addReviewers, addLabels, + ensureComment, // PR findPr, createPr, @@ -337,6 +338,10 @@ async function addLabels(prNo, labels) { await get.put(url); } +async function ensureComment() { + // Todo: implement. See GitHub API for example +} + async function findPr(branchName, prTitle, state = 'all') { logger.debug(`findPr(${branchName}, ${prTitle}, ${state})`); const urlString = `projects/${config.repoName}/merge_requests?state=${state}&per_page=100`; diff --git a/lib/workers/branch/check-existing.js b/lib/workers/branch/check-existing.js index 8557831f53..f07064ea80 100644 --- a/lib/workers/branch/check-existing.js +++ b/lib/workers/branch/check-existing.js @@ -9,16 +9,12 @@ async function prAlreadyExisted(config) { logger.trace({ config }, 'prAlreadyExisted'); if (config.recreateClosed) { logger.debug('recreateClosed is true'); - return false; + return null; } logger.debug('recreateClosed is false'); // Return if same PR already existed // Check for current PR title format - const pr = await config.api.findPr( - config.branchName, - config.prTitle, - 'closed' - ); + let pr = await config.api.findPr(config.branchName, config.prTitle, 'closed'); if (pr) { logger.debug('Found closed PR with current title'); // this code exists to ignore mistakenly closed PRs which occurred due to a bug @@ -31,19 +27,20 @@ async function prAlreadyExisted(config) { { closedAt, problemStart, problemStopped }, 'Ignoring mistakenly closed PR' ); - return false; + return null; } - return true; + return pr; } // Check for legacy PR title format // TODO: remove this in v10 const legacyPrTitle = config.prTitle .replace(/to v(\d+)$/, 'to version $1.x') // Major .replace(/to v(\d+)/, 'to version $1'); // Non-major - if (await config.api.findPr(config.branchName, legacyPrTitle, 'closed')) { + pr = await config.api.findPr(config.branchName, legacyPrTitle, 'closed'); + if (pr) { logger.debug('Found closed PR with legacy title'); - return true; + return pr; } logger.debug('prAlreadyExisted=false'); - return false; + return null; } diff --git a/lib/workers/branch/index.js b/lib/workers/branch/index.js index e39457929d..9a2ab3ac37 100644 --- a/lib/workers/branch/index.js +++ b/lib/workers/branch/index.js @@ -45,8 +45,18 @@ async function processBranch(branchConfig) { logger.info(`Branch has ${dependencies.length} upgrade(s)`); - if (await prAlreadyExisted(config)) { + const pr = await prAlreadyExisted(config); + if (pr) { logger.info('Closed PR already exists. Skipping branch.'); + const subject = 'Renovate Ignore Notification'; + let content = + 'Renovate will now ignore this PR, as it has been closed. If this was a mistake or you changed your mind, you can simply reopen or rename this PR to reactivate Renovate.'; + if (config.isMajor) { + content += `\n\nYou will otherwise not receive PRs for any releases of this major version (${config.newVersionMajor}.x) unless you upgrade it manually yourself. If Renovate later detects that it has been manuall upgraded, it will resume creating PRs for minor and patch upgrades and you do not need to reopen this PR.`; + } else { + content += `\n\n**Note**: The ignore applies only to this specific version (${config.newVersion}), so you will still receive a PR once a newer version is released. If you wish to permanently ignore this dependency, please add it to the \`ignoreDeps\` array of your renovate config.`; + } + await config.api.ensureComment(pr.number, subject, content); return 'already-existed'; } config.parentBranch = await getParentBranch(config); diff --git a/test/api/__snapshots__/github.spec.js.snap b/test/api/__snapshots__/github.spec.js.snap index f4108e4235..053adefe64 100644 --- a/test/api/__snapshots__/github.spec.js.snap +++ b/test/api/__snapshots__/github.spec.js.snap @@ -432,6 +432,38 @@ Array [ ] `; +exports[`api/github ensureComment add comment if not found 1`] = ` +Array [ + Array [ + "repos/some/repo/issues/42/comments", + Object { + "body": Object { + "body": "### some-subject + +some +content", + }, + }, + ], +] +`; + +exports[`api/github ensureComment add updates comment if necessary 1`] = ` +Array [ + Array [ + "repos/some/repo/issues/comments/1234", + Object { + "body": Object { + "body": "### some-subject + +some +content", + }, + }, + ], +] +`; + exports[`api/github findFilePaths(fileName) should return the files matching the fileName 1`] = ` Array [ "package.json", diff --git a/test/api/github.spec.js b/test/api/github.spec.js index 92e48fc18a..262abd3969 100644 --- a/test/api/github.spec.js +++ b/test/api/github.spec.js @@ -968,6 +968,34 @@ describe('api/github', () => { expect(get.post.mock.calls).toMatchSnapshot(); }); }); + describe('ensureComment', () => { + it('add comment if not found', async () => { + await initRepo('some/repo', 'token'); + get.mockReturnValueOnce({ body: [] }); + await github.ensureComment(42, 'some-subject', 'some\ncontent'); + expect(get.post.mock.calls).toHaveLength(1); + expect(get.post.mock.calls).toMatchSnapshot(); + }); + it('add updates comment if necessary', async () => { + await initRepo('some/repo', 'token'); + get.mockReturnValueOnce({ + body: [{ id: 1234, body: '### some-subject\n\nblablabla' }], + }); + await github.ensureComment(42, 'some-subject', 'some\ncontent'); + expect(get.post.mock.calls).toHaveLength(0); + expect(get.patch.mock.calls).toHaveLength(1); + expect(get.patch.mock.calls).toMatchSnapshot(); + }); + it('skips comment', async () => { + await initRepo('some/repo', 'token'); + get.mockReturnValueOnce({ + body: [{ id: 1234, body: '### some-subject\n\nsome\ncontent' }], + }); + await github.ensureComment(42, 'some-subject', 'some\ncontent'); + expect(get.post.mock.calls).toHaveLength(0); + expect(get.patch.mock.calls).toHaveLength(0); + }); + }); describe('findPr(branchName, prTitle, state)', () => { it('returns true if no title and all state', async () => { get.mockReturnValueOnce({ diff --git a/test/api/gitlab.spec.js b/test/api/gitlab.spec.js index 78f6b5f133..72d71f7f99 100644 --- a/test/api/gitlab.spec.js +++ b/test/api/gitlab.spec.js @@ -488,6 +488,11 @@ describe('api/gitlab', () => { expect(get.put.mock.calls).toMatchSnapshot(); }); }); + describe('ensureComment', () => { + it('exists', async () => { + await gitlab.ensureComment(42, 'some-subject', 'some\ncontent'); + }); + }); describe('findPr(branchName, prTitle, state)', () => { it('returns null if no results', async () => { get.mockReturnValueOnce({ diff --git a/test/workers/branch/check-existing.spec.js b/test/workers/branch/check-existing.spec.js index 2bf417c0b5..2f8367a417 100644 --- a/test/workers/branch/check-existing.spec.js +++ b/test/workers/branch/check-existing.spec.js @@ -18,30 +18,30 @@ describe('workers/branch/check-existing', () => { }); it('returns false if recreating closed PRs', async () => { config.recreateClosed = true; - expect(await prAlreadyExisted(config)).toBe(false); + expect(await prAlreadyExisted(config)).toBe(null); expect(config.api.findPr.mock.calls.length).toBe(0); }); it('returns false if both checks miss', async () => { config.recreatedClosed = true; - expect(await prAlreadyExisted(config)).toBe(false); + expect(await prAlreadyExisted(config)).toBe(null); expect(config.api.findPr.mock.calls.length).toBe(2); }); it('returns true if first check hits', async () => { - config.api.findPr.mockReturnValueOnce({}); - expect(await prAlreadyExisted(config)).toBe(true); + config.api.findPr.mockReturnValueOnce({ number: 12 }); + expect(await prAlreadyExisted(config)).toEqual({ number: 12 }); expect(config.api.findPr.mock.calls.length).toBe(1); }); it('returns true if second check hits', async () => { config.api.findPr.mockReturnValueOnce(null); - config.api.findPr.mockReturnValueOnce({}); - expect(await prAlreadyExisted(config)).toBe(true); + config.api.findPr.mockReturnValueOnce({ number: 13 }); + expect(await prAlreadyExisted(config)).toEqual({ number: 13 }); expect(config.api.findPr.mock.calls.length).toBe(2); }); it('returns false if mistaken', async () => { config.api.findPr.mockReturnValueOnce({ closed_at: '2017-10-15T21:28:07.000Z', }); - expect(await prAlreadyExisted(config)).toBe(false); + expect(await prAlreadyExisted(config)).toBe(null); }); }); }); diff --git a/test/workers/branch/index.spec.js b/test/workers/branch/index.spec.js index 8f1a56eac5..e6cee06da6 100644 --- a/test/workers/branch/index.spec.js +++ b/test/workers/branch/index.spec.js @@ -29,7 +29,7 @@ describe('workers/branch', () => { beforeEach(() => { config = { ...defaultConfig, - api: { branchExists: jest.fn() }, + api: { branchExists: jest.fn(), ensureComment: jest.fn() }, errors: [], warnings: [], logger, @@ -49,12 +49,22 @@ describe('workers/branch', () => { await branchWorker.processBranch(config); expect(checkExisting.prAlreadyExisted.mock.calls).toHaveLength(0); }); - it('skips branch if closed PR found', async () => { + it('skips branch if closed major PR found', async () => { schedule.isScheduledNow.mockReturnValueOnce(false); config.api.branchExists.mockReturnValueOnce(true); - checkExisting.prAlreadyExisted.mockReturnValueOnce(true); + config.isMajor = true; + checkExisting.prAlreadyExisted.mockReturnValueOnce({ number: 13 }); await branchWorker.processBranch(config); expect(parent.getParentBranch.mock.calls.length).toBe(0); + expect(config.logger.error.mock.calls).toHaveLength(0); + }); + it('skips branch if closed minor PR found', async () => { + schedule.isScheduledNow.mockReturnValueOnce(false); + config.api.branchExists.mockReturnValueOnce(true); + checkExisting.prAlreadyExisted.mockReturnValueOnce({ number: 13 }); + await branchWorker.processBranch(config); + expect(parent.getParentBranch.mock.calls.length).toBe(0); + expect(config.logger.error.mock.calls).toHaveLength(0); }); it('returns if no branch exists', async () => { packageFiles.getUpdatedPackageFiles.mockReturnValueOnce([]); -- GitLab