Fix suborg-applied settings not removed when targeting rules change

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...`)

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 = []

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 () {

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: [