Skip to content

Commit 65e957e

Browse files
committed
Skip settings sync for repos that are already archived
When a repository is archived, GitHub treats it as read-only and rejects any settings update (repository, branch protection, labels, teams, etc.) with a 403 "Repository was archived so is read-only." error. `updateRepos` only used the archive plugin's state to decide whether to archive/unarchive a repo, but it still ran the repository plugin and all child plugins unconditionally. For repos that are already archived and are staying archived, this issues a `repos.update()` (and other writes) against a read-only repo every sync, which 403s, gets logged as an error and surfaces as a failing check run. Skip the repository and child plugins when a repo is archived and is not being unarchived in this run. Newly-archived repos (shouldArchive) are still configured first and archived last, and repos being unarchived are configured after the unarchive, so those paths are unchanged. Add regression tests covering already-archived, newly-archived and non-archived repos.
1 parent 3d61f1f commit 65e957e

2 files changed

Lines changed: 89 additions & 9 deletions

File tree

lib/settings.js

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -370,23 +370,32 @@ ${this.results.reduce((x, y) => {
370370
const RepoPlugin = Settings.PLUGINS.repository
371371

372372
const archivePlugin = new Archive(this.nop, this.github, repo, repoConfig, this.log)
373-
const { shouldArchive, shouldUnarchive } = await archivePlugin.getState()
373+
const { isArchived, shouldArchive, shouldUnarchive } = await archivePlugin.getState()
374374

375375
if (shouldUnarchive) {
376376
this.log.debug(`Unarchiving repo ${repo.repo}`)
377377
const unArchiveResults = await archivePlugin.sync()
378378
this.appendToResults(unArchiveResults)
379379
}
380380

381-
const repoResults = await new RepoPlugin(this.nop, this.github, repo, repoConfig, this.installation_id, this.log, this.errors).sync()
382-
this.appendToResults(repoResults)
381+
// An archived repo is read-only: GitHub rejects any settings update
382+
// (repository, branch protection, labels, teams, etc.) with a 403. If a
383+
// repo is archived and is not being unarchived in this run, skip every
384+
// other plugin and let only the archive plugin run. Newly-archived repos
385+
// (shouldArchive) are still configured first and archived last, below.
386+
if (isArchived && !shouldUnarchive) {
387+
this.log.debug(`Skipping settings sync for archived repo ${repo.repo}`)
388+
} else {
389+
const repoResults = await new RepoPlugin(this.nop, this.github, repo, repoConfig, this.installation_id, this.log, this.errors).sync()
390+
this.appendToResults(repoResults)
383391

384-
const childResults = await Promise.all(
385-
childPlugins.map(([Plugin, config]) => {
386-
return new Plugin(this.nop, this.github, repo, config, this.log, this.errors).sync()
387-
})
388-
)
389-
this.appendToResults(childResults)
392+
const childResults = await Promise.all(
393+
childPlugins.map(([Plugin, config]) => {
394+
return new Plugin(this.nop, this.github, repo, config, this.log, this.errors).sync()
395+
})
396+
)
397+
this.appendToResults(childResults)
398+
}
390399

391400
if (shouldArchive) {
392401
this.log.debug(`Archiving repo ${repo.repo}`)

test/unit/lib/settings.test.js

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,4 +462,75 @@ repository:
462462
);
463463
});
464464
});
465+
466+
describe('updateRepos archived repos', () => {
467+
const Archive = require('../../../lib/plugins/archive')
468+
let settings
469+
470+
beforeEach(() => {
471+
// suborg must be undefined, otherwise updateRepos returns early because the
472+
// repo is not part of the changed suborg config.
473+
mockSubOrg = undefined
474+
stubConfig = {
475+
restrictedRepos: {},
476+
// Presence of a repository section means a repoConfig is built and the
477+
// main `if (repoConfig)` branch of updateRepos runs.
478+
repository: { has_wiki: false }
479+
}
480+
settings = createSettings(stubConfig)
481+
// Avoid any network calls for config loading.
482+
settings.subOrgConfigs = {}
483+
settings.repoConfigs = {}
484+
jest.spyOn(settings, 'childPluginsList').mockReturnValue([])
485+
})
486+
487+
afterEach(() => {
488+
jest.restoreAllMocks()
489+
})
490+
491+
it('skips the repository plugin for a repo that is already archived', async () => {
492+
jest.spyOn(Archive.prototype, 'getState').mockResolvedValue({
493+
isArchived: true,
494+
shouldArchive: false,
495+
shouldUnarchive: false
496+
})
497+
const repoSync = jest.spyOn(Settings.PLUGINS.repository.prototype, 'sync').mockResolvedValue([])
498+
const archiveSync = jest.spyOn(Archive.prototype, 'sync').mockResolvedValue([])
499+
500+
await settings.updateRepos({ owner: 'test', repo: 'archived-repo' })
501+
502+
// No settings update is attempted against the read-only archived repo.
503+
expect(repoSync).not.toHaveBeenCalled()
504+
expect(archiveSync).not.toHaveBeenCalled()
505+
})
506+
507+
it('configures and then archives a repo that is being newly archived', async () => {
508+
jest.spyOn(Archive.prototype, 'getState').mockResolvedValue({
509+
isArchived: false,
510+
shouldArchive: true,
511+
shouldUnarchive: false
512+
})
513+
const repoSync = jest.spyOn(Settings.PLUGINS.repository.prototype, 'sync').mockResolvedValue([])
514+
const archiveSync = jest.spyOn(Archive.prototype, 'sync').mockResolvedValue([])
515+
516+
await settings.updateRepos({ owner: 'test', repo: 'to-archive' })
517+
518+
// The repo is still writable, so settings are applied before it is archived.
519+
expect(repoSync).toHaveBeenCalled()
520+
expect(archiveSync).toHaveBeenCalled()
521+
})
522+
523+
it('configures a non-archived repo as usual', async () => {
524+
jest.spyOn(Archive.prototype, 'getState').mockResolvedValue({
525+
isArchived: false,
526+
shouldArchive: false,
527+
shouldUnarchive: false
528+
})
529+
const repoSync = jest.spyOn(Settings.PLUGINS.repository.prototype, 'sync').mockResolvedValue([])
530+
531+
await settings.updateRepos({ owner: 'test', repo: 'normal-repo' })
532+
533+
expect(repoSync).toHaveBeenCalled()
534+
})
535+
}) // updateRepos archived repos
465536
}) // Settings Tests

0 commit comments

Comments
 (0)