diff --git a/lib/settings.js b/lib/settings.js index fd7ca2693..db6b4ad9f 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -328,7 +328,7 @@ ${this.results.reduce((x, y) => { } } - async updateRepos (repo) { + async updateRepos (repo, archived) { this.subOrgConfigs = this.subOrgConfigs || await this.getSubOrgConfigs() // Create a new object to avoid mutating the shared this.config.repository // This prevents race conditions when multiple repos are processed concurrently via Promise.all @@ -362,6 +362,24 @@ ${this.results.reduce((x, y) => { if (overrideRepoConfig) { repoConfig = this.mergeDeep.mergeDeep({}, repoConfig, overrideRepoConfig) } + + const archivePlugin = new Archive(this.nop, this.github, repo, repoConfig, this.log) + + // Archived repos get skipped further down, but only after + // archivePlugin.getState() has spent a repos.get on each one. When the caller + // already knows — eachRepositoryRepos gets `archived` for free from + // GET /installation/repositories — skip before issuing any request. On orgs + // where most repos are archived this is where the bulk of a full sync's + // rate-limit budget goes. + // + // An explicit `archived: false` in config is a request to unarchive and must + // still be processed, so the desired state decides. getDesiredArchiveState() + // reads config only and issues no request. + if (archived === true && archivePlugin.getDesiredArchiveState() !== false) { + this.log.debug(`Skipping archived repo ${repo.repo} without fetching it`) + return + } + if (repoConfig) { try { this.log.debug(`found a matching repoconfig for this repo ${JSON.stringify(repoConfig)}`) @@ -369,7 +387,6 @@ ${this.results.reduce((x, y) => { const childPlugins = this.childPluginsList(repo) const RepoPlugin = Settings.PLUGINS.repository - const archivePlugin = new Archive(this.nop, this.github, repo, repoConfig, this.log) const { isArchived, shouldArchive, shouldUnarchive } = await archivePlugin.getState() if (shouldUnarchive) { @@ -410,6 +427,20 @@ ${this.results.reduce((x, y) => { } } else { this.log.debug(`Didnt find any a matching repoconfig for this repo ${JSON.stringify(repo)} in ${JSON.stringify(this.repoConfigs)}`) + + // This branch has no repoConfig, so getState() was never reached and the + // isArchived guard above did not run — child plugins would still issue + // forbidden writes against an archived repo. Only verify when the archived + // state is not already known: `false` from the listing needs no request, + // and `true` already returned above unless an unarchive was requested. + if (archived !== false) { + const { isArchived, shouldUnarchive } = await archivePlugin.getState() + if (isArchived && !shouldUnarchive) { + this.log.debug(`Skipping child plugin updates for archived repo ${repo.repo}`) + return + } + } + const childPlugins = this.childPluginsList(repo) return Promise.all(childPlugins.map(([Plugin, config]) => { return new Plugin(this.nop, this.github, repo, config, this.log, this.errors).sync().then(res => { @@ -540,18 +571,25 @@ ${this.results.reduce((x, y) => { 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) + // `archived` is already part of the listing payload, so passing it down + // lets updateRepos skip archived repos without spending an API call. + const { owner, name, archived } = repository + return this.checkAndProcessRepo(owner.login, name, archived) }) ) }) } - async checkAndProcessRepo (owner, name) { + async checkAndProcessRepo (owner, name, archived) { if (this.isRestricted(name)) { return null } - return this.updateRepos({ owner, repo: name }) + // `archived` travels as its own argument, never merged into the repo ref: the + // Repository plugin does Object.assign({}, settings, repo) and later + // repos.update(settings), so an `archived` key on the ref would land in the + // update payload — and would re-archive a repo in the middle of unarchiving + // it. Plugins must keep receiving a bare { owner, repo }. + return this.updateRepos({ owner, repo: name }, archived) } /** diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index e102379a6..51c2940d3 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -234,6 +234,88 @@ repository: }) }) }) // repoOverrideConfig + + describe('updateRepos with a known-archived repo', () => { + let settings + + beforeEach(() => { + stubConfig = { repository: { has_wiki: true }, restrictedRepos: { exclude: [] } } + // Built without a suborg on purpose: passing one sets subOrgConfigMap, and + // updateRepos then returns early for any repo outside that suborg. + settings = new Settings(false, stubContext, mockRepo, stubConfig, mockRef) + settings.subOrgConfigs = {} + settings.repoConfigs = {} + // repos.get is what archivePlugin.getState() calls. Asserting on it proves + // whether the archived repo was skipped before any request was made. + settings.github.rest.repos.get = jest.fn().mockResolvedValue({ data: { archived: true } }) + settings.github.rest.repos.update = jest.fn().mockResolvedValue({ data: {} }) + }) + + it('Skips without fetching the repo when the caller reports it archived', async () => { + await settings.updateRepos({ owner: 'test', repo: 'archived-repo' }, true) + expect(settings.github.rest.repos.get).not.toHaveBeenCalled() + }) + + it('Still processes the repo when config asks to unarchive it', async () => { + settings.config.repository.archived = false + await settings.updateRepos({ owner: 'test', repo: 'archived-repo' }, true) + expect(settings.github.rest.repos.get).toHaveBeenCalled() + }) + + it('Still processes the repo when the caller does not report archived state', async () => { + await settings.updateRepos({ owner: 'test', repo: 'some-repo' }) + expect(settings.github.rest.repos.get).toHaveBeenCalled() + }) + + it('Guards the no-repoConfig path too, so child plugins do not write to an archived repo', async () => { + // Label-only config: repoConfig is absent, so getState() is never reached + // by the main branch and the child plugins would run unguarded. + settings.config = { restrictedRepos: { exclude: [] }, labels: [{ name: 'bug' }] } + // Realistic stub: the labels plugin builds options via + // listLabelsForRepo.endpoint.merge and then calls github.paginate. Stubbing + // both means the plugin would run cleanly if it were reached, so the + // assertion below fails loudly instead of passing by accident. + settings.github.rest.issues = { + listLabelsForRepo: Object.assign(jest.fn(), { endpoint: { merge: jest.fn(() => ({})) } }) + } + settings.github.paginate = jest.fn().mockResolvedValue([]) + + await settings.updateRepos({ owner: 'test', repo: 'archived-repo' }) + + expect(settings.github.rest.repos.get).toHaveBeenCalled() + expect(settings.github.paginate).not.toHaveBeenCalled() + }) + + it('Does not leak archived onto the repo ref handed to plugins', async () => { + // The Repository plugin does Object.assign({}, settings, repo) and later + // repos.update(settings): an `archived` key on the ref would land in the + // update payload and re-archive the repo mid-unarchive. + settings.github.paginate = jest.fn().mockResolvedValue([ + { name: 'archived-repo', archived: true, owner: { login: 'test' } } + ]) + const seen = [] + settings.updateRepos = jest.fn(async (repo, archived) => { seen.push({ repo, archived }) }) + + await settings.eachRepositoryRepos(settings.github, settings.log) + + expect(seen).toEqual([{ repo: { owner: 'test', repo: 'archived-repo' }, archived: true }]) + expect(seen[0].repo).not.toHaveProperty('archived') + }) + + it('Passes the archived flag from the repository listing as a separate argument', async () => { + settings.github.paginate = jest.fn().mockResolvedValue([ + { name: 'active-repo', archived: false, owner: { login: 'test' } }, + { name: 'archived-repo', archived: true, owner: { login: 'test' } } + ]) + const seen = [] + settings.updateRepos = jest.fn(async (repo, archived) => { seen.push([repo.repo, archived]) }) + + await settings.eachRepositoryRepos(settings.github, settings.log) + + expect(seen).toEqual([['active-repo', false], ['archived-repo', true]]) + }) + }) // updateRepos with a known-archived repo + describe('loadConfigs', () => { describe('load suborg configs', () => { beforeEach(() => {