diff --git a/index.js b/index.js index 09ba59df..d3b801c5 100644 --- a/index.js +++ b/index.js @@ -100,7 +100,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => } } - return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref, baseConfig) + return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref, baseConfig, baseRef) } catch (e) { if (nop) { let filename = env.SETTINGS_FILE_PATH @@ -303,7 +303,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => } if (repoChanges.length > 0 || subOrgChanges.length > 0) { - return syncSelectedSettings(false, context, repoChanges, subOrgChanges) + return syncSelectedSettings(false, context, repoChanges, subOrgChanges, payload.after, payload.before) } robot.log.debug(`No changes in '${Settings.FILE_PATH}' detected, returning...`) diff --git a/lib/settings.js b/lib/settings.js index 9242044e..3a10e4fc 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -584,7 +584,7 @@ class Settings { } } - static async syncSelectedRepos (nop, context, repos, subOrgs, config, ref, baseConfig) { + static async syncSelectedRepos (nop, context, repos, subOrgs, config, ref, baseConfig, baseRef) { const settings = new Settings(nop, context, context.repo(), config, ref, null, baseConfig) settings.setChangedConfigTargets(repos, subOrgs) @@ -594,6 +594,39 @@ class Settings { settings.subOrgConfigs = await settings.getSubOrgConfigs() settings.trackChangedReposFromSubOrgConfigs() + // Identify repos removed from suborg targeting due to targeting rule + // changes in the suborg config file. These repos need processing so + // their suborg-applied settings (e.g. rulesets) are cleaned up. + if (subOrgs.length > 0 && baseRef) { + const removalResult = await settings.getReposRemovedFromSubOrgTargeting(subOrgs, baseRef) + const removedRepos = removalResult.repos + const previousPluginSections = removalResult.previousPluginSections + if (removedRepos.length > 0) { + settings.log.debug(`Repos removed from suborg targeting: ${JSON.stringify(removedRepos)}`) + settings.log.debug(`Previous suborg plugin sections to clean up: ${JSON.stringify(previousPluginSections)}`) + // Add removed repos to changedRepoNames so NOP filtering keeps their results + if (!settings.changedRepoNames) { + settings.changedRepoNames = new Set() + } + for (const repoName of removedRepos) { + settings.changedRepoNames.add(repoName) + } + // Process removed repos with org-only config (no suborg layer). + // Inject empty arrays for plugin sections that were in the previous + // suborg config so the plugins are instantiated and can detect/remove + // existing entries that are no longer desired. + settings.removedFromSubOrgPluginSections = previousPluginSections + for (const repoName of removedRepos) { + if (settings.isRestricted(repoName)) continue + if (settings.processedRepoNames.has(repoName)) continue + const repo = { owner: context.repo().owner, repo: repoName } + settings.repoConfigs = await settings.getRepoConfigs(repo) + await settings.updateRepos(repo) + } + settings.removedFromSubOrgPluginSections = null + } + } + // Re-eval is enabled only for the per-repo iteration (repo-yml change // path). The trailing suborg iteration below already iterates all suborg // repos, so it is left with the flag off. @@ -732,6 +765,125 @@ class Settings { }) } + // Identify repos that were previously targeted by suborg config files but + // are no longer targeted after the targeting rules changed. Loads the + // previous version of each changed suborg file from `baseRef`, resolves its + // targeting, and returns repo names present in the old targeting but absent + // from the current `this.subOrgConfigs`. + async getReposRemovedFromSubOrgTargeting (changedSubOrgs, baseRef) { + const emptyResult = { repos: [], previousPluginSections: [] } + if (!changedSubOrgs || changedSubOrgs.length === 0 || !baseRef) { + return emptyResult + } + + const removedRepos = [] + let previousPluginSections = null + + for (const suborg of changedSubOrgs) { + const filePath = suborg.path + if (!filePath) continue + + // Load the previous version of this suborg config file + let previousData + try { + previousData = await this.loadYamlFromRef(filePath, baseRef) + } catch (e) { + this.log.debug(`Could not load previous suborg config from ref ${baseRef}: ${e.message}`) + continue + } + + if (!previousData) continue + + // Resolve repos targeted by the old config + const previouslyTargetedRepos = new Set() + + // 1. suborgrepos: resolve glob patterns to concrete repo names + if (previousData.suborgrepos && Array.isArray(previousData.suborgrepos)) { + const allRepos = await this.github.paginate('GET /installation/repositories') + for (const repoPattern of previousData.suborgrepos) { + const glob = new Glob(repoPattern) + for (const repo of allRepos) { + if (glob.test(repo.name)) { + previouslyTargetedRepos.add(repo.name) + } + } + } + } + + // 2. suborgteams: resolve via GitHub API (team membership is live state) + if (previousData.suborgteams && Array.isArray(previousData.suborgteams)) { + try { + const teamPromises = previousData.suborgteams.map(teamslug => + this.getReposForTeam(teamslug) + ) + const teamResults = await Promise.all(teamPromises) + for (const repos of teamResults) { + for (const repo of repos) { + previouslyTargetedRepos.add(repo.name) + } + } + } catch (e) { + this.log.debug(`Error resolving previous suborgteams: ${e.message}`) + } + } + + // 3. suborgproperties: resolve via GitHub API (property values are live state) + if (previousData.suborgproperties && Array.isArray(previousData.suborgproperties)) { + try { + const subOrgRepositories = await this.getSubOrgRepositories(previousData.suborgproperties) + for (const repo of subOrgRepositories) { + previouslyTargetedRepos.add(repo.repository_name) + } + } catch (e) { + this.log.debug(`Error resolving previous suborgproperties: ${e.message}`) + } + } + + // Find repos in previous targeting that are NOT in current targeting + for (const repoName of previouslyTargetedRepos) { + if (!this.getSubOrgConfig(repoName)) { + removedRepos.push(repoName) + } + } + + // Collect plugin sections from previous config that need cleanup + // (these are sections that were applied by the suborg and need to be + // synced with empty config so existing entries are removed) + if (!previousPluginSections) { + previousPluginSections = new Set() + } + for (const key of Object.keys(previousData)) { + if (key in Settings.PLUGINS) { + previousPluginSections.add(key) + } + } + } + + return { + repos: [...new Set(removedRepos)], + previousPluginSections: previousPluginSections ? [...previousPluginSections] : [] + } + } + + // Load a YAML file from a specific git ref, bypassing the file cache. + // Used to load previous versions of config files for comparison. + async loadYamlFromRef (filePath, ref) { + const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO } + const params = Object.assign(repo, { path: filePath, ref }) + + const response = await this.github.repos.getContent(params) + + if (Array.isArray(response.data)) { + return null + } + + if (typeof response.data.content !== 'string') { + return null + } + + return yaml.load(Buffer.from(response.data.content, 'base64').toString()) || {} + } + // Create a check in the Admin repo for safe-settings. async createCheckRun () { const startTime = new Date() @@ -1581,6 +1733,18 @@ class Settings { const overrideConfig = this.mergeDeep.mergeDeep({}, sources.org, sources.suborg, sources.repo) + // When processing repos removed from suborg targeting, inject empty arrays + // for plugin sections that were previously provided by the suborg. This + // ensures those plugins are instantiated and can detect/remove existing + // entries that are no longer desired. + if (this.removedFromSubOrgPluginSections && !subOrgOverrideConfig) { + for (const section of this.removedFromSubOrgPluginSections) { + if (!(section in overrideConfig)) { + overrideConfig[section] = [] + } + } + } + this.log.debug(`consolidated config is ${JSON.stringify(overrideConfig)}`) const childPlugins = [] diff --git a/smoke-test.js b/smoke-test.js index 00ca7d30..40b705dc 100644 --- a/smoke-test.js +++ b/smoke-test.js @@ -644,6 +644,15 @@ const SUBORG_EXPERT_SERVICES_PROPERTY_YML = SUBORG_EXPERT_SERVICES_YML.replace( 'suborgproperties:\n - ent-ownership: expert-services' ) +// Suborg config that narrows targeting to only the 'test' repo via +// suborgrepos. Used to test that repos dropping out of suborg targeting +// (due to targeting rule changes in the suborg.yml) have their +// suborg-applied rulesets removed. +const SUBORG_EXPERT_SERVICES_NARROW_YML = SUBORG_EXPERT_SERVICES_YML.replace( + 'suborgteams:\n - expert-services-developers', + 'suborgrepos:\n - test' +) + const REPO_DEMO_SERVICE1_ARCHIVED_YML = `# Safe-Settings Configuration repository: name: demo-repo-service1 @@ -1404,9 +1413,61 @@ async function phase5Suborg () { if (!await safeMerge(ORG, ADMIN_REPO, pr3.number)) return await sleep(WEBHOOK_SETTLE_MS) + // ── Sub-test: suborg targeting rule change removes rulesets from dropped repos ── + log('Verifying suborg ruleset is on demo-repo-service1 before targeting change...') + const demo1RulesetBeforeNarrow = await poll(async () => { + return await getRepoRuleset(ORG, 'demo-repo-service1', suborgRulesetName) + }, { desc: 'suborg ruleset on demo-repo-service1 before narrowing', timeout: 90000 }) + assert(demo1RulesetBeforeNarrow !== null, 'Suborg ruleset present on demo-repo-service1 before targeting change') + + const branch4 = 'smoke-test-phase5-narrow-targeting' + await deleteBranch(ORG, ADMIN_REPO, branch4) + await createBranch(ORG, ADMIN_REPO, branch4) + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/suborgs/expert-services.yml`, SUBORG_EXPERT_SERVICES_NARROW_YML, branch4, 'Narrow suborg targeting to only test repo') + + const pr4 = await createPR(ORG, ADMIN_REPO, 'Smoke test: narrow suborg targeting (remove demo-repo-service1)', branch4, defaultBranch) + log('Waiting for NOP check run on narrowed targeting...') + await sleep(WEBHOOK_SETTLE_MS) + const checkRun4 = await waitForCheckRun(ORG, ADMIN_REPO, pr4.head.sha) + assert(checkRun4 !== null, 'Check run completed for narrowed targeting') + if (checkRun4) assert(checkRun4.conclusion === 'success', `Check run conclusion is success (got: ${checkRun4.conclusion})`) + + if (!await safeMerge(ORG, ADMIN_REPO, pr4.number)) return + await sleep(WEBHOOK_SETTLE_MS + 15000) + + log('Checking suborg ruleset removed from demo-repo-service1 after targeting change...') + const demo1RulesetAfterNarrow = await poll(async () => { + const ruleset = await getRepoRuleset(ORG, 'demo-repo-service1', suborgRulesetName) + return ruleset === null ? true : null + }, { desc: 'suborg ruleset to be removed from demo-repo-service1 after targeting narrowed', timeout: 90000 }) + assert(demo1RulesetAfterNarrow === true, 'Suborg ruleset removed from demo-repo-service1 after targeting rule change') + + const testRulesetAfterNarrow = await poll(async () => { + return await getRepoRuleset(ORG, 'test', suborgRulesetName) + }, { desc: 'suborg ruleset retained on test after narrowing', timeout: 60000 }) + assert(testRulesetAfterNarrow !== null, 'Suborg ruleset retained on test after targeting rule change') + + // Restore team-targeted config for subsequent phases + const branch5 = 'smoke-test-phase5-restore-after-narrow' + await deleteBranch(ORG, ADMIN_REPO, branch5) + await createBranch(ORG, ADMIN_REPO, branch5) + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/suborgs/expert-services.yml`, SUBORG_EXPERT_SERVICES_YML, branch5, 'Restore team-targeted expert-services suborg config after narrow test') + + const pr5 = await createPR(ORG, ADMIN_REPO, 'Smoke test: restore suborg after narrow targeting test', branch5, defaultBranch) + log('Waiting for NOP check run on restore...') + await sleep(WEBHOOK_SETTLE_MS) + const checkRun5 = await waitForCheckRun(ORG, ADMIN_REPO, pr5.head.sha) + assert(checkRun5 !== null, 'Check run completed for restore after narrow') + if (checkRun5) assert(checkRun5.conclusion === 'success', `Check run conclusion is success (got: ${checkRun5.conclusion})`) + + if (!await safeMerge(ORG, ADMIN_REPO, pr5.number)) return + await sleep(WEBHOOK_SETTLE_MS) + await deleteBranch(ORG, ADMIN_REPO, branch) await deleteBranch(ORG, ADMIN_REPO, branch2) await deleteBranch(ORG, ADMIN_REPO, branch3) + await deleteBranch(ORG, ADMIN_REPO, branch4) + await deleteBranch(ORG, ADMIN_REPO, branch5) } async function phase6Archive () { diff --git a/test/unit/lib/plugins/custom_properties.test.js b/test/unit/lib/plugins/custom_properties.test.js index 429544f8..6afc8228 100644 --- a/test/unit/lib/plugins/custom_properties.test.js +++ b/test/unit/lib/plugins/custom_properties.test.js @@ -15,9 +15,11 @@ describe('CustomProperties', () => { beforeEach(() => { github = { paginate: jest.fn(), - repos: { - getCustomPropertiesValues: jest.fn(), - createOrUpdateCustomPropertiesValues: jest.fn() + rest: { + repos: { + getCustomPropertiesValues: jest.fn(), + createOrUpdateCustomPropertiesValues: jest.fn() + } } } @@ -42,7 +44,7 @@ describe('CustomProperties', () => { const result = await plugin.find() expect(github.paginate).toHaveBeenCalledWith( - github.repos.getCustomPropertiesValues, + github.rest.repos.getCustomPropertiesValues, { owner, repo, @@ -75,14 +77,14 @@ describe('CustomProperties', () => { return plugin.sync().then(() => { expect(github.paginate).toHaveBeenCalledWith( - github.repos.getCustomPropertiesValues, + github.rest.repos.getCustomPropertiesValues, { owner, repo, per_page: 100 } ) - expect(github.repos.createOrUpdateCustomPropertiesValues).not.toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).not.toHaveBeenCalledWith({ owner, repo, properties: [ @@ -92,7 +94,7 @@ describe('CustomProperties', () => { } ] }) - expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ owner, repo, properties: [ @@ -102,7 +104,7 @@ describe('CustomProperties', () => { } ] }) - expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ owner, repo, properties: [ @@ -112,7 +114,7 @@ describe('CustomProperties', () => { } ] }) - expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ owner, repo, properties: [ @@ -127,7 +129,7 @@ describe('CustomProperties', () => { // const plugin = configure([{ name: 'Test', value: 'test' }]) // await plugin.update({ name: 'test', value: 'old' }, { name: 'test', value: 'test' }) - // expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + // expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ // owner, // repo, // properties: [ diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 14660189..79d4a8a1 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1406,4 +1406,213 @@ repository: }) }) }) + + describe('getReposRemovedFromSubOrgTargeting', () => { + let settings + + beforeEach(() => { + stubConfig = { restrictedRepos: {} } + settings = createSettings(stubConfig) + }) + + it('returns empty result when no changedSubOrgs provided', async () => { + const result = await settings.getReposRemovedFromSubOrgTargeting([], 'prev-sha') + expect(result.repos).toEqual([]) + expect(result.previousPluginSections).toEqual([]) + }) + + it('returns empty result when no baseRef provided', async () => { + const result = await settings.getReposRemovedFromSubOrgTargeting([{ path: '.github/suborgs/frontend.yml' }], null) + expect(result.repos).toEqual([]) + expect(result.previousPluginSections).toEqual([]) + }) + + it('identifies repos removed from suborgrepos targeting', async () => { + // Previous config had repo-a and repo-b in suborgrepos + const previousContent = Buffer.from(yaml.dump({ + suborgrepos: ['repo-a', 'repo-b'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + // Current config: default mock (has new-repo in suborgrepos) + const currentContent = Buffer.from(yaml.dump({ + suborgrepos: ['repo-b'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + return Promise.resolve({ data: { content: currentContent } }) + }) + + // Mock installation repos for glob resolution + stubContext.octokit.paginate = jest.fn().mockResolvedValue([ + { name: 'repo-a', owner: { login: 'test' } }, + { name: 'repo-b', owner: { login: 'test' } } + ]) + + // Current subOrgConfigs only has repo-b (repo-a was removed from targeting) + settings.subOrgConfigs = { + 'repo-b': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result.repos).toContain('repo-a') + expect(result.repos).not.toContain('repo-b') + expect(result.previousPluginSections).toContain('teams') + }) + + it('identifies repos removed from suborgrepos glob targeting', async () => { + const previousContent = Buffer.from(yaml.dump({ + suborgrepos: ['team-*'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + return Promise.resolve({ data: { content: previousContent } }) + }) + + stubContext.octokit.paginate = jest.fn().mockResolvedValue([ + { owner: { login: 'test' }, name: 'team-a1' }, + { owner: { login: 'test' }, name: 'team-b1' }, + { owner: { login: 'test' }, name: 'other' } + ]) + + // Current targeting only matches team-a* + settings.subOrgConfigs = { + 'team-a*': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result.repos).toContain('team-b1') + expect(result.repos).not.toContain('team-a1') + expect(result.repos).not.toContain('other') + }) + + it('identifies repos removed from suborgteams targeting', async () => { + // Previous config used suborgteams: [team-a] + const previousContent = Buffer.from(yaml.dump({ + suborgteams: ['team-a'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + return Promise.resolve({ data: { content: previousContent } }) + }) + + // Mock getReposForTeam to return repos for team-a + settings.getReposForTeam = jest.fn().mockResolvedValue([ + { name: 'team-repo-1' }, + { name: 'team-repo-2' } + ]) + + // Current subOrgConfigs: only team-repo-1 still matches (team-repo-2 was removed) + settings.subOrgConfigs = { + 'team-repo-1': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result.repos).toContain('team-repo-2') + expect(result.repos).not.toContain('team-repo-1') + }) + + it('identifies repos removed from suborgproperties targeting', async () => { + // Previous config used suborgproperties + const previousContent = Buffer.from(yaml.dump({ + suborgproperties: [{ EDP: true }], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + return Promise.resolve({ data: { content: previousContent } }) + }) + + // Mock getSubOrgRepositories to return repos with the property + settings.getSubOrgRepositories = jest.fn().mockResolvedValue([ + { repository_name: 'prop-repo-1' }, + { repository_name: 'prop-repo-2' } + ]) + + // Current subOrgConfigs: only prop-repo-1 still matches + settings.subOrgConfigs = { + 'prop-repo-1': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result.repos).toContain('prop-repo-2') + expect(result.repos).not.toContain('prop-repo-1') + }) + + it('deduplicates removed repos across multiple suborg files', async () => { + const previousContent = Buffer.from(yaml.dump({ + suborgrepos: ['repo-a', 'repo-b'] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockResolvedValue({ + data: { content: previousContent } + }) + + // Mock installation repos for glob resolution + stubContext.octokit.paginate = jest.fn().mockResolvedValue([ + { name: 'repo-a', owner: { login: 'test' } }, + { name: 'repo-b', owner: { login: 'test' } } + ]) + + // Neither repo matches current targeting + settings.subOrgConfigs = {} + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [ + { path: '.github/suborgs/frontend.yml', name: 'frontend' }, + { path: '.github/suborgs/frontend.yml', name: 'frontend' } // duplicate + ], + 'prev-sha' + ) + + // Should be deduplicated + const repoACount = result.repos.filter(r => r === 'repo-a').length + expect(repoACount).toBe(1) + }) + + it('handles 404 gracefully when previous file does not exist', async () => { + stubContext.octokit.repos.getContent = jest.fn().mockRejectedValue( + Object.assign(new Error('Not Found'), { status: 404 }) + ) + + settings.subOrgConfigs = {} + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/new-suborg.yml', name: 'new-suborg' }], + 'prev-sha' + ) + + expect(result).toEqual({ repos: [], previousPluginSections: [] }) + }) + }) }) // Settings Tests