diff --git a/Dockerfile b/Dockerfile index 1da906ff3..44a5df652 100644 --- a/Dockerfile +++ b/Dockerfile @@ -11,6 +11,7 @@ LABEL version="1.0" \ ## to the image to be as small as possible COPY package*.json /opt/safe-settings/ COPY index.js /opt/safe-settings/ +COPY full-sync.js /opt/safe-settings/ COPY lib /opt/safe-settings/lib ## Install the app and dependencies diff --git a/README.md b/README.md index 00c2af198..c9147bd07 100644 --- a/README.md +++ b/README.md @@ -205,6 +205,13 @@ When a repo-level change (a push to `.github/repos/.yml`, or a `repository To handle this, after applying a repo-yml change `safe-settings` re-evaluates the repo's suborg membership. If the matched suborg source set changed, it runs the repo through the apply pipeline a second time so newly matched suborg settings are applied and settings from a no-longer-matching suborg can be removed in the same sync. +For a repository that does not exist yet, team and custom-property membership +lookups are deferred until after creation. A 404 from a membership endpoint is +treated as an unmatched selector only when the repository itself also returns +404; other lookup failures remain errors. Name-based `suborgrepos` matching still +applies before creation. This allows `force_create` validation and apply runs to +work when earlier changes have already configured team- or property-based suborgs. + **Scope:** Re-evaluation runs only on the repo-yml change paths (`Settings.sync` and the per-repo loop of `Settings.syncSelectedRepos`). Global settings changes (`syncAll`) and suborg-yml changes (`syncSubOrgs`) already iterate all relevant repos and do not need it. **Loop prevention.** Two guards prevent infinite re-evaluation: diff --git a/docs/docker-debugging.md b/docs/docker-debugging.md index 6a917a644..a4d6d5b3e 100644 --- a/docs/docker-debugging.md +++ b/docs/docker-debugging.md @@ -32,6 +32,14 @@ Build the local image: docker build -t safe-settings:local . ``` +The image also includes the one-shot full-sync entrypoint. To preview a sync +without applying settings or starting the webhook server: + +```bash +docker run --rm --env-file ./.env --env FULL_SYNC_NOP=true \ + safe-settings:local npm run full-sync +``` + Run container in foreground with explicit runtime env and port mapping: ```bash diff --git a/docs/github-action.md b/docs/github-action.md index 5424b7cc8..10c284497 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -10,6 +10,14 @@ Follow the [Create the GitHub App](deploy.md#create-the-github-app) guide to cre ## Defining the GitHub Action Workflow Running a full-sync with `safe-settings` can be done via `npm run full-sync`. This requires installing Node, such as with [actions/setup-node](https://github.com/actions/setup-node) (see example below). When doing so, the appropriate environment variables must be set (see the [Environment variables](#environment-variables) document for more details). +Installation repositories are processed in batches of up to ten, with each batch +settling before the next starts. A repository failure does not stop later +repositories from being processed. Failures are logged with the repository name +and retained in the sync errors, so a partial failure still produces a failed +check and a nonzero full-sync exit status. Dry runs report these errors alongside +planned changes. The same batching applies when scanning installation repositories +for suborg configuration changes; existing repository restrictions still apply. + ### Example GHA Workflow The below example uses the GHA "cron" feature to run a full-sync every 4 hours. While not required, this example uses the `.github` repo as the `admin` repo (set via `ADMIN_REPO` env var) and the safe-settings configurations are stored in the `safe-settings/` directory (set via `CONFIG_PATH` and `DEPLOYMENT_CONFIG_FILE`). diff --git a/lib/settings.js b/lib/settings.js index f1527df88..bdff9270e 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -1061,11 +1061,11 @@ class Settings { }) } - logError (msg) { + logError (msg, repo = this.repo) { this.log.error(msg) this.errors.push({ - owner: this.repo.owner, - repo: this.repo.repo, + owner: repo.owner, + repo: repo.repo, msg, plugin: this.constructor.name }) @@ -1074,7 +1074,7 @@ class Settings { // by the syncAll/syncSelectedRepos top-level catch (e.g. invalid // disable_plugins entries) would go unnoticed by PR reviewers. if (this.nop) { - const nopcommand = new NopCommand(this.constructor.name, this.repo, null, msg, 'ERROR') + const nopcommand = new NopCommand(this.constructor.name, repo, null, msg, 'ERROR') this.appendToResults([nopcommand]) } } @@ -2114,10 +2114,7 @@ class Settings { } } catch (e) { if (this.nop) { - const nopcommand = new NopCommand(this.constructor.name, this.repo, null, `${e}`, 'ERROR') - this.log.error(`NOPCOMMAND ${JSON.stringify(nopcommand)}`) - this.appendToResults([nopcommand]) - // throw e + this.logError(`${e}`, repo) } else { throw e } @@ -2586,13 +2583,24 @@ class Settings { async eachRepositoryRepos (github, log) { log.debug('Fetching repositories') - return github.paginate('GET /installation/repositories').then(repositories => { - return Promise.all(repositories.map(repository => { - const { owner, name } = repository - return this.checkAndProcessRepo(owner.login, name) - }) + const repositories = await github.paginate('GET /installation/repositories') + const CONCURRENCY = 10 + const results = [] + for (let i = 0; i < repositories.length; i += CONCURRENCY) { + const batch = repositories.slice(i, i + CONCURRENCY) + const batchResults = await Promise.allSettled( + batch.map(({ owner, name }) => this.checkAndProcessRepo(owner.login, name)) ) - }) + batchResults.forEach((result, index) => { + if (result.status === 'fulfilled') { + results.push(result.value) + } else { + const { owner, name } = batch[index] + this.logError(`Error processing repository ${owner.login}/${name}: ${result.reason}`, { owner: owner.login, repo: name }) + } + }) + } + return results } async checkAndProcessRepo (owner, name) { @@ -2910,6 +2918,27 @@ class Settings { // actually references teams/properties. let repoTeamSlugs let repoProperties + let repoMissing = false + + const getMembership = async lookup => { + if (repoMissing) return [] + try { + return await lookup() + } catch (error) { + if (error.status !== 404) throw error + // Membership endpoints also return 404 for a repo awaiting force_create. + // Verify the repo itself is missing, rather than hiding permission errors. + try { + await this.github.rest.repos.get(repo) + } catch (repoError) { + if (repoError.status !== 404) throw repoError + repoMissing = true + this.log.debug(`Suborg membership deferred for missing repository ${repo.owner}/${repo.repo}`) + return [] + } + throw error + } + } for (const override of overridePaths) { const data = await this.loadYaml(override.path) @@ -2924,14 +2953,14 @@ class Settings { if (!matched && data.suborgteams) { if (repoTeamSlugs === undefined) { - repoTeamSlugs = (await this.getReposTeams(repo)).map(team => team.slug) + repoTeamSlugs = (await getMembership(() => this.getReposTeams(repo))).map(team => team.slug) } matched = data.suborgteams.some(teamslug => repoTeamSlugs.includes(teamslug)) } if (!matched && data.suborgproperties) { if (repoProperties === undefined) { - repoProperties = await this.getRepoCustomPropertyValues(repo) + repoProperties = await getMembership(() => this.getRepoCustomPropertyValues(repo)) } matched = this.repoMatchesProperties(repoProperties, data.suborgproperties) } diff --git a/test/unit/lib/settings-batching.test.js b/test/unit/lib/settings-batching.test.js new file mode 100644 index 000000000..d97f47855 --- /dev/null +++ b/test/unit/lib/settings-batching.test.js @@ -0,0 +1,379 @@ +const Settings = require('../../../lib/settings') +const NopCommand = require('../../../lib/nopcommand') +const env = require('../../../lib/env') +const Archive = require('../../../lib/plugins/archive') +const Repository = require('../../../lib/plugins/repository') +const Labels = require('../../../lib/plugins/labels') +const { spawnSync } = require('child_process') + +describe('Repository sync batching', () => { + let context + let settings + const admin = { owner: 'test-org', repo: 'admin' } + + function repositories (count) { + return Array.from({ length: count }, (_, index) => ({ + owner: { login: admin.owner }, + name: `repo-${index}`, + archived: index === 0 + })) + } + + beforeEach(() => { + context = { + payload: { installation: { id: 123 } }, + repo: () => admin, + octokit: { + paginate: jest.fn().mockResolvedValue([]), + rest: { + repos: { listCommits: jest.fn().mockResolvedValue({ data: [{ sha: 'head' }] }) }, + checks: { + create: jest.fn().mockResolvedValue({}), + update: jest.fn().mockResolvedValue({}) + }, + issues: { createComment: jest.fn().mockResolvedValue({}) } + } + }, + log: { debug: jest.fn(), info: jest.fn(), error: jest.fn() } + } + settings = new Settings(false, context, admin, { restrictedRepos: {} }, 'main') + }) + + it.each([1, 10, 11, 23])('limits %i repositories to batches of ten and preserves result order', async count => { + const repos = repositories(count) + context.octokit.paginate.mockResolvedValue(repos) + const pending = new Map() + let active = 0 + let maxActive = 0 + const update = jest.spyOn(settings, 'updateRepos').mockImplementation(({ repo }) => { + active++ + maxActive = Math.max(maxActive, active) + return new Promise(resolve => { + pending.set(repo, () => { + active-- + resolve([repo]) + }) + }) + }) + + const sync = settings.eachRepositoryRepos(context.octokit, context.log) + await new Promise(resolve => setImmediate(resolve)) + + for (let offset = 0; offset < count; offset += 10) { + const batch = repos.slice(offset, offset + 10) + expect(active).toBe(batch.length) + expect(update).toHaveBeenCalledTimes(offset + batch.length) + // Finish out of order, leaving one repository pending in this batch. + for (const repo of batch.slice(1).reverse()) pending.get(repo.name)() + await new Promise(resolve => setImmediate(resolve)) + expect(active).toBe(1) + expect(update).toHaveBeenCalledTimes(offset + batch.length) + pending.get(batch[0].name)() + await new Promise(resolve => setImmediate(resolve)) + } + + expect(await sync).toEqual(repos.map(repo => [repo.name])) + expect(maxActive).toBe(Math.min(10, count)) + expect(active).toBe(0) + expect(settings.processedRepoNames).toEqual(new Set(repos.map(repo => repo.name))) + expect(update).toHaveBeenCalledWith({ owner: admin.owner, repo: 'repo-0' }) + expect(context.octokit.paginate).toHaveBeenCalledWith('GET /installation/repositories') + }) + + it.each([false, true])('continues after failures in every batch and records errors (nop=%s)', async nop => { + settings.nop = nop + const repos = repositories(23) + const failures = new Map([ + ['repo-0', new Error('first batch failure')], + ['repo-12', 'second batch failure'], + ['repo-22', new Error('last batch failure')] + ]) + context.octokit.paginate.mockResolvedValue(repos) + const update = jest.spyOn(settings, 'updateRepos').mockImplementation(async ({ repo }) => { + if (failures.has(repo)) throw failures.get(repo) + return [repo] + }) + + const results = await settings.eachRepositoryRepos(context.octokit, context.log) + + expect(update).toHaveBeenCalledTimes(23) + expect(results).toEqual(repos.filter(repo => !failures.has(repo.name)).map(repo => [repo.name])) + expect(settings.processedRepoNames).toEqual(new Set(repos.map(repo => repo.name))) + expect(settings.errors).toEqual(Array.from(failures, ([repo, reason]) => ({ + owner: admin.owner, + repo, + plugin: 'Settings', + msg: `Error processing repository ${admin.owner}/${repo}: ${reason}` + }))) + expect(context.log.error.mock.calls).toEqual(settings.errors.map(error => [error.msg])) + expect(settings.results).toEqual(nop + ? settings.errors.map(error => new NopCommand('Settings', error, null, error.msg, 'ERROR')) + : []) + expect(settings.repo).toBe(admin) + }) + + it('waits for the rest of a failing batch before starting later repositories', async () => { + context.octokit.paginate.mockResolvedValue(repositories(11)) + let release + const update = jest.spyOn(settings, 'updateRepos').mockImplementation(async ({ repo }) => { + if (repo === 'repo-0') throw new Error('failed') + if (repo === 'repo-1') await new Promise(resolve => { release = resolve }) + return repo + }) + + const sync = settings.eachRepositoryRepos(context.octokit, context.log) + await new Promise(resolve => setImmediate(resolve)) + expect(update).toHaveBeenCalledTimes(10) + release() + expect(await sync).toEqual(repositories(11).slice(1).map(repo => repo.name)) + expect(update).toHaveBeenCalledTimes(11) + expect(settings.errors).toHaveLength(1) + }) + + it('returns an empty array for an empty installation', async () => { + const update = jest.spyOn(settings, 'updateRepos') + + expect(await settings.eachRepositoryRepos(context.octokit, context.log)).toEqual([]) + expect(update).not.toHaveBeenCalled() + expect(settings.errors).toEqual([]) + }) + + it('retains null, undefined and nested successful results without flattening', async () => { + context.octokit.paginate.mockResolvedValue(repositories(3)) + jest.spyOn(settings, 'updateRepos') + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce([['last']]) + + expect(await settings.eachRepositoryRepos(context.octokit, context.log)) + .toEqual([null, undefined, [['last']]]) + }) + + it.each([ + [{ include: ['repo-0', 'repo-12'] }, ['repo-0', 'repo-12']], + [{ exclude: ['repo-*'] }, []], + [['repo-*'], []] + ])('preserves repository restrictions %j across batches', async (restrictedRepos, included) => { + settings.config.restrictedRepos = restrictedRepos + const repos = repositories(13) + context.octokit.paginate.mockResolvedValue(repos) + const update = jest.spyOn(settings, 'updateRepos').mockImplementation(async ({ repo }) => repo) + + expect(await settings.eachRepositoryRepos(context.octokit, context.log)) + .toEqual(repos.map(repo => included.includes(repo.name) ? repo.name : null)) + expect(update.mock.calls).toEqual(included.map(repo => [{ owner: admin.owner, repo }])) + expect(settings.processedRepoNames).toEqual(new Set(repos.map(repo => repo.name))) + expect(settings.errors).toEqual([]) + }) + + it('preserves suborg selection through the real repository-processing path', async () => { + context.octokit.paginate.mockResolvedValue(repositories(13)) + settings.subOrgConfigMap = [{ name: 'selected', path: '.github/suborgs/selected.yml' }] + settings.subOrgConfigs = { 'repo-0': {}, 'repo-12': {} } + settings.repoConfigs = {} + const sync = jest.fn().mockResolvedValue([]) + class Plugin { + constructor (nop, github, repo) { + this.repo = repo + } + + sync () { return sync(this.repo) } + } + jest.spyOn(settings, 'childPluginsList').mockReturnValue([[Plugin, {}, 'labels']]) + + await settings.eachRepositoryRepos(context.octokit, context.log) + + expect(sync.mock.calls).toEqual([ + [{ owner: admin.owner, repo: 'repo-0' }], + [{ owner: admin.owner, repo: 'repo-12' }] + ]) + expect(settings.errors).toEqual([]) + }) + + it('propagates installation-list failures to the caller', async () => { + const error = new Error('installation unavailable') + context.octokit.paginate.mockRejectedValue(error) + const update = jest.spyOn(settings, 'updateRepos') + + await expect(settings.eachRepositoryRepos(context.octokit, context.log)).rejects.toBe(error) + expect(update).not.toHaveBeenCalled() + }) + + it.each(['apply', 'pull-request', 'full-sync'])('reports failures and later successes through syncAll in %s mode', async mode => { + const nop = mode !== 'apply' + jest.replaceProperty(env, 'CREATE_PR_COMMENT', 'true') + if (mode === 'pull-request') { + context.payload.repository = { owner: { login: admin.owner }, name: admin.repo } + context.payload.check_run = { id: 42, check_suite: { pull_requests: [{ number: 1 }] } } + } + context.octokit.paginate.mockResolvedValue(repositories(12)) + jest.spyOn(Settings.prototype, 'loadConfigs').mockResolvedValue() + jest.spyOn(Settings.prototype, 'updateOrg').mockResolvedValue() + jest.spyOn(Settings.prototype, 'syncAppInstallations').mockResolvedValue() + const orgRulesets = jest.spyOn(Settings.prototype, 'syncOrgLevelRulesets').mockResolvedValue() + const changes = [] + const update = jest.spyOn(Settings.prototype, 'updateRepos').mockImplementation(async repo => { + if (repo.repo === 'repo-0') throw new Error('unavailable') + const change = new NopCommand('Labels', repo, null, { + additions: [{ name: repo.repo }], deletions: [], modifications: [] + }) + changes.push(change) + return [[change]] + }) + const config = { restrictedRepos: {} } + + const result = await Settings.syncAll(nop, context, admin, config, 'main', config, { + repos: [{ owner: admin.owner, repo: 'new-repo' }, { owner: admin.owner, repo: 'repo-0' }] + }) + + expect(orgRulesets).toHaveBeenCalledTimes(1) + expect(update).toHaveBeenCalledTimes(13) + expect(update).toHaveBeenLastCalledWith({ owner: admin.owner, repo: 'new-repo' }) + expect(result.errors).toEqual([{ + owner: admin.owner, + repo: 'repo-0', + plugin: 'Settings', + msg: `Error processing repository ${admin.owner}/repo-0: Error: unavailable` + }]) + expect(result.results).toEqual(nop ? expect.arrayContaining(changes.slice(0, 11)) : []) + if (mode === 'apply') { + const check = context.octokit.rest.checks.create.mock.calls[0][0] + expect(check.conclusion).toBe('failure') + expect(check.output.text).toContain('repo-0') + expect(check.output.text).toContain('unavailable') + } else if (mode === 'pull-request') { + const check = context.octokit.rest.checks.update.mock.calls[0][0] + expect(check.conclusion).toBe('failure') + const comment = context.octokit.rest.issues.createComment.mock.calls[0][0] + for (const output of [check.output.summary, comment.body]) { + expect(output).toContain('repo-0') + expect(output).toContain('unavailable') + expect(output).toContain('repo-11') + } + } else { + expect(context.log.debug).toHaveBeenCalledWith({ results: result.results }, 'Dry-run results') + expect(context.log.info).toHaveBeenCalledWith(expect.stringContaining('ERROR Settings repo-0')) + expect(context.octokit.rest.checks.create).not.toHaveBeenCalled() + expect(context.octokit.rest.checks.update).not.toHaveBeenCalled() + } + }) + + it('continues through later suborgs and app installations after a repository failure', async () => { + const subOrgs = [ + { name: 'first', path: '.github/suborgs/first.yml' }, + { name: 'second', path: '.github/suborgs/second.yml' } + ] + context.octokit.paginate.mockResolvedValue(repositories(12)) + jest.spyOn(Settings.prototype, 'getSubOrgConfigs').mockResolvedValue({}) + jest.spyOn(Settings.prototype, 'loadConfigs').mockResolvedValue() + const orgRulesets = jest.spyOn(Settings.prototype, 'syncOrgLevelRulesets').mockResolvedValue() + const apps = jest.spyOn(Settings.prototype, 'syncAppInstallations').mockResolvedValue() + const update = jest.spyOn(Settings.prototype, 'updateRepos').mockImplementation(async function ({ repo }) { + if (this.subOrgConfigMap[0].name === 'first' && repo === 'repo-0') { + throw new Error('first suborg failure') + } + return [] + }) + + await Settings.syncSelectedRepos(false, context, [], subOrgs, { restrictedRepos: {} }, 'main') + + expect(update).toHaveBeenCalledTimes(24) + expect(update).toHaveBeenLastCalledWith({ owner: admin.owner, repo: 'repo-11' }) + expect(orgRulesets).toHaveBeenCalledTimes(1) + expect(apps).toHaveBeenCalledTimes(1) + const check = context.octokit.rest.checks.create.mock.calls[0][0] + expect(check.conclusion).toBe('failure') + expect(check.output.text).toContain('first suborg failure') + }) + + it.each(['archive', 'repository', 'child'])('retains caught %s failures through real NOP processing and full-sync exit status', async stage => { + const repos = repositories(12) + const failedRepos = ['repo-0', 'repo-10'] + const fail = repo => { + if (failedRepos.includes(repo.repo)) throw new Error(`${stage} failed for ${repo.repo}`) + } + context.octokit.paginate.mockResolvedValue(repos) + jest.spyOn(Settings.prototype, 'getSubOrgConfigs').mockResolvedValue({}) + jest.spyOn(Settings.prototype, 'getRepoConfigs').mockResolvedValue({}) + jest.spyOn(Settings.prototype, 'updateOrg').mockResolvedValue() + jest.spyOn(Settings.prototype, 'syncAppInstallations').mockResolvedValue() + jest.spyOn(Settings.prototype, 'syncOrgLevelRulesets').mockResolvedValue() + jest.spyOn(Settings.prototype, 'childPluginsList').mockReturnValue([[Labels, [], 'labels']]) + const archive = jest.spyOn(Archive.prototype, 'getState').mockImplementation(async function () { + if (stage === 'archive') fail(this.repo) + return { shouldArchive: false, shouldUnarchive: false } + }) + jest.spyOn(Repository.prototype, 'sync').mockImplementation(async function () { + if (stage === 'repository') fail(this.repo) + return [] + }) + const labels = jest.spyOn(Labels.prototype, 'sync').mockImplementation(async function () { + if (stage === 'child') fail(this.repo) + return [new NopCommand('Labels', this.repo, null, { + additions: [{ name: this.repo.repo }], modifications: [], deletions: [] + })] + }) + + const result = await Settings.syncAll(true, context, admin, { + restrictedRepos: {}, repository: {} + }, 'main') + + // Execute the real CLI entrypoint with this sync's error collection. + const cli = spawnSync(process.execPath, ['-e', ` + const fs = require('fs') + const vm = require('vm') + const settings = JSON.parse(fs.readFileSync(0, 'utf8')) + vm.runInNewContext(fs.readFileSync(process.argv[1], 'utf8'), { + require: name => { + if (name === './') return () => ({ syncInstallation: async () => settings }) + if (name === './lib/env') return { FULL_SYNC_NOP: true } + if (name === 'probot') return { createProbot: () => ({ log: console }) } + throw new Error('Unexpected dependency: ' + name) + }, + process, + console + }) + `, require.resolve('../../../full-sync')], { + input: JSON.stringify({ errors: result.errors }), + encoding: 'utf8' + }) + expect(cli.status).toBe(1) + expect(cli.stderr).toContain('Errors occurred during full sync.') + expect(cli.stdout).not.toContain('Full sync completed successfully.') + + expect(archive).toHaveBeenCalledTimes(12) + expect(labels.mock.instances.at(-1).repo).toEqual({ owner: admin.owner, repo: 'repo-11' }) + expect(result.processedRepoNames).toEqual(new Set(repos.map(repo => repo.name))) + expect(result.repo).toBe(admin) + expect(result.errors).toEqual(failedRepos.map(repo => ({ + owner: admin.owner, repo, plugin: 'Settings', msg: `Error: ${stage} failed for ${repo}` + }))) + expect(result.results.filter(row => row.type === 'ERROR')).toEqual( + result.errors.map(error => new NopCommand('Settings', error, null, error.msg, 'ERROR')) + ) + expect(result.results.filter(row => row.plugin === 'Labels').map(row => row.repo)) + .toEqual(repos.filter(repo => !failedRepos.includes(repo.name)).map(repo => repo.name)) + expect(context.log.error.mock.calls).toEqual(result.errors.map(error => [error.msg])) + + context.payload.repository = { owner: { login: admin.owner }, name: admin.repo } + context.payload.check_run = { id: 42, check_suite: { pull_requests: [{ number: 1 }] } } + jest.replaceProperty(env, 'CREATE_PR_COMMENT', 'true') + await result.handleResults() + const check = context.octokit.rest.checks.update.mock.calls[0][0] + const comment = context.octokit.rest.issues.createComment.mock.calls[0][0] + expect(check.conclusion).toBe('failure') + for (const output of [check.output.summary, comment.body]) { + for (const repo of failedRepos) expect(output).toContain(`**${repo}**`) + expect(output).toContain('repo-11') + } + }) + + it.each([false, true])('keeps logError default attribution (nop=%s)', nop => { + settings.nop = nop + + settings.logError('configuration failed') + + expect(settings.errors).toEqual([{ ...admin, plugin: 'Settings', msg: 'configuration failed' }]) + expect(settings.results).toEqual(nop ? [new NopCommand('Settings', admin, null, 'configuration failed', 'ERROR')] : []) + }) +}) diff --git a/test/unit/lib/settings-new-repo.test.js b/test/unit/lib/settings-new-repo.test.js new file mode 100644 index 000000000..92c2bace5 --- /dev/null +++ b/test/unit/lib/settings-new-repo.test.js @@ -0,0 +1,200 @@ +const Settings = require('../../../lib/settings') +const Repository = require('../../../lib/plugins/repository') +const Teams = require('../../../lib/plugins/teams') +const Rulesets = require('../../../lib/plugins/rulesets') +const NopCommand = require('../../../lib/nopcommand') + +describe('Suborg membership before repository creation', () => { + const repo = { owner: 'test-org', repo: 'demo-repo-service2' } + const suborgPath = '.github/suborgs/expert-services.yml' + const team = 'expert-services-developers' + const notFound = () => Object.assign(new Error('Not Found'), { status: 404 }) + let context + let settings + let suborg + let getTeams + let getProperties + + beforeEach(() => { + context = { + payload: { + installation: { id: 123 }, + repository: { owner: { login: repo.owner }, name: 'admin' }, + check_run: { id: 42, check_suite: { pull_requests: [{ number: 1 }] } } + }, + repo: () => ({ owner: repo.owner, repo: 'admin' }), + octokit: { + rest: { + repos: { + get: jest.fn().mockRejectedValue(notFound()), + listCommits: jest.fn().mockResolvedValue({ data: [{ sha: 'head' }] }) + }, + checks: { + create: jest.fn().mockResolvedValue({}), + update: jest.fn().mockResolvedValue({}) + }, + issues: { createComment: jest.fn().mockResolvedValue({}) } + } + }, + log: { debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn() } + } + suborg = { + suborgteams: [team], + rulesets: [{ name: 'Protect release and production branches' }] + } + settings = new Settings(false, context, repo, { restrictedRepos: {} }, 'main') + jest.spyOn(Settings.prototype, 'getSubOrgConfigMap').mockResolvedValue([{ name: 'expert-services.yml', path: suborgPath }]) + jest.spyOn(Settings.prototype, 'loadYaml').mockImplementation(async () => suborg) + getTeams = jest.spyOn(Settings.prototype, 'getReposTeams').mockRejectedValue(notFound()) + getProperties = jest.spyOn(Settings.prototype, 'getRepoCustomPropertyValues').mockRejectedValue(notFound()) + }) + + it.each(['teams', 'properties'])('treats %s as unmatched only when the repository is also missing', async selector => { + if (selector === 'properties') suborg = { suborgproperties: [{ ownership: 'expert-services' }] } + + expect(await settings.getSubOrgConfigs(repo)).toEqual({}) + + expect(context.octokit.rest.repos.get).toHaveBeenCalledTimes(1) + expect(context.octokit.rest.repos.get).toHaveBeenCalledWith(repo) + expect(context.log.error).not.toHaveBeenCalled() + expect(settings.errors).toEqual([]) + }) + + it('keeps later name-based matches and avoids repeated lookups for a missing repo', async () => { + const byName = { suborgrepos: ['demo-repo-*'] } + settings.getSubOrgConfigMap.mockResolvedValue([ + { name: 'first.yml', path: 'first.yml' }, + { name: 'second.yml', path: 'second.yml' }, + { name: 'by-name.yml', path: 'by-name.yml' } + ]) + settings.loadYaml.mockImplementation(async path => path === 'by-name.yml' + ? byName + : { suborgteams: [team], suborgproperties: [{ ownership: 'expert-services' }] }) + + expect(await settings.getSubOrgConfigs(repo)).toEqual({ + [repo.repo]: { ...byName, source: 'by-name.yml' } + }) + expect(getTeams).toHaveBeenCalledTimes(1) + expect(getProperties).not.toHaveBeenCalled() + expect(context.octokit.rest.repos.get).toHaveBeenCalledTimes(1) + }) + + it.each(['teams', 'properties'])('does not hide a %s 404 for an existing repository', async selector => { + if (selector === 'properties') suborg = { suborgproperties: [{ ownership: 'expert-services' }] } + context.octokit.rest.repos.get.mockResolvedValue({ data: { name: repo.repo } }) + const error = notFound() + const lookup = selector === 'teams' ? getTeams : getProperties + lookup.mockRejectedValue(error) + + await expect(settings.getSubOrgConfigs(repo)).rejects.toBe(error) + }) + + it.each([403, 429, 500])('propagates lookup errors with status %i without checking existence', async status => { + const error = Object.assign(new Error('Lookup failed'), { status }) + getTeams.mockRejectedValue(error) + + await expect(settings.getSubOrgConfigs(repo)).rejects.toBe(error) + expect(context.octokit.rest.repos.get).not.toHaveBeenCalled() + }) + + it('propagates a failure to verify repository existence', async () => { + const error = Object.assign(new Error('Cannot verify repository'), { status: 403 }) + context.octokit.rest.repos.get.mockRejectedValue(error) + + await expect(settings.getSubOrgConfigs(repo)).rejects.toBe(error) + }) + + it('retains genuine lookup failures in NOP results', async () => { + settings.nop = true + context.octokit.rest.repos.get.mockResolvedValue({ data: { name: repo.repo } }) + + await settings.getSubOrgConfigs(repo) + + expect(settings.results).toEqual([ + expect.objectContaining({ type: 'ERROR', action: expect.objectContaining({ msg: 'Error: Not Found' }) }) + ]) + expect(context.log.error).toHaveBeenCalled() + }) + + it('does not add existence checks for successful cached lookups', async () => { + settings.getSubOrgConfigMap.mockResolvedValue([ + { name: 'first.yml', path: 'first.yml' }, + { name: 'second.yml', path: 'second.yml' } + ]) + suborg = { suborgteams: [team], suborgproperties: [{ ownership: 'expert-services' }] } + getTeams.mockResolvedValue([]) + getProperties.mockResolvedValue([]) + + expect(await settings.getSubOrgConfigs(repo)).toEqual({}) + + expect(getTeams).toHaveBeenCalledTimes(1) + expect(getProperties).toHaveBeenCalledTimes(1) + expect(context.octokit.rest.repos.get).not.toHaveBeenCalled() + }) + + it.each([ + [false, false], + [false, true], + [true, false], + [true, true] + ])('preserves creation with nop=%s and previous-phase suborg=%s', async (nop, hasSuborg) => { + let created = false + let addedTeam = false + if (!hasSuborg) settings.getSubOrgConfigMap.mockResolvedValue([]) + const override = { + repository: { name: repo.repo, force_create: true, archived: false }, + teams: [{ name: team, permission: 'push' }] + } + context.octokit.rest.repos.get.mockImplementation(async () => { + if (!created) throw notFound() + return { data: { name: repo.repo, archived: false } } + }) + getTeams.mockImplementation(async () => { + if (!created) throw notFound() + return addedTeam ? [{ slug: team }] : [] + }) + jest.spyOn(Settings.prototype, 'getReposForTeam').mockImplementation(async () => { + return [ + { name: 'test' }, + { name: 'demo-repo-service1', archived: true }, + ...(addedTeam ? [{ name: repo.repo }] : []) + ] + }) + jest.spyOn(Settings.prototype, 'getRepoConfigs').mockResolvedValue({ [`${repo.repo}.yml`]: override }) + jest.spyOn(Settings.prototype, 'syncOrgLevelRulesets').mockResolvedValue() + jest.spyOn(Settings.prototype, 'syncAppInstallations').mockResolvedValue() + const create = jest.spyOn(Repository.prototype, 'sync').mockImplementation(async function () { + this.created = !created + if (!this.nop) created = true + return this.nop ? [new NopCommand('Repository', this.repo, null, 'Create Repo')] : [] + }) + const addTeam = jest.spyOn(Teams.prototype, 'sync').mockImplementation(async function () { + this.hasChanges = !addedTeam + if (!this.nop) addedTeam = true + return [] + }) + const applyRulesets = jest.spyOn(Rulesets.prototype, 'sync').mockResolvedValue([]) + + await Settings.syncSelectedRepos(nop, context, [repo], [], { restrictedRepos: {} }, 'main') + + expect(create).toHaveBeenCalled() + expect(addTeam).toHaveBeenCalled() + expect(context.log.error).not.toHaveBeenCalled() + if (nop) { + expect(created).toBe(false) + expect(addedTeam).toBe(false) + const check = context.octokit.rest.checks.update.mock.calls[0][0] + expect(check.conclusion).toBe('success') + expect(check.output.summary).toContain('Create Repo') + } else { + expect(created).toBe(true) + expect(addedTeam).toBe(true) + expect(applyRulesets).toHaveBeenCalledTimes(hasSuborg ? 1 : 0) + if (hasSuborg) { + expect(applyRulesets.mock.instances[0].entries).toEqual(suborg.rulesets) + expect(applyRulesets.mock.instances[0].repo).toEqual(repo) + } + expect(context.octokit.rest.checks.create.mock.calls[0][0].conclusion).toBe('success') + } + }) +})