diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index e0238364..24daaf12 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -291,7 +291,7 @@ module.exports = class Rulesets extends Diffable { this.log.debug(`Ruleset created successfully ${JSON.stringify(res.url)}`) return res }).catch(e => { - return this.handleError(e) + return this.handleDuplicateOrError(e, attrs) }) } else { if (this.nop) { @@ -305,11 +305,45 @@ module.exports = class Rulesets extends Diffable { this.log.debug(`Ruleset created successfully ${JSON.stringify(res.url)}`) return res }).catch(e => { - return this.handleError(e) + return this.handleDuplicateOrError(e, attrs) }) } } + // A ruleset create (POST) is not idempotent. Octokit's retry / auth-app + // layers can re-send a POST that already succeeded, and a repo can also be + // processed by two overlapping syncs (e.g. a full sync racing with the + // repository.created webhook). In both cases the second create hits + // "Name must be unique" (422) even though the ruleset now exists. Instead of + // failing the whole run, reconcile by looking the ruleset up by name and + // updating it in place so the create effectively becomes idempotent. + isDuplicateNameError (e) { + if (!e || e.status !== 422) return false + const errors = e.response && e.response.data && e.response.data.errors + const list = Array.isArray(errors) ? errors : [] + return list.some(err => { + const msg = typeof err === 'string' ? err : (err && err.message) + return typeof msg === 'string' && /name must be unique/i.test(msg) + }) + } + + handleDuplicateOrError (e, attrs) { + if (!this.isDuplicateNameError(e)) { + return this.handleError(e) + } + this.log.debug(`Ruleset '${attrs && attrs.name}' already exists (concurrent or retried create); reconciling by update`) + return this.find().then(existing => { + const match = Array.isArray(existing) + ? existing.find(record => this.comparator(record, attrs)) + : undefined + if (!match) { + return this.handleError(e) + } + const { id: _ignoredId, ...attrsWithoutId } = attrs || {} + return this.update(match, attrsWithoutId) + }).catch(err => this.handleError(err)) + } + remove (existing) { const parms = this.wrapAttrs(Object.assign({ id: existing.id })) if (this.scope === 'org') { diff --git a/lib/settings.js b/lib/settings.js index 02bb6526..d334ef6e 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -1161,6 +1161,18 @@ class Settings { }) } + // Full-sync NOP runs do not have the webhook fields needed to report to a + // check run. Keep potentially sensitive diff values at debug level and log + // only a value-free summary at info level. + if (!payload?.check_run || !payload?.repository) { + this.log.debug({ results: this.results }, 'Dry-run results') + const summary = this.results + .map(res => `${res.type} ${res.plugin} ${res.repo}: ${res.action?.msg ?? ''}`) + .join('\n') + this.log.info(`Dry-run finished with ${this.results.length} planned change(s); the full diff is logged at debug level.\n${summary}`) + return + } + let error = false const stats = { reposProcessed: {}, diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js index df72e0ce..c3cc475d 100644 --- a/test/unit/lib/plugins/rulesets.test.js +++ b/test/unit/lib/plugins/rulesets.test.js @@ -179,6 +179,84 @@ describe('Rulesets', () => { }) }) + describe('idempotent create when the ruleset already exists (retried/concurrent POST)', () => { + function duplicateNameError () { + const e = new Error('Validation Failed') + e.status = 422 + e.response = { data: { errors: ['Name must be unique'] } } + return e + } + + function wireRequest (routeResults) { + const calls = [] + const request = jest.fn().mockImplementation((route, body) => { + calls.push({ route, body }) + const handler = routeResults[route] + return handler ? handler() : Promise.resolve({ url: route }) + }) + request.endpoint = jest.fn().mockImplementation((route, body) => ({ url: route, body })) + request.endpoint.merge = jest.fn().mockImplementation((route, body) => ({ method: 'GET', url: route, ...body })) + github.request = request + return calls + } + + it('reconciles a repo ruleset by updating the existing one on 422 "Name must be unique"', async () => { + const attrs = generateRequestRuleset(0, 'synk', repo_conditions, []) + delete attrs.id + const existing = generateResponseRuleset(42, 'synk', repo_conditions, []) + const calls = wireRequest({ + 'POST /repos/{owner}/{repo}/rulesets': () => Promise.reject(duplicateNameError()) + }) + github.paginate = jest.fn() + .mockResolvedValueOnce([{ id: 42, name: 'synk', source_type: 'Repository' }]) + .mockResolvedValueOnce([existing]) + + const plugin = configure([attrs]) + await plugin.add(attrs) + + const put = calls.find(c => c.route === 'PUT /repos/{owner}/{repo}/rulesets/{id}') + expect(put).toBeDefined() + expect(put.body.id).toBe(42) + }) + + it('reconciles an org ruleset by updating the existing one on 422 "Name must be unique"', async () => { + const attrs = generateRequestRuleset(0, 'synk', org_conditions, [], true) + delete attrs.id + const existing = generateResponseRuleset(7, 'synk', org_conditions, [], true) + const calls = wireRequest({ + 'POST /orgs/{org}/rulesets': () => Promise.reject(duplicateNameError()) + }) + github.paginate = jest.fn() + .mockResolvedValueOnce([{ id: 7, name: 'synk', source_type: 'Organization' }]) + .mockResolvedValueOnce([existing]) + + const plugin = configure([attrs], 'org') + await plugin.add(attrs) + + const put = calls.find(c => c.route === 'PUT /orgs/{org}/rulesets/{id}') + expect(put).toBeDefined() + expect(put.body.id).toBe(7) + }) + + it('does not reconcile (surfaces the error) for a 422 that is not a name-uniqueness violation', async () => { + const attrs = generateRequestRuleset(0, 'synk', repo_conditions, []) + delete attrs.id + const other = new Error('Validation Failed') + other.status = 422 + other.response = { data: { errors: ['Something else is invalid'] } } + const calls = wireRequest({ + 'POST /repos/{owner}/{repo}/rulesets': () => Promise.reject(other) + }) + github.paginate = jest.fn() + + const plugin = configure([attrs]) + await plugin.add(attrs) + + expect(github.paginate).not.toHaveBeenCalled() + expect(calls.some(c => c.route === 'PUT /repos/{owner}/{repo}/rulesets/{id}')).toBe(false) + }) + }) + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exist in GitHub', () => { it('it initialises the status checks with an empty list', () => { // Mock the GitHub API response diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 93de139f..133ae753 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1057,6 +1057,35 @@ repository: expect(msgs.some(m => /teams/.test(m))).toBe(true) }) + it.each([ + ['without a check run', {}], + ['without a repository', { check_run: { id: 123 } }] + ])('28. full-sync dry run %s logs a value-free summary instead of updating a check run', async (_description, payload) => { + stubContext.payload = { installation: { id: 123 }, ...payload } + stubContext.octokit.checks = { update: jest.fn().mockResolvedValue({}) } + + const settings = new Settings(true, stubContext, mockRepo, {}, mockRef) + settings.results = [{ + type: 'INFO', + plugin: 'Variables', + repo: 'test/test-repo', + endpoint: '', + action: { + msg: 'Changes found', + additions: {}, + modifications: { MY_VAR: { value: 'plain-value' } }, + deletions: {} + } + }] + + await settings.handleResults() + + expect(stubContext.log.info).toHaveBeenCalledWith(expect.stringContaining('Changes found')) + expect(stubContext.log.info).not.toHaveBeenCalledWith(expect.stringContaining('plain-value')) + expect(stubContext.log.debug).toHaveBeenCalledWith({ results: settings.results }, 'Dry-run results') + expect(stubContext.octokit.checks.update).not.toHaveBeenCalled() + }) + it('28. base-config filtering preserves org-rulesets informational NopCommands', async () => { stubContext.payload.repository = { owner: { login: 'test' }, name: 'safe-settings' } stubContext.payload.check_run = { id: 123, check_suite: { pull_requests: [{ number: 456 }] } }