diff --git a/index.js b/index.js index 4ed6c2398..a041dcc82 100644 --- a/index.js +++ b/index.js @@ -21,6 +21,8 @@ let deploymentConfig module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => { let appSlug = 'safe-settings' + // Set by info() once the app has authenticated; undefined until then. + let appId // Cache of enterprise slug → enterprise installation id. Keyed by slug so a // cached id is never reused for a different enterprise (e.g. when the app // handles events from multiple enterprises). @@ -362,6 +364,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => const github = await robot.auth(installation.id) const app = await github.rest.apps.getAuthenticated() appSlug = app.data.slug + appId = app.data.id robot.log.info(`Validated the app is configured properly = \n${JSON.stringify(app.data, null, 2)}`) } @@ -731,7 +734,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => return syncAllSettings(false, updatedContext) }) - robot.on('check_suite.requested', async context => { + robot.on(['check_suite.requested', 'check_suite.rerequested'], async context => { const { payload } = context const { repository } = payload const adminRepo = repository.name === env.ADMIN_REPO @@ -769,7 +772,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => robot.log.debug('Not working on the Admin repo, returning...') return } - const defaultBranch = payload.pull_request.head_branch === repository.default_branch + const defaultBranch = payload.pull_request.head.ref === repository.default_branch if (defaultBranch) { robot.log.debug(' Working on the default branch, returning...') return @@ -791,7 +794,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => return } - const defaultBranch = payload.pull_request.head_branch === repository.default_branch + const defaultBranch = payload.pull_request.head.ref === repository.default_branch if (defaultBranch) { robot.log.debug(' Working on the default branch, returning...') return @@ -799,16 +802,6 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => return createCheckRun(context, pull_request, payload.pull_request.head.sha, payload.pull_request.head.ref) }) - robot.on(['check_suite.rerequested'], async context => { - robot.log.debug('Check suite was rerequested!') - return createCheckRun(context) - }) - - robot.on(['check_suite.rerequested'], async context => { - robot.log.debug('Check suite was rerequested!') - return createCheckRun(context) - }) - robot.on(['check_run.created'], async context => { robot.log.debug('Check run was created!') const { payload } = context @@ -822,6 +815,13 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => return } + // Another safe-settings deployment (e.g. prod and non-prod apps on the same org) + // creates runs with the same name, but only the creating app can update them. + if (appId && check_run.app && check_run.app.id !== appId) { + robot.log.debug(' Check run belongs to another app, returning...') + return + } + if (check_run.status === 'completed') { robot.log.debug(' Checkrun created as completed, returning') return @@ -852,8 +852,9 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => params = Object.assign(context.repo(), { pull_number: pull_request.number }) - const changes = await context.octokit.rest.pulls.listFiles(params) - const files = changes.data.map(f => { return f.filename }) + // listFiles returns at most 30 files per page (and 3,000 files in total), so paginate. + const changes = await context.octokit.paginate(context.octokit.rest.pulls.listFiles, { ...params, per_page: 100 }) + const files = changes.map(f => { return f.filename }) const settingsModified = files.includes(Settings.FILE_PATH) const repoChanges = getChangedRepoConfigName(files, context.repo().owner) diff --git a/lib/plugins/repository.js b/lib/plugins/repository.js index 2627b39c4..745ef3c85 100644 --- a/lib/plugins/repository.js +++ b/lib/plugins/repository.js @@ -129,11 +129,12 @@ module.exports = class Repository extends ErrorStash { } else { promises.push(this.updateSecurity(resp.data, resArray)) } - if (this.nop) { - return Promise.resolve(resArray) - } else { - return Promise.all(promises) - } + // Settle every side effect in both modes. In NOP mode they used to be left + // unawaited, so a failure (e.g. creating the default branch of an empty repo) + // became an unhandled rejection; in apply mode it fell through to the 404 + // handler below and was mistaken for a missing repo. + const settled = Promise.all(promises).catch(e => this.handleSideEffectError(e, resArray)) + return this.nop ? settled.then(() => resArray) : settled }).catch(e => { if (e.status === 404) { if (this.force_create) { @@ -175,6 +176,14 @@ module.exports = class Repository extends ErrorStash { }) } + handleSideEffectError (e, resArray) { + const msg = `Error applying repository settings for ${this.repo.owner}/${this.repo.repo}: ${e && e.message ? e.message : JSON.stringify(e)}` + if (this.nop) { + resArray.push(new NopCommand(this.constructor.name, this.repo, null, msg, 'ERROR')) + } + this.logError(msg) + } + updateDefaultBranch (oldname, newname, resArray) { this.log.debug(`Checking if ${newname} is already a branch`) return this.github.rest.repos.getBranch({ diff --git a/lib/settings.js b/lib/settings.js index e78fc49e3..f1b44d28a 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -1275,6 +1275,7 @@ class Settings { const renderedCommentMessage = await eta.renderString(commetMessageTemplate, stats) + let commentError if (env.CREATE_PR_COMMENT === 'true') { const pluginSectionList = renderChangeSections(stats.changeSections) @@ -1357,14 +1358,20 @@ class Settings { const totalPages = pages.length const pullRequest = payload.check_run.check_suite.pull_requests[0] - for (let i = 0; i < pages.length; i++) { - const body = `${makeHeader(i + 1, totalPages)}${pages[i]}` - await this.github.rest.issues.createComment({ - owner: payload.repository.owner.login, - repo: payload.repository.name, - issue_number: pullRequest.number, - body: truncateWithSuffix(body, COMMENT_LIMIT - footer.length, '... (too many changes to report)', Boolean(footer)) + footer - }) + // A failed comment (size limit, 404, rate limit) must not leave the check run in_progress. + try { + for (let i = 0; i < pages.length; i++) { + const body = `${makeHeader(i + 1, totalPages)}${pages[i]}` + await this.github.rest.issues.createComment({ + owner: payload.repository.owner.login, + repo: payload.repository.name, + issue_number: pullRequest.number, + body: truncateWithSuffix(body, COMMENT_LIMIT - footer.length, '... (too many changes to report)', Boolean(footer)) + footer + }) + } + } catch (e) { + commentError = e + this.log.error(`Failed to comment on the PR with the dry-run results: ${e.message || JSON.stringify(e)}`) } } @@ -1380,6 +1387,9 @@ class Settings { summary: truncateWithSuffix(renderedCommentMessage, COMMENT_LIMIT, '... (too many changes to report)') } } + if (commentError) { + params.output.text = `Posting the dry-run results as a PR comment failed: ${commentError.message || JSON.stringify(commentError)}` + } this.log.debug(`Completing check run ${JSON.stringify(params)}`) await this.github.rest.checks.update(params) diff --git a/test/integration/transport/comment-markup.test.js b/test/integration/transport/comment-markup.test.js index 64b0928b7..e7263cdcc 100644 --- a/test/integration/transport/comment-markup.test.js +++ b/test/integration/transport/comment-markup.test.js @@ -127,18 +127,29 @@ test('comments-disabled mode sends only a completed check', async () => { assert(output.summary.includes('`description`')) }) -for (const failure of ['comment', 'check']) { - test(`real transport propagates ${failure} failure instead of reporting success`, async () => { - const scope = nock(apiUrl).post('/repos/test-org/report-repo/issues/7/comments') - if (failure === 'comment') { - scope.reply(422, { message: 'Comment rejected' }) - } else { - scope.reply(201, { id: 100 }) - .patch('/repos/test-org/report-repo/check-runs/42').reply(422, { message: 'Check rejected' }) - } - await assert.rejects((await reporter([change()])).handleResults(), error => error.status === 422) - }) -} +test('real transport completes the check and reports the error when the comment fails', async () => { + let check + nock(apiUrl) + .post('/repos/test-org/report-repo/issues/7/comments').reply(422, { message: 'Comment rejected' }) + .patch('/repos/test-org/report-repo/check-runs/42') + .reply((_uri, body) => { check = body; return [200, { id: 42, ...body }] }) + const errors = [] + const settings = await reporter([change()]) + settings.log.error = message => errors.push(message) + + await settings.handleResults() + + assert.equal(check.status, 'completed') + assert.match(check.output.text, /Comment rejected/) + assert.equal(errors.length, 1) + assert.match(errors[0], /Failed to comment on the PR/) +}) + +test('real transport propagates check failure instead of reporting success', async () => { + nock(apiUrl).post('/repos/test-org/report-repo/issues/7/comments').reply(201, { id: 100 }) + .patch('/repos/test-org/report-repo/check-runs/42').reply(422, { message: 'Check rejected' }) + await assert.rejects((await reporter([change()])).handleResults(), error => error.status === 422) +}) function credentials (t) { const originalEnv = process.env diff --git a/test/integration/transport/pr-comment-summary.test.js b/test/integration/transport/pr-comment-summary.test.js index 8f4ae522e..19d7d7d40 100644 --- a/test/integration/transport/pr-comment-summary.test.js +++ b/test/integration/transport/pr-comment-summary.test.js @@ -141,7 +141,14 @@ for (const failAt of [ test(`phase 27 preserves flags and removes only a successfully created fixture after ${failAt} fails`, async t => { const state = await fixture(t, { failAt }) const flags = { ...env } - await assert.rejects(state.run, /Controlled reporting failure/) + const commentFails = failAt.endsWith('/comments') + // handleResults() completes the check when commenting fails, so the phase's own + // comment-count assertion is what stops the run. + await assert.rejects(state.run, commentFails ? /expected comment page count/ : /Controlled reporting failure/) + if (commentFails) { + assert(state.checks.length > 0) + assert(state.checks.every(check => check.status === 'completed')) + } assert.deepEqual(env, flags) assert.equal(state.deleted(), failAt !== 'POST /orgs/test-org/repos') assert(!state.logs.includes('Phase 27 complete')) diff --git a/test/unit/index-check-runs.test.js b/test/unit/index-check-runs.test.js new file mode 100644 index 000000000..eba214fbd --- /dev/null +++ b/test/unit/index-check-runs.test.js @@ -0,0 +1,173 @@ +const fs = require('fs') +const plugin = require('../../index') +const Settings = require('../../lib/settings') +const env = require('../../lib/env') + +describe('validator check run handlers', () => { + let handlers + let github + let log + + const repository = { name: 'admin', default_branch: 'main', owner: { login: 'test-org' } } + + beforeEach(async () => { + jest.replaceProperty(process, 'env', { ...process.env, CRON: '', GH_ENTERPRISE: '' }) + jest.replaceProperty(env, 'ADMIN_REPO', 'admin') + jest.spyOn(fs, 'existsSync').mockReturnValue(false) + handlers = new Map() + const installation = { id: 123, account: { login: 'test-org' } } + github = { + paginate: jest.fn(async (method, params) => { + if (method === github.rest.pulls.listFiles) return method(params) + return [installation] + }), + rest: { + apps: { + listInstallations: { endpoint: { merge: jest.fn().mockReturnValue({}) } }, + getAuthenticated: jest.fn().mockResolvedValue({ data: { id: 1, slug: 'safe-settings' } }) + }, + repos: { getContent: jest.fn().mockResolvedValue({ data: { content: '' } }) }, + pulls: { listFiles: jest.fn().mockResolvedValue([]) }, + checks: { + create: jest.fn().mockResolvedValue({}), + update: jest.fn().mockResolvedValue({}) + } + } + } + log = { debug: jest.fn(), info: jest.fn(), error: jest.fn(), trace: jest.fn() } + const robot = { + auth: jest.fn().mockResolvedValue(github), + log, + on: (events, handler) => { + for (const event of [].concat(events)) { + if (!handlers.has(event)) handlers.set(event, []) + handlers.get(event).push(handler) + } + } + } + plugin(robot, {}, Settings) + // Let the startup info() call resolve the app's own identity. + await new Promise(resolve => setImmediate(resolve)) + }) + + afterEach(() => { + jest.restoreAllMocks() + }) + + async function receive (event, payload) { + const context = { + payload, + octokit: github, + log, + repo: () => ({ owner: 'test-org', repo: payload.repository.name }) + } + for (const handler of handlers.get(event) || []) { + await handler(context) + } + } + + describe('check_suite.rerequested', () => { + function suite (overrides = {}) { + return { + repository, + check_suite: { + head_branch: 'config-change', + head_sha: 'abc123', + pull_requests: [{ number: 1 }], + ...overrides + } + } + } + + it('creates one check run for the suite head_sha', async () => { + await receive('check_suite.rerequested', suite()) + + expect(github.rest.checks.create).toHaveBeenCalledTimes(1) + expect(github.rest.checks.create).toHaveBeenCalledWith({ + owner: 'test-org', + repo: 'admin', + name: 'Safe-setting validator', + head_sha: 'abc123' + }) + }) + + it('ignores suites outside the admin repo', async () => { + await receive('check_suite.rerequested', { ...suite(), repository: { ...repository, name: 'other' } }) + + expect(github.rest.checks.create).not.toHaveBeenCalled() + }) + + it('ignores suites without a PR', async () => { + await receive('check_suite.rerequested', suite({ pull_requests: [] })) + + expect(github.rest.checks.create).not.toHaveBeenCalled() + }) + }) + + describe.each(['pull_request.opened', 'pull_request.reopened'])('%s', event => { + function pr (ref) { + return { repository, pull_request: { number: 1, head: { ref, sha: 'abc123' } } } + } + + it('creates a check run for a feature branch', async () => { + await receive(event, pr('config-change')) + + expect(github.rest.checks.create).toHaveBeenCalledWith(expect.objectContaining({ head_sha: 'abc123' })) + }) + + // Behavior change: this guard used to read the non-existent pull_request.head_branch + // and never fired, so PRs from the default branch got a check run too. + it('skips PRs whose head is the default branch', async () => { + await receive(event, pr('main')) + + expect(github.rest.checks.create).not.toHaveBeenCalled() + }) + }) + + describe('check_run.created', () => { + function run (app) { + return { + repository, + check_run: { + id: 42, + name: 'Safe-setting validator', + status: 'queued', + app, + check_suite: { + pull_requests: [{ number: 1, head: { ref: 'config-change' }, base: { ref: 'main' } }] + } + } + } + } + + it('ignores check runs created by another app', async () => { + await receive('check_run.created', run({ id: 2, slug: 'safe-settings-nonprod' })) + + expect(github.rest.checks.update).not.toHaveBeenCalled() + }) + + it('processes check runs created by this app', async () => { + await receive('check_run.created', run({ id: 1, slug: 'safe-settings' })) + + expect(github.rest.checks.update).toHaveBeenCalledWith(expect.objectContaining({ check_run_id: 42, status: 'in_progress' })) + expect(github.rest.checks.update).toHaveBeenLastCalledWith(expect.objectContaining({ check_run_id: 42, status: 'completed' })) + }) + + it('syncs a settings file that is only on the second page of changed files', async () => { + const syncSelectedRepos = jest.spyOn(Settings, 'syncSelectedRepos').mockResolvedValue() + const page1 = Array.from({ length: 100 }, (_, i) => ({ filename: `docs/file-${i}.md` })) + const page2 = [{ filename: `${env.CONFIG_PATH}/repos/my-repo.yml` }] + github.rest.pulls.listFiles.mockResolvedValue({ data: page1.slice(0, 30) }) + github.paginate.mockImplementation(async (method, params) => { + if (method === github.rest.pulls.listFiles) return [...page1, ...page2] + return [] + }) + + await receive('check_run.created', run({ id: 1, slug: 'safe-settings' })) + + expect(github.paginate).toHaveBeenCalledWith(github.rest.pulls.listFiles, expect.objectContaining({ pull_number: 1, per_page: 100 })) + expect(syncSelectedRepos).toHaveBeenCalledTimes(1) + expect(syncSelectedRepos.mock.calls[0][2]).toEqual([{ repo: 'my-repo', owner: 'test-org' }]) + }) + }) +}) diff --git a/test/unit/index-config-errors.test.js b/test/unit/index-config-errors.test.js index ac6a42815..a3a7fbf4e 100644 --- a/test/unit/index-config-errors.test.js +++ b/test/unit/index-config-errors.test.js @@ -20,7 +20,7 @@ describe('configuration read error reporting', () => { handlers = new Map() const installation = { id: 123, account: { login: 'test-org' } } github = { - paginate: jest.fn().mockResolvedValue([installation]), + paginate: jest.fn(async (method, params) => method === github.rest.pulls.listFiles ? (await method(params)).data : [installation]), rest: { apps: { listInstallations: { endpoint: { merge: jest.fn().mockReturnValue({}) } }, diff --git a/test/unit/lib/plugins/repository.test.js b/test/unit/lib/plugins/repository.test.js index 3ade857b3..582057543 100644 --- a/test/unit/lib/plugins/repository.test.js +++ b/test/unit/lib/plugins/repository.test.js @@ -181,4 +181,97 @@ describe('Repository', () => { }) }) }) + + describe('default branch creation failures', () => { + const originalFlag = env.CREATE_DEFAULT_BRANCH + let client + + function withEndpoint (name) { + return Object.assign(jest.fn().mockResolvedValue({}), { + endpoint: jest.fn(params => ({ url: `/${name}`, body: params })) + }) + } + + function branch (name, sha) { + return { data: { name, commit: { sha } } } + } + + beforeEach(() => { + jest.clearAllMocks() + env.CREATE_DEFAULT_BRANCH = true + client = { + rest: { + repos: { + // An empty repo still reports a default branch, but the branch itself 404s. + get: jest.fn().mockResolvedValue({ + data: { name: 'test', default_branch: 'main', topics: [], owner: { login: 'bkeepers' } } + }), + getBranch: jest.fn().mockRejectedValue({ status: 404 }), + update: withEndpoint('update'), + createInOrg: withEndpoint('createInOrg'), + replaceAllTopics: withEndpoint('replaceAllTopics') + }, + git: { + createRef: withEndpoint('createRef') + } + } + } + }) + + afterEach(() => { + env.CREATE_DEFAULT_BRANCH = originalFlag + }) + + function build (nop, config, errors = []) { + return new Repository(nop, client, { owner: 'bkeepers', repo: 'test' }, config, 1, log, errors) + } + + it('reports an ERROR in NOP mode when the repo is empty instead of rejecting unhandled', async () => { + const errors = [] + const plugin = build(true, { default_branch: 'develop', force_create: true }, errors) + + const res = await plugin.sync() + + expect(res).toEqual(expect.arrayContaining([expect.objectContaining({ type: 'ERROR', plugin: 'Repository' })])) + expect(errors.map(e => e.msg)).toEqual(expect.arrayContaining([ + expect.stringContaining('Error creating default branch develop from main') + ])) + expect(client.rest.repos.createInOrg).not.toHaveBeenCalled() + }) + + it('includes the default branch NOP commands that resolve after sync() starts', async () => { + client.rest.repos.getBranch + .mockRejectedValueOnce({ status: 404 }) + .mockImplementationOnce(() => new Promise(resolve => setTimeout(() => resolve(branch('main', 'deadbeef')), 10))) + const plugin = build(true, { default_branch: 'develop' }) + + const res = await plugin.sync() + + const msgs = res.map(r => r.action.msg) + expect(msgs).toContain('Create default branch develop from main') + expect(msgs).toContain('Set default branch to develop') + }) + + it('does not try to create an existing empty repo in apply mode', async () => { + const errors = [] + const plugin = build(false, { default_branch: 'develop', force_create: true }, errors) + + await plugin.sync() + + expect(client.rest.repos.createInOrg).not.toHaveBeenCalled() + expect(client.rest.git.createRef).not.toHaveBeenCalled() + expect(errors.length).toBeGreaterThan(0) + expect(log.error).toHaveBeenCalledWith(expect.stringContaining('Error creating default branch develop from main')) + }) + + it('still plans a single repo creation when the repo is missing', async () => { + client.rest.repos.get.mockRejectedValue({ status: 404 }) + const plugin = build(true, { default_branch: 'develop', force_create: true }) + + const res = await plugin.sync() + + expect(res).toHaveLength(1) + expect(res[0].action.msg).toBe('Create Repo') + }) + }) }) diff --git a/test/unit/lib/settings-results.test.js b/test/unit/lib/settings-results.test.js index 017f86d95..be3f0b6e2 100644 --- a/test/unit/lib/settings-results.test.js +++ b/test/unit/lib/settings-results.test.js @@ -628,3 +628,36 @@ describe('Settings result deduplication', () => { }) }) }) + +describe('Settings PR comment failures', () => { + const repo = { owner: 'test', repo: 'test-repo' } + + it('still completes the check run when the PR comment fails', async () => { + jest.replaceProperty(env, 'CREATE_PR_COMMENT', 'true') + const context = { + payload: { + installation: { id: 123 }, + repository: { owner: { login: 'test' }, name: 'admin' }, + check_run: { id: 42, check_suite: { pull_requests: [{ number: 1 }] } } + }, + octokit: { + rest: { + checks: { update: jest.fn().mockResolvedValue({}) }, + issues: { createComment: jest.fn().mockRejectedValue(Object.assign(new Error('Validation Failed'), { status: 422 })) } + } + }, + log: { debug: jest.fn(), info: jest.fn(), error: jest.fn() } + } + const settings = new Settings(true, context, repo, {}, 'main') + settings.appendToResults([new NopCommand('Branches', repo, null, 'Branch protection changes')]) + + await settings.handleResults() + + expect(context.octokit.rest.issues.createComment).toHaveBeenCalled() + expect(context.octokit.rest.checks.update).toHaveBeenCalledTimes(1) + const params = context.octokit.rest.checks.update.mock.calls[0][0] + expect(params).toMatchObject({ check_run_id: 42, status: 'completed' }) + expect(params.output.text).toContain('Validation Failed') + expect(context.log.error).toHaveBeenCalledWith(expect.stringContaining('Validation Failed')) + }) +})