diff --git a/.travis.yml b/.travis.yml index be1bb65..3bb65ee 100644 --- a/.travis.yml +++ b/.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 diff --git a/README.md b/README.md index 7e2be8b..103901f 100644 --- a/README.md +++ b/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. diff --git a/exe/pr-checklist.rb b/exe/pr-checklist.rb index 5c30141..97c5d6f 100755 --- a/exe/pr-checklist.rb +++ b/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| diff --git a/lib/deploy_complexity/checklists.rb b/lib/deploy_complexity/checklists.rb index 753f9b2..0af9ddb 100644 --- a/lib/deploy_complexity/checklists.rb +++ b/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 diff --git a/spec/lib/deploy_complexity/checklists_spec.rb b/spec/lib/deploy_complexity/checklists_spec.rb index 3f4d6de..96558d4 100644 --- a/spec/lib/deploy_complexity/checklists_spec.rb +++ b/spec/lib/deploy_complexity/checklists_spec.rb @@ -105,68 +105,6 @@ def relevant_for(files) end end - describe 'ElmFactoriesChecklist' do - subject { Checklists::ElmFactoriesChecklist.new } - - it_behaves_like "a checklist class" - - it "should be relevant for elm specs" do - expect(subject).to be_relevant_for("ui/tests/SomeNeatSpec.elm") - end - end - - describe 'CapistranoChecklist' do - subject { Checklists::CapistranoChecklist.new } - - it_behaves_like "a checklist class" - - it "should be relevant for files under lib/capistrano/" do - expect(subject).to be_relevant_for("lib/capistrano/tasks/foobar.rake") - end - - it "should be relevant for the Capfile" do - expect(subject).to be_relevant_for("Capfile") - end - - it "should be relevant for the Gemfile" do - expect(subject).to be_relevant_for("Gemfile") - end - - it "should be relevant for files under lib/deploy/" do - expect(subject).to be_relevant_for("lib/deploy/foobar.rb") - end - - it "should be relevant for files with cap as a word in their name" do - expect(subject).to be_relevant_for("script/a_cap_ital_idea.sh") - end - - it "should not be relevant for files with 'cap' as part of another word" do - expect(subject).to_not be_relevant_for("a_capital_idea.rb") - end - end - - describe 'OpsWorksChecklist' do - subject { Checklists::OpsWorksChecklist.new } - - it_behaves_like "a checklist class" - - it "should be relevant for any file with opsworks in the name" do - expect(subject).to be_relevant_for("script/opsworks-foo.rb") - end - - it "should be relevant for config/deploy.rb" do - expect(subject).to be_relevant_for("config/deploy.rb") - end - - it "should be relevant for files under deploy/" do - expect(subject).to be_relevant_for("deploy/before_migrate.rb") - end - - it "should be relevant for files under lib/deploy/" do - expect(subject).to be_relevant_for("lib/deploy/foobar.rb") - end - end - describe 'RoutesChecklist' do subject { Checklists::RoutesChecklist.new } @@ -186,58 +124,5 @@ def relevant_for(files) expect(subject).to be_relevant_for("app/jobs/foobar_job.rb") end end - - describe 'MigrationChecklist' do - subject { Checklists::MigrationChecklist.new } - - it_behaves_like "a checklist class" - - it "should be relevant for migrations" do - expect(subject).to be_relevant_for("db/migrate/hey_whats_up.rb") - end - end - - describe "DockerfileChecklist" do - subject { Checklists::DockerfileChecklist.new } - - it_behaves_like "a checklist class" - - it "should be relevant for Dockerfile" do - expect(subject).to be_relevant_for("Dockerfile") - end - end - - describe 'NixChecklist' do - subject { Checklists::NixChecklist.new } - - it_behaves_like "a checklist class" - - it "should be relevant for any nix file" do - expect(subject).to be_relevant_for("shell.nix") - end - - it "should be relevant for any file in the nix directory" do - expect(subject).to be_relevant_for("nix/something.sh") - end - - it "should be relevant for any Ruby package file" do - expect(subject).to be_relevant_for("Gemfile") - expect(subject).to be_relevant_for("Gemfile.lock") - end - - it "should be relevant for any Node package file" do - expect(subject).to be_relevant_for("package.json") - expect(subject).to be_relevant_for("package-lock.json") - end - - it "should be relevant for any Node package file in a subdirectory" do - expect(subject).to be_relevant_for("subdir/package.json") - expect(subject).to be_relevant_for("subdir/package-lock.json") - end - - it "should be releavnt for any Python package file" do - expect(subject).to be_relevant_for("requirements.txt") - end - end end end