feat: allow some settings to be marked 'unsafe', to allow overriding in the repository

Author: jonathanmorley

index.js

@@ -13,13 +13,15 @@ let deploymentConfig
 module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => {
   let appName = 'safe-settings'
   let appSlug = 'safe-settings'
+  
+  // All repos _could_ be affected, so sync everything
   async function syncAllSettings (nop, context, repo = context.repo(), ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
-      robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
+      robot.log.info('Synchronizing all settings')
+
       const configManager = new ConfigManager(context, ref)
-      const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = await configManager.loadGlobalSettingsYaml()
+
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       if (ref) {
         return Settings.syncAll(nop, context, repo, config, ref)
@@ -42,13 +44,14 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     }
   }
 
+  // Only the suborg has been modified, so only sync that
   async function syncSubOrgSettings (nop, context, suborg, repo = context.repo(), ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
-      robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
+      robot.log.info(`Synchronizing settings for suborg: ${suborg}`)
+
       const configManager = new ConfigManager(context, ref)
-      const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = await configManager.loadGlobalSettingsYaml()
+
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       return Settings.syncSubOrgs(nop, context, suborg, repo, config, ref)
     } catch (e) {
@@ -67,13 +70,14 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     }
   }
 
+  // Only the repository has been modified, so only sync that
   async function syncSettings (nop, context, repo = context.repo(), ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
-      robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
+      robot.log.info(`Synchronizing settings for repo: ${repo}`)
+
       const configManager = new ConfigManager(context, ref)
-      const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = await configManager.loadGlobalSettingsYaml()
+
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       return Settings.sync(nop, context, repo, config, ref)
     } catch (e) {
@@ -258,47 +262,57 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     return null
   }
 
+  // On any push
   robot.on('push', async context => {
     const { payload } = context
     const { repository } = payload
 
-    const adminRepo = repository.name === env.ADMIN_REPO
-    if (!adminRepo) {
-      return
-    }
-
     const defaultBranch = payload.ref === 'refs/heads/' + repository.default_branch
     if (!defaultBranch) {
       robot.log.debug('Not working on the default branch, returning...')
       return
     }
 
-    const settingsModified = payload.commits.find(commit => {
-      return commit.added.includes(Settings.FILE_NAME) ||
-        commit.modified.includes(Settings.FILE_NAME)
-    })
-    if (settingsModified) {
-      robot.log.debug(`Changes in '${Settings.FILE_NAME}' detected, doing a full synch...`)
-      return syncAllSettings(false, context)
-    }
+    const adminRepo = repository.name === env.ADMIN_REPO
+    if (adminRepo) {
+      const settingsModified = payload.commits.find(commit => {
+        return commit.added.includes(Settings.FILE_NAME) ||
+          commit.modified.includes(Settings.FILE_NAME)
+      })
+      if (settingsModified) {
+        robot.log.debug(`Changes in '${Settings.FILE_NAME}' detected, doing a full sync...`)
+        return syncAllSettings(false, context)
+      }
 
-    const repoChanges = getAllChangedRepoConfigs(payload, context.repo().owner)
-    if (repoChanges.length > 0) {
-      return Promise.all(repoChanges.map(repo => {
-        return syncSettings(false, context, repo)
-      }))
-    }
+      const repoChanges = getAllChangedRepoConfigs(payload, context.repo().owner)
+      if (repoChanges.length > 0) {
+        return Promise.all(repoChanges.map(repo => {
+          return syncSettings(false, context, repo)
+        }))
+      }
 
-    const changes = getAllChangedSubOrgConfigs(payload)
-    if (changes.length) {
-      return Promise.all(changes.map(suborg => {
-        return syncSubOrgSettings(false, context, suborg)
-      }))
-    }
+      const changes = getAllChangedSubOrgConfigs(payload)
+      if (changes.length) {
+        return Promise.all(changes.map(suborg => {
+          return syncSubOrgSettings(false, context, suborg)
+        }))
+      }
 
-    robot.log.debug(`No changes in '${Settings.FILE_NAME}' detected, returning...`)
+      robot.log.debug(`No changes in '${Settings.FILE_NAME}' detected, returning...`)
+    } else {
+      const settingsModified = payload.commits.find(commit => {
+        return commit.added.includes('.github/settings.yml') ||
+          commit.modified.includes('.github/settings.yml')
+      })
+      if (settingsModified) {
+        robot.log.debug(`Changes in '.github/settings.yml' detected, doing a sync for ${repository.name}...`)
+        return syncSettings(false, context)
+      }
+    }
   })
 
+  // Invoke syncSettings for events that could cause drift
+
   robot.on('create', async context => {
     const { payload } = context
     const { sender } = payload
@@ -394,6 +408,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     return syncSettings(false, context)
   })
 
+  // Renames need some extra handling
   robot.on('repository.renamed', async context => {
     if (env.BLOCK_REPO_RENAME_BY_HUMAN!== 'true') {
       robot.log.debug(`"env.BLOCK_REPO_RENAME_BY_HUMAN" is 'false' by default. Repo rename is not managed by Safe-settings. Continue with the default behavior.`)
@@ -474,7 +489,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     }
   })
 
-
+  // Validate settings when PR checks are needed
   robot.on('check_suite.requested', async context => {
     const { payload } = context
     const { repository } = payload
@@ -503,6 +518,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     return createCheckRun(context, pull_request, headSha, headBranch)
   })
 
+  // Validate settings when PR checks are needed
   robot.on('pull_request.opened', async context => {
     robot.log.debug('Pull_request opened !')
     const { payload } = context
@@ -522,6 +538,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     return createCheckRun(context, pull_request, payload.pull_request.head.sha, payload.pull_request.head.ref)
   })
 
+  // Validate settings when PR checks are needed
   robot.on('pull_request.reopened', async context => {
     robot.log.debug('Pull_request REopened !')
     const { payload } = context
@@ -543,16 +560,13 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     return createCheckRun(context, pull_request, payload.pull_request.head.sha, payload.pull_request.head.ref)
   })
 
+  // Validate settings when PR checks are needed
   robot.on(['check_suite.rerequested'], async context => {
     robot.log.debug('Check suite was rerequested!')
     return createCheckRun(context)
   })
 
-  robot.on(['check_suite.rerequested'], async context => {
-    robot.log.debug('Check suite was rerequested!')
-    return createCheckRun(context)
-  })
-
+  // Validate settings when PR checks are needed
   robot.on(['check_run.created'], async context => {
     robot.log.debug('Check run was created!')
     const { payload } = context

lib/configManager.js

@@ -10,18 +10,30 @@ module.exports = class ConfigManager {
   }
 
   /**
-* Loads a file from GitHub
-*
-* @param params Params to fetch the file with
-* @return The parsed YAML file
-*/
-  async loadYaml (filePath) {
+   * Loads the settings file from GitHub
+   *
+   * @return The parsed YAML file
+   */
+  async loadGlobalSettingsYaml () {
+    const CONFIG_PATH = env.CONFIG_PATH
+    const filePath = path.posix.join(CONFIG_PATH, env.SETTINGS_FILE_PATH)
+    return ConfigManager.loadYaml(this.context.octokit, {
+      owner: this.context.repo().owner,
+      repo: env.ADMIN_REPO,
+      path: filePath,
+      ref: this.ref
+    })
+  }
+
+  /**
+   * Loads a file from GitHub
+   *
+   * @param params Params to fetch the file with
+   * @return The parsed YAML file
+   */
+  static async loadYaml (octokit, params) {
     try {
-      const repo = { owner: this.context.repo().owner, repo: env.ADMIN_REPO }
-      const params = Object.assign(repo, { path: filePath, ref: this.ref })
-      const response = await this.context.octokit.repos.getContent(params).catch(e => {
-        this.log.error(`Error getting settings ${e}`)
-      })
+      const response = await octokit.repos.getContent(params)
 
       // Ignore in case path is a folder
       // - https://developer.github.com/v3/repos/contents/#response-if-content-is-a-directory
@@ -43,16 +55,4 @@ module.exports = class ConfigManager {
       throw e
     }
   }
-
-  /**
-   * Loads a file from GitHub
-   *
-   * @param params Params to fetch the file with
-   * @return The parsed YAML file
-   */
-  async loadGlobalSettingsYaml () {
-    const CONFIG_PATH = env.CONFIG_PATH
-    const filePath = path.posix.join(CONFIG_PATH, env.SETTINGS_FILE_PATH)
-    return this.loadYaml(filePath)
-  }
 }

lib/deploymentConfig.js

@@ -4,51 +4,56 @@ const env = require('./env')
 
 /**
  * Class representing a deployment config.
- * It is a singleton (class object) for the deployment settings.
- * The settings are loaded from the deployment-settings.yml file during initialization and stored as static properties.
+ * The settings are loaded from the deployment-settings.yml file during initialization and stored as fields.
  */
 class DeploymentConfig {
-    //static config
-    static configvalidators = {}
-    static overridevalidators = {}
-
-    static {
-        const deploymentConfigPath = process.env.DEPLOYMENT_CONFIG_FILE ? process.env.DEPLOYMENT_CONFIG_FILE : 'deployment-settings.yml'
-        if (fs.existsSync(deploymentConfigPath)) {
-          this.config = yaml.load(fs.readFileSync(deploymentConfigPath))
-        } else {
-          this.config = { restrictedRepos: ['admin', '.github', 'safe-settings'] }
-        }
-
-        const overridevalidators = this.config.overridevalidators
-        if (this.isIterable(overridevalidators)) {
-          for (const validator of overridevalidators) {
-            // eslint-disable-next-line no-new-func
-            const f = new Function('baseconfig', 'overrideconfig', 'githubContext', validator.script)
-            this.overridevalidators[validator.plugin] = { canOverride: f, error: validator.error }
-          }
-        }
-        const configvalidators = this.config.configvalidators
-        if (this.isIterable(configvalidators)) {
-          for (const validator of configvalidators) {
-            // eslint-disable-next-line no-new-func
-            const f = new Function('baseconfig', 'githubContext', validator.script)
-            this.configvalidators[validator.plugin] = { isValid: f, error: validator.error }
-          }
-        }
+  constructor () {
+    const deploymentConfigPath = process.env.DEPLOYMENT_CONFIG_FILE ? process.env.DEPLOYMENT_CONFIG_FILE : 'deployment-settings.yml'
+
+    let config = {}
+    if (fs.existsSync(deploymentConfigPath)) {
+      config = yaml.load(fs.readFileSync(deploymentConfigPath))
+    }
+
+    this.overrideValidators = {}
+    if (config.overridevalidators) {
+      if (!Array.isArray(config.overridevalidators)) {
+        throw new Error('overridevalidators must be an array')
+      }
+      for (const validator of config.overridevalidators) {
+        // eslint-disable-next-line no-new-func
+        const f = new Function('baseconfig', 'overrideconfig', 'githubContext', validator.script)
+        this.overrideValidators[validator.plugin] = { canOverride: f, error: validator.error }
+      }
     }
 
-    static isIterable (obj) {
-        // checks for null and undefined
-        if (obj == null) {
-          return false
-        }
-        return typeof obj[Symbol.iterator] === 'function'
+    this.configValidators = {}
+    if (config.configvalidators) {
+      if (!Array.isArray(config.configvalidators)) {
+        throw new Error('configvalidators must be an array')
       }
+      for (const validator of config.configvalidators) {
+        // eslint-disable-next-line no-new-func
+        const f = new Function('baseconfig', 'githubContext', validator.script)
+        this.configvalidators[validator.plugin] = { isValid: f, error: validator.error }
+      }
+    }
+
+    this.restrictedRepos = config.restrictedRepos ?? ['admin', '.github', 'safe-settings']
 
-    constructor (nop, context, repo, config, ref, suborg) {
+    this.unsafeFields = []
+    // eslint-disable-next-line dot-notation
+    if (config['unsafe_fields']) {
+      // eslint-disable-next-line dot-notation
+      if (!Array.isArray(config['unsafe_fields'])) {
+        throw new Error('unsafe_fields must be an array')
+      }
+      // eslint-disable-next-line dot-notation
+      this.unsafeFields = config['unsafe_fields']
     }
+  }
 }
+
 DeploymentConfig.FILE_NAME = `${env.CONFIG_PATH}/settings.yml`
 
 module.exports = DeploymentConfig

lib/mergeDeep.js

@@ -1,17 +1,16 @@
 const mergeBy = require('./mergeArrayBy')
-const DeploymentConfig = require('./deploymentConfig')
 
 const NAME_FIELDS = ['name', 'username', 'actor_id', 'login', 'type', 'key_prefix']
 const NAME_USERNAME_PROPERTY = item => NAME_FIELDS.find(prop => Object.prototype.hasOwnProperty.call(item, prop))
 const GET_NAME_USERNAME_PROPERTY = item => { if (NAME_USERNAME_PROPERTY(item)) return item[NAME_USERNAME_PROPERTY(item)] }
 
 class MergeDeep {
-  constructor (log, github, ignorableFields = [], configvalidators = {}, overridevalidators = {}) {
+  constructor (log, github, ignorableFields = [], deploymentConfig) {
     this.log = log
     this.github = github
     this.ignorableFields = ignorableFields
-    this.configvalidators = DeploymentConfig.configvalidators
-    this.overridevalidators = DeploymentConfig.overridevalidators
+    this.configvalidators = deploymentConfig.configValidators
+    this.overridevalidators = deploymentConfig.overrideValidators
   }
 
   isObjectNotArray (item) {
@@ -336,11 +335,20 @@ class MergeDeep {
             this.validateConfig(key, target[key])
           }
         } else {
-          // Not calling validators when target[key] is primitive or empty
-          target[key] = source[key]
+          // Handle nulls differently than undefined
+          if (source[key] === null) {
+            // nulls will be respected, and remove the value
+            delete target[key]
+          } else if (source[key] === undefined) {
+            // undefined will be ignored
+          } else {
+            // Not calling validators when target[key] is primitive
+            target[key] = source[key]
+          }
         }
       }
     }
+    this.log.debug(`returning ${JSON.stringify(target)}`)
     return target
   }