Same repository that is part of multiple suborgs (#664)

* add initial code for preventing multiple suborg config for repos

* detect conflict even when a single suborg config is changed

* remove duplicate errors

* dont add nulls and undefined to results

* Update ESLint config for browser and standard

Author: decyjphr

lib/settings.js

@@ -25,7 +25,7 @@ class Settings {
     return settings
   }
 
-  static async syncSubOrgs (nop, context, suborg, repo, config, ref) {
+  static async syncSubOrgs(nop, context, suborg, repo, config, ref) {
     const settings = new Settings(nop, context, repo, config, ref, suborg)
     try {
       await settings.loadConfigs()
@@ -37,7 +37,7 @@ class Settings {
     }
   }
 
-  static async sync (nop, context, repo, config, ref) {
+  static async sync(nop, context, repo, config, ref) {
     const settings = new Settings(nop, context, repo, config, ref)
     try {
       await settings.loadConfigs(repo)
@@ -52,13 +52,13 @@ class Settings {
     }
   }
 
-  static async handleError (nop, context, repo, config, ref, nopcommand) {
+  static async handleError(nop, context, repo, config, ref, nopcommand) {
     const settings = new Settings(nop, context, repo, config, ref)
     settings.appendToResults([nopcommand])
     await settings.handleResults()
   }
 
-  constructor (nop, context, repo, config, ref, suborg) {
+  constructor(nop, context, repo, config, ref, suborg) {
     this.ref = ref
     this.context = context
     this.installation_id = context.payload.installation.id
@@ -97,7 +97,7 @@ class Settings {
   }
 
   // Create a check in the Admin repo for safe-settings.
-  async createCheckRun () {
+  async createCheckRun() {
     const startTime = new Date()
     let conclusion = 'success'
     let details = `Run on: \`${new Date().toISOString()}\``
@@ -143,7 +143,7 @@ class Settings {
       })
   }
 
-  logError (msg) {
+  logError(msg) {
     this.log.error(msg)
     this.errors.push({
       owner: this.repo.owner,
@@ -153,7 +153,7 @@ class Settings {
     })
   }
 
-  async handleResults () {
+  async handleResults() {
     const { payload } = this.context
 
     // Create a checkrun if not in nop mode
@@ -163,6 +163,13 @@ class Settings {
       return
     }
 
+    //remove duplicate rows in this.results
+    this.results = this.results.filter((thing, index, self) => {
+        return index === self.findIndex((t) => {
+          return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin
+        })
+      })
+
     let error = false
     // Different logic
     const stats = {
@@ -227,23 +234,23 @@ class Settings {
 #### :robot: Safe-Settings config changes detected:
 
 ${this.results.reduce((x, y) => {
-  if (!y) {
-    return x
-  }
-  if (y.type === 'ERROR') {
-    error = true
-    return `${x}
+        if (!y) {
+          return x
+        }
+        if (y.type === 'ERROR') {
+          error = true
+          return `${x}
 <tr><td> ❗ ${y.action.msg} </td><td> ${y.plugin} </td><td> ${prettify(y.repo)} </td><td> ${prettify(y.action.additions)} </td><td> ${prettify(y.action.deletions)} </td><td> ${prettify(y.action.modifications)} </td><tr>`
-  } else if (y.action.additions === null && y.action.deletions === null && y.action.modifications === null) {
-    return `${x}`
-  } else {
-    if (y.action === undefined) {
-      return `${x}`
-    }
-    return `${x}
+        } else if (y.action.additions === null && y.action.deletions === null && y.action.modifications === null) {
+          return `${x}`
+        } else {
+          if (y.action === undefined) {
+            return `${x}`
+          }
+          return `${x}
 <tr><td> ✋ </td><td> ${y.plugin} </td><td> ${prettify(y.repo)} </td><td> ${prettify(y.action.additions)} </td><td> ${prettify(y.action.deletions)} </td><td> ${prettify(y.action.modifications)} </td><tr>`
-  }
-}, table)}
+        }
+      }, table)}
 `
 
       const pullRequest = payload.check_run.check_suite.pull_requests[0]
@@ -273,12 +280,12 @@ ${this.results.reduce((x, y) => {
     await this.github.checks.update(params)
   }
 
-  async loadConfigs (repo) {
+  async loadConfigs(repo) {
     this.subOrgConfigs = await this.getSubOrgConfigs()
     this.repoConfigs = await this.getRepoConfigs(repo)
   }
 
-  async updateOrg () {
+  async updateOrg() {
     const rulesetsConfig = this.config.rulesets
     if (rulesetsConfig) {
       const RulesetsPlugin = Settings.PLUGINS.rulesets
@@ -288,7 +295,7 @@ ${this.results.reduce((x, y) => {
     }
   }
 
-  async updateRepos (repo) {
+  async updateRepos(repo) {
     this.subOrgConfigs = this.subOrgConfigs || await this.getSubOrgConfigs()
     let repoConfig = this.config.repository
     if (repoConfig) {
@@ -354,15 +361,15 @@ ${this.results.reduce((x, y) => {
     }
   }
 
-  async updateAll () {
+  async updateAll() {
     // this.subOrgConfigs = this.subOrgConfigs || await this.getSubOrgConfigs(this.github, this.repo, this.log)
     // this.repoConfigs = this.repoConfigs || await this.getRepoConfigs(this.github, this.repo, this.log)
     return this.eachRepositoryRepos(this.github, this.config.restrictedRepos, this.log).then(res => {
       this.appendToResults(res)
     })
   }
 
-  getSubOrgConfig (repoName) {
+  getSubOrgConfig(repoName) {
     if (this.subOrgConfigs) {
       for (const k of Object.keys(this.subOrgConfigs)) {
         const repoPattern = new Glob(k)
@@ -375,13 +382,13 @@ ${this.results.reduce((x, y) => {
   }
 
   // Remove Org specific configs from the repo config
-  returnRepoSpecificConfigs (config) {
+  returnRepoSpecificConfigs(config) {
     const newConfig = Object.assign({}, config) // clone
     delete newConfig.rulesets
     return newConfig
   }
 
-  childPluginsList (repo) {
+  childPluginsList(repo) {
     const repoName = repo.repo
     const subOrgOverrideConfig = this.getSubOrgConfig(repoName)
     this.log.debug(`suborg config for ${repoName}  is ${JSON.stringify(subOrgOverrideConfig)}`)
@@ -413,7 +420,7 @@ ${this.results.reduce((x, y) => {
     return childPlugins
   }
 
-  validate (section, baseConfig, overrideConfig) {
+  validate(section, baseConfig, overrideConfig) {
     const configValidator = this.configvalidators[section]
     if (configValidator) {
       this.log.debug(`Calling configvalidator for key ${section} `)
@@ -432,7 +439,7 @@ ${this.results.reduce((x, y) => {
     }
   }
 
-  isRestricted (repoName) {
+  isRestricted(repoName) {
     const restrictedRepos = this.config.restrictedRepos
     // Skip configuring any restricted repos
     if (Array.isArray(restrictedRepos)) {
@@ -464,11 +471,11 @@ ${this.results.reduce((x, y) => {
     return false
   }
 
-  includesRepo (repoName, restrictedRepos) {
+  includesRepo(repoName, restrictedRepos) {
     return restrictedRepos.filter((restrictedRepo) => { return RegExp(restrictedRepo).test(repoName) }).length > 0
   }
 
-  async eachRepositoryRepos (github, restrictedRepos, log) {
+  async eachRepositoryRepos(github, restrictedRepos, log) {
     log.debug('Fetching repositories')
     return github.paginate('GET /installation/repositories').then(repositories => {
       return Promise.all(repositories.map(repository => {
@@ -489,7 +496,7 @@ ${this.results.reduce((x, y) => {
    * @param params Params to fetch the file with
    * @return The parsed YAML file
    */
-  async loadConfigMap (params) {
+  async loadConfigMap(params) {
     try {
       this.log.debug(` In loadConfigMap ${JSON.stringify(params)}`)
       const response = await this.github.repos.getContent(params).catch(e => {
@@ -536,7 +543,7 @@ ${this.results.reduce((x, y) => {
    * @param params Params to fetch the file with
    * @return The parsed YAML file
    */
-  async getRepoConfigMap () {
+  async getRepoConfigMap() {
     try {
       this.log.debug(` In getRepoConfigMap ${JSON.stringify(this.repo)}`)
       // GitHub getContent api has a hard limit of returning 1000 entries without
@@ -603,7 +610,7 @@ ${this.results.reduce((x, y) => {
    * @param params Params to fetch the file with
    * @return The parsed YAML file
    */
-  async getSubOrgConfigMap () {
+  async getSubOrgConfigMap() {
     try {
       this.log.debug(` In getSubOrgConfigMap ${JSON.stringify(this.repo)}`)
       const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO }
@@ -630,7 +637,7 @@ ${this.results.reduce((x, y) => {
    * @param {*} repo repo param
    * @returns repoConfigs object
    */
-  async getRepoConfigs (repo) {
+  async getRepoConfigs(repo) {
     try {
       const overridePaths = await this.getRepoConfigMap()
       const repoConfigs = {}
@@ -682,12 +689,11 @@ ${this.results.reduce((x, y) => {
    * @param params Params to fetch the file with
    * @return The parsed YAML file
    */
-  async getSubOrgConfigs () {
+  async getSubOrgConfigs() {
     try {
-      if (this.subOrgConfigMap) {
-        this.log.debug(`SubOrg config was changed and the associated overridePaths is = ${JSON.stringify(this.subOrgConfigMap)}`)
-      }
-      const overridePaths = this.subOrgConfigMap || await this.getSubOrgConfigMap()
+      // Get all suborg configs even though we might be here becuase of a suborg config change
+      // we will filter them out if request is due to a suborg config change
+      const overridePaths = await this.getSubOrgConfigMap()
       const subOrgConfigs = {}
 
       for (const override of overridePaths) {
@@ -699,7 +705,19 @@ ${this.results.reduce((x, y) => {
         subOrgConfigs[override.name] = data
         if (data.suborgrepos) {
           data.suborgrepos.forEach(repository => {
-            subOrgConfigs[repository] = data
+            this.storeSubOrgConfigIfNoConflicts(subOrgConfigs, override.path, repository, data)
+
+            // In case support for multiple suborg configs for the same repo is required, merge the configs.
+            //
+            // Planned for the future to support multiple suborgrepos for the same repo
+            //
+            // if (existingConfigForRepo) {
+            //   subOrgConfigs[repository] = this.mergeDeep.mergeDeep({}, existingConfigForRepo, data)
+            // } else {
+            //   subOrgConfigs[repository] = data
+            // }
+
+            subOrgConfigs[repository] = Object.assign({}, data, { source: override.path })
           })
         }
         if (data.suborgteams) {
@@ -709,7 +727,7 @@ ${this.results.reduce((x, y) => {
           await Promise.all(promises).then(res => {
             res.forEach(r => {
               r.forEach(e => {
-                subOrgConfigs[e.name] = data
+                this.storeSubOrgConfigIfNoConflicts(subOrgConfigs, override.path, e.name, data)
               })
             })
           })
@@ -721,12 +739,26 @@ ${this.results.reduce((x, y) => {
           await Promise.all(promises).then(res => {
             res.forEach(r => {
               r.forEach(e => {
-                subOrgConfigs[e.repository_name] = data
+                this.storeSubOrgConfigIfNoConflicts(subOrgConfigs, override.path, e.repository_name, data)
               })
             })
           })
         }
       }
+
+      // If this was result of a suborg config change, only return the repos that are part of the suborg config
+      if (this.subOrgConfigMap) {
+        this.log.debug(`SubOrg config was changed and the associated overridePaths is = ${JSON.stringify(this.subOrgConfigMap)}`)
+        // enumerate the properties of the subOrgConfigs object and delete the ones that are not part of the suborg
+        for (const [key, value] of Object.entries(subOrgConfigs)) {
+          if (!this.subOrgConfigMap.some((overridePath) => {
+            return overridePath.path === value.source
+          }
+          )) {
+            delete subOrgConfigs[key]
+          } 
+        }
+      }
       return subOrgConfigs
     } catch (e) {
       if (this.nop) {
@@ -740,13 +772,21 @@ ${this.results.reduce((x, y) => {
     }
   }
 
+  storeSubOrgConfigIfNoConflicts(subOrgConfigs, overridePath, repoName, data) {
+    const existingConfigForRepo = subOrgConfigs[repoName]
+    if (existingConfigForRepo && existingConfigForRepo.source !== overridePath) {
+      throw new Error(`Multiple suborg configs for ${repoName} in ${overridePath} and ${existingConfigForRepo?.source}`)
+    }
+    subOrgConfigs[repoName] = Object.assign({}, data, { source: overridePath })
+  }
+
   /**
    * Loads a file from GitHub
    *
    * @param params Params to fetch the file with
    * @return The parsed YAML file
    */
-  async loadYaml (filePath) {
+  async loadYaml(filePath) {
     try {
       const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO }
       const params = Object.assign(repo, { path: filePath, ref: this.ref })
@@ -783,13 +823,16 @@ ${this.results.reduce((x, y) => {
     }
   }
 
-  appendToResults (res) {
+  appendToResults(res) {
     if (this.nop) {
-      this.results = this.results.concat(res.flat(3))
+      //Remove nulls and undefined from the results
+      const results = res.flat(3).filter(r => r)
+      
+      this.results = this.results.concat(results)
     }
   }
 
-  async getReposForTeam (teamslug) {
+  async getReposForTeam(teamslug) {
     const options = this.github.rest.teams.listReposInOrg.endpoint.merge({
       org: this.repo.owner,
       team_slug: teamslug,
@@ -798,20 +841,19 @@ ${this.results.reduce((x, y) => {
     return this.github.paginate(options)
   }
 
-  async getReposForCustomProperty (customPropertyTuple) {
-    const name=Object.keys(customPropertyTuple)[0]
+  async getReposForCustomProperty(customPropertyTuple) {
+    const name = Object.keys(customPropertyTuple)[0]
     let q = `props.${name}:${customPropertyTuple[name]}`
     q = encodeURIComponent(q)
     const options = this.github.request.endpoint((`/orgs/${this.repo.owner}/properties/values?repository_query=${q}`))
     return this.github.paginate(options)
   }
 
-
-  isObject (item) {
+  isObject(item) {
     return (item && typeof item === 'object' && !Array.isArray(item))
   }
 
-  isIterable (obj) {
+  isIterable(obj) {
     // checks for null and undefined
     if (obj == null) {
       return false