Repository: JasonEtco/todo
Pull request #185 (2018-12-30 04:29:57)

Performance guards
I've been noticing some massive spikes in memory usage at seemingly random times, and I think I've narrowed it down to a couple repos committing massive diffs via automation. This PR adds a couple of guards against intentional or unintentional "bad actors" that impact performance of the app:

* Added an `IGNORED_REPOS` environment variable
  * Wrap the handlers to check against that list before fetching the diff
* Always exclude `*.min.*` files
* Don't work with titles over 256 characters. GitHub wouldn't accept them anyway.


Patch:
diff --git a/index.js b/index.js
index 306f9b7..521dafc 100644
--- a/index.js
+++ b/index.js
@@ -2,17 +2,18 @@ const pullRequestHandler = require('./lib/pull-request-handler')
 const pullRequestMergedHandler = require('./lib/pull-request-merged-handler')
 const pushHandler = require('./lib/push-handler')
 const issueRenameHandler = require('./lib/issue-rename-handler')
+const ignoreRepos = require('./lib/ignore-repos')
 
 module.exports = app => {
   // PR handler (comments on pull requests)
-  app.on(['pull_request.opened', 'pull_request.synchronize'], pullRequestHandler)
+  app.on(['pull_request.opened', 'pull_request.synchronize'], ignoreRepos(pullRequestHandler))
 
   // Merge handler (opens new issues)
-  app.on('pull_request.closed', pullRequestMergedHandler)
+  app.on('pull_request.closed', ignoreRepos(pullRequestMergedHandler))
 
   // Push handler (opens new issues)
-  app.on('push', pushHandler)
+  app.on('push', ignoreRepos(pushHandler))
 
   // Prevent tampering with the issue title
-  app.on('issues.edited', issueRenameHandler)
+  app.on('issues.edited', ignoreRepos(issueRenameHandler))
 }
diff --git a/lib/ignore-repos.js b/lib/ignore-repos.js
new file mode 100644
index 0000000..eff2a72
--- /dev/null
+++ b/lib/ignore-repos.js
@@ -0,0 +1,17 @@
+module.exports = function ignoreRepos (handler) {
+  return async context => {
+    const { owner, repo } = context.repo()
+
+    if (process.env.IGNORED_REPOS) {
+      const repos = process.env.IGNORED_REPOS.split(',')
+      const fullName = `${owner}/${repo}`
+      if (repos.includes(fullName)) {
+        context.log(`Specifically ignoring ${fullName} based on IGNORED_REPOS`)
+        return
+      }
+    }
+
+    context.log({ owner, repo }, 'Received event')
+    return handler(context)
+  }
+}
diff --git a/lib/utils/main-loop.js b/lib/utils/main-loop.js
index 6b12bdc..8a805e7 100644
--- a/lib/utils/main-loop.js
+++ b/lib/utils/main-loop.js
@@ -46,8 +46,9 @@ module.exports = async (context, handler) => {
         // Trim whitespace to ensure a clean title
         const title = matches.groups.title.trim()
 
-        // The match might only be whitespace
-        if (!title) continue
+        // This might have matched a minified file, or something
+        // huge. GitHub wouldn't allow this anyway, so let's just ignore it.
+        if (!title || title.length > 256) continue
 
         // Get the details of this commit or PR
         const deets = getDetails({ context, config, chunk, line: change.ln || change.ln2 })
diff --git a/lib/utils/should-exclude-file.js b/lib/utils/should-exclude-file.js
index 2ba8910..4c6575b 100644
--- a/lib/utils/should-exclude-file.js
+++ b/lib/utils/should-exclude-file.js
@@ -1,7 +1,12 @@
+const alwaysExclude = /\.min\./
+
 module.exports = (logger, filename, excludePattern) => {
   if (filename === '.github/config.yml') {
     logger.debug('Skipping .github/config.yml')
     return true
+  } else if (alwaysExclude.test(filename)) {
+    logger.debug('Skipping ' + filename + ' as it matches the the alwaysExclude pattern')
+    return true
   } else if (excludePattern && new RegExp(excludePattern).test(filename)) {
     logger.debug('Skipping ' + filename + ' as it matches the exclude pattern ' + excludePattern)
     return true


Test patch:
diff --git a/tests/lib/ignore-repos.test.js b/tests/lib/ignore-repos.test.js
new file mode 100644
index 0000000..6f54881
--- /dev/null
+++ b/tests/lib/ignore-repos.test.js
@@ -0,0 +1,30 @@
+const ignoreRepos = require('../../lib/ignore-repos')
+
+describe('ignoreRepos', () => {
+  let spy, func, context
+
+  beforeEach(() => {
+    spy = jest.fn()
+    func = ignoreRepos(spy)
+    context = {
+      repo: () => ({ owner: 'JasonEtco', repo: 'todo' }),
+      log: jest.fn()
+    }
+  })
+
+  it('does call the handler if the repo is not being ignored', () => {
+    process.env.IGNORED_REPOS = 'JasonEtco/pizza'
+    func(context)
+    expect(spy).toHaveBeenCalled()
+  })
+
+  it('does not call the handler if the repo is being ignored', () => {
+    process.env.IGNORED_REPOS = 'JasonEtco/todo'
+    func(context)
+    expect(spy).not.toHaveBeenCalled()
+  })
+
+  afterEach(() => {
+    delete process.env.IGNORED_REPOS
+  })
+})
diff --git a/tests/lib/should-exclude-file.test.js b/tests/lib/should-exclude-file.test.js
new file mode 100644
index 0000000..37e077c
--- /dev/null
+++ b/tests/lib/should-exclude-file.test.js
@@ -0,0 +1,24 @@
+const shouldExcludeFile = require('../../lib/utils/should-exclude-file')
+
+describe('shouldExcludeFile', () => {
+  let logger
+
+  beforeEach(() => {
+    logger = { debug: jest.fn() }
+  })
+
+  it('returns true for the .github/config.yml file', () => {
+    const actual = shouldExcludeFile(logger, '.github/config.yml')
+    expect(actual).toBe(true)
+  })
+
+  it('returns true for any files that match the alwaysExclude RegEx', () => {
+    const actual = shouldExcludeFile(logger, 'example.min.js')
+    expect(actual).toBe(true)
+  })
+
+  it('returns true for any files that match the provded pattern', () => {
+    const actual = shouldExcludeFile(logger, 'pizza.js', 'pi')
+    expect(actual).toBe(true)
+  })
+})