feat: separate runtime and deployment configs

Author: jonathanmorley

index.js

@@ -4,6 +4,7 @@ const fs = require('fs')
 const cron = require('node-cron')
 const Glob = require('./lib/glob')
 const ConfigManager = require('./lib/configManager')
+const DeploymentConfig = require('./lib/deploymentConfig')
 const NopCommand = require('./lib/nopcommand')
 const env = require('./lib/env')
 
@@ -13,11 +14,11 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
   let appSlug = 'safe-settings'
   async function syncAllSettings (nop, context, repo = context.repo(), ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
+      deploymentConfig = await loadYamlFileSystem(context)
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = { deploymentConfig, runtimeConfig }
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       if (ref) {
         return Settings.syncAll(nop, context, repo, config, ref)
@@ -42,11 +43,11 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
 
   async function syncSubOrgSettings (nop, context, suborg, repo = context.repo(), ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
+      deploymentConfig = await loadYamlFileSystem(context)
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = { deploymentConfig, runtimeConfig }
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       return Settings.syncSubOrgs(nop, context, suborg, repo, config, ref)
     } catch (e) {
@@ -67,11 +68,11 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
 
   async function syncSettings (nop, context, repo = context.repo(), ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
+      deploymentConfig = await loadYamlFileSystem(context)
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
+      const config = { deploymentConfig, runtimeConfig }
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
       return Settings.sync(nop, context, repo, config, ref)
     } catch (e) {
@@ -92,14 +93,14 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
 
   async function renameSync (nop, context, repo = context.repo(), rename, ref) {
     try {
-      deploymentConfig = await loadYamlFileSystem()
+      deploymentConfig = await loadYamlFileSystem(context)
       robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`)
       const configManager = new ConfigManager(context, ref)
       const runtimeConfig = await configManager.loadGlobalSettingsYaml()
-      const config = Object.assign({}, deploymentConfig, runtimeConfig)
-      const renameConfig = Object.assign({}, config, rename)
+      const renameConfig = Object.assign({}, runtimeConfig, rename)
+      const config = { deploymentConfig, runtimeConfig: renameConfig }
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
-      return Settings.sync(nop, context, repo, renameConfig, ref)
+      return Settings.sync(nop, context, repo, config, ref)
     } catch (e) {
       if (nop) {
         let filename = env.SETTINGS_FILE_PATH
@@ -121,16 +122,8 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
    *
    * @return The parsed YAML file
    */
-  async function loadYamlFileSystem () {
-    if (deploymentConfig === undefined) {
-      const deploymentConfigPath = env.DEPLOYMENT_CONFIG_FILE
-      if (fs.existsSync(deploymentConfigPath)) {
-        deploymentConfig = yaml.load(fs.readFileSync(deploymentConfigPath))
-      } else {
-        deploymentConfig = { restrictedRepos: ['admin', '.github', 'safe-settings'] }
-      }
-    }
-    return deploymentConfig
+  async function loadYamlFileSystem (context) {
+    return new DeploymentConfig(context)
   }
 
   function getAllChangedSubOrgConfigs (payload) {

lib/deploymentConfig.js

@@ -2,50 +2,43 @@ const yaml = require('js-yaml')
 const fs = require('fs')
 const env = require('./env')
 
+function isNonEmptyArray (obj) {
+  return Array.isArray(obj) && obj.length > 0
+}
+
 /**
  * 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.
  */
-class DeploymentConfig {
-  // static config
-  static configvalidators = {}
-  static overridevalidators = {}
+module.exports = class DeploymentConfig {
+  constructor (context, configPath) {
+    const deploymentConfigPath = configPath ?? env.DEPLOYMENT_CONFIG_FILE
 
-  static {
-    const deploymentConfigPath = process.env.DEPLOYMENT_CONFIG_FILE ? process.env.DEPLOYMENT_CONFIG_FILE : 'deployment-settings.yml'
+    let deploymentConfig = {}
     if (fs.existsSync(deploymentConfigPath)) {
-      this.config = yaml.load(fs.readFileSync(deploymentConfigPath)) || {}
+      deploymentConfig = yaml.load(fs.readFileSync(deploymentConfigPath)) ?? {}
     } else {
-      this.config = { restrictedRepos: ['admin', '.github', 'safe-settings'] }
+      context.log.info(`No deployment settings found at ${deploymentConfigPath}`)
     }
 
-    const overridevalidators = this.config.overridevalidators || []
-    if (this.isNonEmptyArray(overridevalidators)) {
-      for (const validator of overridevalidators) {
+    this.overridevalidators = {}
+    if (isNonEmptyArray(deploymentConfig.overridevalidators)) {
+      for (const validator of deploymentConfig.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.isNonEmptyArray(configvalidators)) {
-      for (const validator of configvalidators) {
+
+    this.configvalidators = {}
+    if (isNonEmptyArray(deploymentConfig.configvalidators)) {
+      for (const validator of deploymentConfig.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 }
       }
     }
-  }
 
-  static isNonEmptyArray (obj) {
-    return Array.isArray(obj) && obj.length > 0
-  }
-
-  // eslint-disable-next-line no-useless-constructor
-  constructor (nop, context, repo, config, ref, suborg) {
+    this.restrictedRepos = deploymentConfig.restrictedRepos ?? ['admin', '.github', 'safe-settings']
   }
 }
-DeploymentConfig.FILE_NAME = `${env.CONFIG_PATH}/settings.yml`
-
-module.exports = DeploymentConfig

lib/mergeDeep.js

@@ -6,12 +6,14 @@ const NAME_USERNAME_PROPERTY = item => NAME_FIELDS.find(prop => Object.prototype
 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 = []) {
     this.log = log
     this.github = github
     this.ignorableFields = ignorableFields
-    this.configvalidators = DeploymentConfig.configvalidators
-    this.overridevalidators = DeploymentConfig.overridevalidators
+
+    const deploymentConfig = new DeploymentConfig({ log })
+    this.configvalidators = deploymentConfig.configvalidators
+    this.overridevalidators = deploymentConfig.overridevalidators
   }
 
   isObjectNotArray (item) {

lib/settings.js

@@ -10,6 +10,7 @@ const env = require('./env')
 const CONFIG_PATH = env.CONFIG_PATH
 const eta = new Eta({ views: path.join(__dirname) })
 const SCOPE = { ORG: 'org', REPO: 'repo' } // Determine if the setting is a org setting or repo setting
+
 class Settings {
   static async syncAll (nop, context, repo, config, ref) {
     const settings = new Settings(nop, context, repo, config, ref)
@@ -65,7 +66,6 @@ class Settings {
     this.installation_id = context.payload.installation.id
     this.github = context.octokit
     this.repo = repo
-    this.config = config
     this.nop = nop
     this.suborgChange = !!suborg
     // If suborg config has been updated, do not load the entire suborg config, and only process repos restricted to it.
@@ -75,26 +75,15 @@ class Settings {
     this.log = context.log
     this.results = []
     this.errors = []
-    this.configvalidators = {}
-    this.overridevalidators = {}
-    const overridevalidators = 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 = config.configvalidators
-    if (this.isIterable(configvalidators)) {
-      for (const validator of configvalidators) {
-        this.log.debug(`Logging each script: ${typeof validator.script}`)
-        // 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.mergeDeep = new MergeDeep(this.log, this.github, [], this.configvalidators, this.overridevalidators)
+
+    this.mergeDeep = new MergeDeep(this.log, this.github, [])
+
+    this.config = config.runtimeConfig
+
+    // these can only be defined in the deployment config
+    this.overridevalidators = config.deploymentConfig.overridevalidators
+    this.configvalidators = config.deploymentConfig.configvalidators
+    this.restrictedRepos = config.deploymentConfig.restrictedRepos
   }
 
   // Create a check in the Admin repo for safe-settings.
@@ -449,7 +438,7 @@ ${this.results.reduce((x, y) => {
   }
 
   isRestricted(repoName) {
-    const restrictedRepos = this.config.restrictedRepos
+    const restrictedRepos = this.restrictedRepos
     // Skip configuring any restricted repos
     if (Array.isArray(restrictedRepos)) {
       // For backward compatibility support the old format
@@ -891,14 +880,6 @@ ${this.results.reduce((x, y) => {
   isObject (item) {
     return (item && typeof item === 'object' && !Array.isArray(item))
   }
-
-  isIterable(obj) {
-    // checks for null and undefined
-    if (obj == null) {
-      return false
-    }
-    return typeof obj[Symbol.iterator] === 'function'
-  }
 }
 
 function prettify (obj) {

test/unit/lib/deploymentConfig.test.js

@@ -0,0 +1,62 @@
+const DeploymentConfig = require('../../../lib/deploymentConfig')
+
+const defaultConfig = {
+  configvalidators: {},
+  overridevalidators: {},
+  restrictedRepos: ['admin', '.github', 'safe-settings']
+}
+
+const context = { log: { info: jest.fn() } }
+
+describe('no deploymentConfig', () => {
+  const deploymentConfig = new DeploymentConfig(context, 'nonexistent.yml')
+
+  test('matches default config', () => {
+    expect(deploymentConfig).toMatchObject(defaultConfig)
+  })
+
+  test('outputs info message', () => {
+    expect(context.log.info).toHaveBeenCalledWith('No deployment settings found at nonexistent.yml')
+  })
+})
+
+describe('sample deploymentConfig', () => {
+  const deploymentConfig = new DeploymentConfig(context, './docs/sample-settings/sample-deployment-settings.yml')
+
+  test('matches snapshot', () => {
+    expect(deploymentConfig).toMatchInlineSnapshot(`
+DeploymentConfig {
+  "configvalidators": {
+    "collaborators": {
+      "error": "\`Admin cannot be assigned to collaborators\`
+",
+      "isValid": [Function],
+    },
+  },
+  "overridevalidators": {
+    "branches": {
+      "canOverride": [Function],
+      "error": "\`Branch protection required_approving_review_count cannot be overidden to a lower value\`
+",
+    },
+    "labels": {
+      "canOverride": [Function],
+      "error": "Some error
+",
+    },
+  },
+  "restrictedRepos": {
+    "exclude": [
+      "^admin$",
+      "^\\.github$",
+      "^safe-settings$",
+      ".*-test",
+    ],
+    "include": [
+      "^test$",
+    ],
+  },
+}
+`)
+  })
+})