Merge pull request #20 from NoRedInk/custom-pr-checklist

Allow specification of custom checklists from inside of client repo

.travis.yml

@@ -10,7 +10,7 @@ cache:
 
 before_install:
   - gem update --system
-  - gem install bundler
+  - gem install bundler:1.16.3
 
 script:
   - bundle exec rubocop

README.md

@@ -31,6 +31,44 @@ Use `GIT_DIR` to run a local copy of `pr-checklist` or `deploy-complexity` again
 GIT_DIR=../repo bundle exec ./exe/pr-checklist.rb -b branch
 ```
 
+### Custom Checklists
+
+If you want to define checklists within your repo, create a ruby file `tools/deploy_complexity/checklists.rb`:
+
+```
+module Checklists
+  # Example checklist item
+  class BlarghDetector < Checklist
+    def human_name
+      "Blarg!"
+    end
+    
+    def checklist
+    '- [ ] Removed extraneous blargh'.strip
+    end
+    
+    def relevant_for(files)
+      files.select do |file| 
+        file.ends_with(".rb") && IO.read(file) =~ /blargh/
+      end
+    end
+  end
+
+  module_function
+  
+  # List of checklists to run
+  def checklists
+    [BlarghDetector].freeze
+  end
+end
+```
+
+And then execute the pr-checklist runner inside of CI using:
+
+```
+bundle exec pr-checklist.rb -b branch -c tools/deploy_complexity/checklists.rb
+```
+
 ### Github Token
 
 pr-checklist.rb requires a github token with role REPO to edit PR descriptions and comment on a PR. Make sure that `GITHUB_TOKEN` is set in the environment.

exe/pr-checklist.rb

@@ -8,7 +8,7 @@
 
 # options and validation
 class Options
-  attr_writer :branch, :token, :org, :repo, :dry_run
+  attr_writer :branch, :token, :org, :repo, :dry_run, :checklist
 
   def branch
     # origin/master or master are both fine, but we need to drop origin/
@@ -36,6 +36,10 @@ def repo
   def dry_run
     @dry_run || false
   end
+
+  def checklist
+    @checklist || nil
+  end
 end
 
 options = Options.new
@@ -65,6 +69,11 @@ def dry_run
     "-n", "--dry-run",
     "Check things, but do not make any edits or comments"
   ) { |dry_run| options.dry_run = dry_run }
+
+  opts.on(
+    "-c", "--custom-checklist config.rb", String,
+    "Specify external ruby file to load checklists from"
+  ) { |checklist| options.checklist = checklist }
 end.parse!
 
 puts "Checking branch #{options.branch}..."
@@ -80,7 +89,14 @@ def dry_run
 
 puts "Found pull request #{pr}"
 files_changed = `git diff --name-only '#{pr.base}...#{pr.head}'`.split("\n")
-checklists = Checklists.for_files(files_changed)
+
+# conditionally load externally defined checklist from project
+if options.checklist
+  puts "Loading external configuration from %s" % [options.checklist]
+  load options.checklist
+end
+
+checklists = Checklists.for_files(Checklists.checklists, files_changed)
 new_checklists = pr.update_with_checklists(checklists, options.dry_run)
 
 new_checklists.each do |checklist, files|

lib/deploy_complexity/checklists.rb

@@ -46,76 +46,6 @@ def relevant_for(files)
     end
   end
 
-  class ElmFactoriesChecklist < Checklist
-    def human_name
-      "Elm Factories"
-    end
-
-    def checklist
-      '
-- [ ] Elm fuzz tests: use [shortList](https://github.com/NoRedInk/NoRedInk/blob/72626abf20e44eb339dd60ebb716e9447910127f/ui/tests/SpecHelpers.elm#L59) when a list fuzzer is generating too many cases
-      '.strip
-    end
-
-    def relevant_for(files)
-      files.select { |file| file.start_with?("ui/tests/") }
-    end
-  end
-
-  class CapistranoChecklist < Checklist
-    def human_name
-      "Capistrano"
-    end
-
-    def checklist
-      "
-The process for testing capistrano is to deploy the capistrano changes branch to staging prior to merging to master and verify the deploy doesn't explode.
-
-- [ ] Make a branch with capistrano changes
-- [ ] Wait for free time to test staging
-- [ ] Reset/deploy that branch to staging using the normal jenkins deploy process
-- [ ] Verify the deploy passes
-  - If it doesn't, fix the branch and redeploy until it works
-  - [ ] If it does, reset back to origin/master and request review of the PR
-      ".strip
-    end
-
-    def relevant_for(files)
-      files.select do |file|
-        file == "Capfile" \
-          || file == "Gemfile" \
-          || file.start_with?("lib/capistrano/") \
-          || file.start_with?("lib/deploy/") \
-          || file.start_with?("config/deploy") \
-          || !file.match('.*[\b_\./]cap[\b_\./].*').nil?
-      end
-    end
-  end
-
-  class OpsWorksChecklist < Checklist
-    def human_name
-      "OpsWorks"
-    end
-
-    def checklist
-      "
-- [ ] Change the source code branch for staging to the branch being tested in the opsworks UI
-- [ ] Rebase your code over `origin/staging` to prevent a successful deploy of your changes from making staging run possibly outdated code
-- [ ] Turn on an additional time-based instance in the layer ([see instructions](https://github.com/NoRedInk/wiki/blob/1f618042ed1d6b7c7297ec2672ae568e57944fde/ops-playbook/ops-plays.md#using-opsworks-to-bring-up-an-additional-time-based-instance))
-- [ ] Verify that the instances passes setup to online and doesn't fail
-      ".strip
-    end
-
-    def relevant_for(files)
-      files.select do |file|
-        file.start_with?("config/deploy") \
-          || file.include?("opsworks") \
-          || file.start_with?("deploy/") \
-          || file.start_with?("lib/deploy/")
-      end
-    end
-  end
-
   class RoutesChecklist < Checklist
     def human_name
       "Routes"
@@ -148,73 +78,6 @@ def relevant_for(files)
     end
   end
 
-  class MigrationChecklist < Checklist
-    def human_name
-      "Migrations"
-    end
-
-    def checklist
-      '
-- [ ] If there are any potential [Slow Migrations](https://github.com/NoRedInk/wiki/blob/master/Slow-Migrations.md), make sure that:
-  - [ ] They are in separate PRs so each can be run independently
-  - [ ] There is a deployment plan where the resulting code on prod will support the db schema both before and after the migration
-- [ ] If migrations include dropping a column, modifying a column, or adding a non-nullable column, ensure the previously deployed model is prepared to handle both the previous schema and the new schema. ([See "Rails Migrations with Zero Downtime](https://blog.codeship.com/rails-migrations-zero-downtime/)")
-      '.strip
-    end
-
-    def relevant_for(files)
-      files.select { |file| file.start_with? "db/migrate/" }
-    end
-  end
-
-  class DockerfileChecklist < Checklist
-    def human_name
-      "Dockerfile"
-    end
-
-    def checklist
-      "
-- [ ] Dependencies added or changed in the Dockerfile are mirrored in the production/staging/demo environment [Cookbooks](https://github.com/NoRedInk/NoRedInk-opsworks/blob/master/noredink/recipes/setup.rb)
-- In case of dependency changes, ensure Capistrano deploys still work:
-  - [ ] Wait for free time to test staging
-  - [ ] Reset/deploy that branch to staging using the normal jenkins deploy process
-  - [ ] Verify the deploy passes
-    - If it doesn't, fix the branch and redeploy until it works
-    - [ ] If it does, reset back to origin/master and request review of the PR
-      ".strip
-    end
-
-    def relevant_for(files)
-      files.select { |file| file.include? "Dockerfile" }
-    end
-  end
-
-  class NixChecklist < Checklist
-    def human_name
-      "Nix"
-    end
-
-    def checklist
-      '
-- [Instructions on how to use Nix](https://github.com/NoRedInk/wiki/blob/master/engineering/using-nix.md)
-- [ ] changes build successfully with Nix (`nix-shell --pure` to check)
-- [ ] once approved, but before merging, make sure to update the Nix cache so that other people don\'t have to rebuild all changes. Run `script/cache_nix_shell.sh`.
-      '
-    end
-
-    def relevant_for(files)
-      files.select do |file|
-        file.start_with?("nix") \
-          || file.end_with?("nix") \
-          || file == "Gemfile" \
-          || file == "Gemfile.lock" \
-          || file.end_with?("package.json") \
-          || file.end_with?("package-lock.json") \
-          || file == "requirements.txt"
-      end
-    end
-  end
-
   # all done!
   # rubocop:enable Style/Documentation
   # rubocop:enable Metrics/LineLength
@@ -236,19 +99,16 @@ def for_files(files)
 
   module_function
 
-  CHECKLISTS = [
-    RubyFactoriesChecklist,
-    ElmFactoriesChecklist,
-    CapistranoChecklist,
-    OpsWorksChecklist,
-    RoutesChecklist,
-    ResqueChecklist,
-    MigrationChecklist,
-    DockerfileChecklist,
-    NixChecklist
-  ].freeze
-
-  def for_files(files)
-    Checker.new(CHECKLISTS).for_files(files)
+  def checklists
+    [
+      RubyFactoriesChecklist,
+      RoutesChecklist,
+      ResqueChecklist
+    ].freeze
+  end
+
+  def for_files(checklists, files)
+    puts "Applying %s" % [checklists.join(",").gsub(/Checklists::/, '')]
+    Checker.new(checklists).for_files(files)
   end
 end