Feature/compress pr comment

index.js

@@ -19,16 +19,29 @@ let deploymentConfig
 
 module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => {
   let appSlug = 'safe-settings'
-  async function syncAllSettings (nop, context, repo = context.repo(), ref) {
+  async function syncAllSettings (nop, context, repo = context.repo(), ref, baseRef, changedFiles = {}) {
     try {
       deploymentConfig = await loadYamlFileSystem()
       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)
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
+
+      // Load base branch config for NOP filtering (only show PR-introduced changes)
+      let baseConfig = null
+      if (nop && baseRef) {
+        try {
+          const baseConfigManager = new ConfigManager(context, baseRef)
+          const baseRuntimeConfig = await baseConfigManager.loadGlobalSettingsYaml()
+          baseConfig = Object.assign({}, deploymentConfig, baseRuntimeConfig)
+        } catch (e) {
+          robot.log.debug(`Could not load base config for NOP filtering: ${e.message}`)
+        }
+      }
+
       if (ref) {
-        return Settings.syncAll(nop, context, repo, config, ref)
+        return Settings.syncAll(nop, context, repo, config, ref, baseConfig, changedFiles)
       } else {
         return Settings.syncAll(nop, context, repo, config)
       }
@@ -73,15 +86,28 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     }
   }
 
-  async function syncSelectedSettings (nop, context, repos, subOrgs, ref) {
+  async function syncSelectedSettings (nop, context, repos, subOrgs, ref, baseRef) {
     try {
       deploymentConfig = await loadYamlFileSystem()
       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)
       robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`)
-      return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref)
+
+      // Load base branch config for NOP filtering
+      let baseConfig = null
+      if (nop && baseRef) {
+        try {
+          const baseConfigManager = new ConfigManager(context, baseRef)
+          const baseRuntimeConfig = await baseConfigManager.loadGlobalSettingsYaml()
+          baseConfig = Object.assign({}, deploymentConfig, baseRuntimeConfig)
+        } catch (e) {
+          robot.log.debug(`Could not load base config for NOP filtering: ${e.message}`)
+        }
+      }
+
+      return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref, baseConfig)
     } catch (e) {
       if (nop) {
         let filename = env.SETTINGS_FILE_PATH
@@ -585,17 +611,21 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     const files = changes.data.map(f => { return f.filename })
 
     const settingsModified = files.includes(Settings.FILE_PATH)
+    const repoChanges = getChangedRepoConfigName(files, context.repo().owner)
+    const subOrgChanges = getChangedSubOrgConfigName(files)
 
     if (settingsModified) {
       robot.log.debug(`Changes in '${Settings.FILE_PATH}' detected, doing a full synch...`)
-      return syncAllSettings(true, context, context.repo(), pull_request.head.ref)
+      const baseRef = pull_request.base.ref || repository.default_branch
+      return syncAllSettings(true, context, context.repo(), pull_request.head.ref, baseRef, {
+        repos: repoChanges,
+        subOrgs: subOrgChanges
+      })
     }
 
-    const repoChanges = getChangedRepoConfigName(files, context.repo().owner)
-    const subOrgChanges = getChangedSubOrgConfigName(files)
-
     if (repoChanges.length > 0 || subOrgChanges.length > 0) {
-      return syncSelectedSettings(true, context, repoChanges, subOrgChanges, pull_request.head.ref)
+      const baseRef = pull_request.base.ref || repository.default_branch
+      return syncSelectedSettings(true, context, repoChanges, subOrgChanges, pull_request.head.ref, baseRef)
     }
 
     // if no safe-settings changes detected, send a success to the check run

lib/commentmessage.js

@@ -1,31 +1,32 @@
-module.exports = `* Run on: \` <%= new Date() %> \`
+module.exports = `* Run on: \`<%= new Date() %>\`
 
-* Number of repos that were considered: \`<%= Object.keys(it.reposProcessed).length %> \`
+* Number of repos that were considered: \`<%= Object.keys(it.reposProcessed).length %>\`
+* Number of repos affected: \`<%= it.reposAffected || 0 %>\`
 
 ### Breakdown of changes
-| Repo <% Object.keys(it.changes).forEach(plugin => { %> | <%= plugin %> settings  <% }) %> |
-| -- <% Object.keys(it.changes).forEach(plugin => { -%>  | --  <% }) %>
-| 
-<% Object.keys(it.reposProcessed).forEach( repo => { -%>
-| <%= repo -%>
-  <%- Object.keys(it.changes).forEach(plugin => { -%>
-    <%_ if (it.changes[plugin][repo]) { -%> |  :hand:  <% } else { %>  |  :grey_exclamation:  <% } -%>
-  <%_ }) -%> |
-<% }) -%>
-
-:hand: -> Changes to be applied to the GitHub repository.
-:grey_exclamation: -> nothing to be changed in that particular GitHub repository.
+<% if (!it.changeSections || it.changeSections.length === 0) { %>
 
-### Breakdown of errors
+No changes to apply.
+<% } else { %>
 
+<%~ it.checkRunDetails %>
+<% } %>
+
+### Breakdown of errors
 <% if (Object.keys(it.errors).length === 0) { %>
+
 \`None\`
 <% } else { %>
-  <% Object.keys(it.errors).forEach(repo => { %>
-    <%_= repo %>: 
-    <% it.errors[repo].forEach(plugin => { %>
-      * <%= plugin.msg %>
-    <% }) %>
 
-  <% }) %>
+<details>
+<summary>Errors by repo</summary>
+
+<% Object.keys(it.errors).forEach(function(repo) { %>
+**<%= repo %>**
+
+<% it.errors[repo].forEach(function(err) { %>* <%= err.msg %>
+<% }) %>
+<% }) %>
+
+</details>
 <% } %>`

lib/plugins/custom_properties.js

@@ -12,24 +12,7 @@ module.exports = class CustomProperties extends Diffable {
 
   // Force all names to lowercase to avoid comparison issues.
   normalizeEntries () {
-    this.entries = this.entries.reduce((normalizedEntries, entry) => {
-      if (!entry || typeof entry !== 'object') {
-        return normalizedEntries
-      }
-
-      const entryName = entry.name || entry.property_name
-
-      if (typeof entryName !== 'string') {
-        return normalizedEntries
-      }
-
-      normalizedEntries.push({
-        name: entryName.toLowerCase(),
-        value: entry.value
-      })
-
-      return normalizedEntries
-    }, [])
+    this.entries = this.normalize(this.entries)
   }
 
   async find () {
@@ -52,24 +35,16 @@ module.exports = class CustomProperties extends Diffable {
 
   // Force all names to lowercase to avoid comparison issues.
   normalize (properties) {
-    return properties.reduce((normalizedProperties, property) => {
-      if (!property || typeof property !== 'object') {
-        return normalizedProperties
-      }
-
-      const propertyName = property.property_name || property.name
-
-      if (typeof propertyName !== 'string') {
-        return normalizedProperties
-      }
-
-      normalizedProperties.push({
-        name: propertyName.toLowerCase(),
-        value: property.value
-      })
-
-      return normalizedProperties
-    }, [])
+    return properties
+      .map(({ property_name: propertyName, name, value }) => ({
+        name: propertyName ?? name,
+        value
+      }))
+      .filter(({ name }) => name != null)
+      .map(({ name, value }) => ({
+        name: name.toLowerCase(),
+        value
+      }))
   }
 
   comparator (existing, attrs) {