From abc19237585ddaa69f24d921ef956e69335fdd8c Mon Sep 17 00:00:00 2001 From: Jason Frey Date: Tue, 6 Feb 2018 17:45:00 -0500 Subject: [PATCH] Change PrMergeabilityChecker to be queued by BranchMergeabilityChecker. Instead of doing something "special" for regular branches in the commit monitor and doing "branch" handlers in a special way, instead create a BranchMergeabilityChecker which monitors regular branches for changes and the queues up individual PrMergeabilityChecker runs for each PR that is targeting the branch in question. --- app/models/branch.rb | 5 ++++ app/models/repo.rb | 14 +++++++-- .../branch_mergeability_checker.rb | 19 ++++++++++++ .../branch => }/pr_mergeability_checker.rb | 13 ++++---- spec/factories/branch.rb | 2 +- spec/models/branch_spec.rb | 13 ++++++++ spec/models/repo_spec.rb | 30 +++++++++++++++---- .../branch_mergeability_checker_spec.rb | 13 ++++++++ .../pr_mergeability_checker_spec.rb | 2 +- 9 files changed, 92 insertions(+), 19 deletions(-) create mode 100644 app/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker.rb rename app/workers/{commit_monitor_handlers/branch => }/pr_mergeability_checker.rb (78%) create mode 100644 spec/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker_spec.rb rename spec/workers/{commit_monitor_handlers/branch => }/pr_mergeability_checker_spec.rb (96%) diff --git a/app/models/branch.rb b/app/models/branch.rb index f9ab8692..976ab852 100644 --- a/app/models/branch.rb +++ b/app/models/branch.rb @@ -15,6 +15,7 @@ class Branch < ActiveRecord::Base after_initialize(:unless => :commit_uri) { self.commit_uri = self.class.github_commit_uri(repo.try(:name)) } scope :regular_branches, -> { where(:pull_request => [false, nil]) } + scope :pr_branches, -> { where(:pull_request => true) } def self.with_branch_or_pr_number(n) n = MinigitService.pr_branch(n) if n.kind_of?(Fixnum) @@ -65,6 +66,10 @@ def pr_number MinigitService.pr_number(name) if pull_request? end + def fq_pr_number + "#{fq_repo_name}##{pr_number}" if pull_request? + end + def pr_title_tags pr_title.to_s.match(/^(?:\s*\[\w+\])+/).to_s.gsub("[", " [").split.map { |s| s[1...-1] } end diff --git a/app/models/repo.rb b/app/models/repo.rb index ff5efb17..e8f34275 100644 --- a/app/models/repo.rb +++ b/app/models/repo.rb @@ -62,15 +62,23 @@ def with_travis_service end def branch_names - branches.collect(&:name) + branches.pluck(:name) + end + + def regular_branches + branches.regular_branches + end + + def regular_branch_names + regular_branches.pluck(:name) end def pr_branches - branches.select(&:pull_request?) + branches.pr_branches end def pr_branch_names - pr_branches.collect(&:name) + pr_branches.pluck(:name) end # @param expected [Array] The desired state of the PR branches. diff --git a/app/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker.rb b/app/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker.rb new file mode 100644 index 00000000..a7945ff2 --- /dev/null +++ b/app/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker.rb @@ -0,0 +1,19 @@ +class CommitMonitorHandlers::CommitRange::BranchMergeabilityChecker + include Sidekiq::Worker + sidekiq_options :queue => :miq_bot + + include BranchWorkerMixin + + def self.handled_branch_modes + [:regular] + end + + def perform(branch_id, _new_commits) + return unless find_branch(branch_id, :regular) + + repo.pr_branches.where(:merge_target => branch.name).each do |pr| + logger.info("Queueing PrMergeabilityChecker for PR #{pr.fq_pr_number}.") + PrMergeabilityChecker.perform_async(pr.id) + end + end +end diff --git a/app/workers/commit_monitor_handlers/branch/pr_mergeability_checker.rb b/app/workers/pr_mergeability_checker.rb similarity index 78% rename from app/workers/commit_monitor_handlers/branch/pr_mergeability_checker.rb rename to app/workers/pr_mergeability_checker.rb index 44b486d7..f034b8a9 100644 --- a/app/workers/commit_monitor_handlers/branch/pr_mergeability_checker.rb +++ b/app/workers/pr_mergeability_checker.rb @@ -1,4 +1,4 @@ -class CommitMonitorHandlers::Branch::PrMergeabilityChecker +class PrMergeabilityChecker include Sidekiq::Worker sidekiq_options :queue => :miq_bot @@ -6,12 +6,9 @@ class CommitMonitorHandlers::Branch::PrMergeabilityChecker LABEL = "unmergeable".freeze - def self.handled_branch_modes - [:pr] - end - def perform(branch_id) return unless find_branch(branch_id, :pr) + logger.info("Determining mergeability of PR #{branch.fq_pr_number}.") process_mergeability end @@ -38,7 +35,7 @@ def process_mergeability end def write_to_github - logger.info("Updating PR #{branch.pr_number} with mergability comment.") + logger.info("Updating PR #{branch.fq_pr_number} with mergability comment.") GithubService.add_comment( fq_repo_name, @@ -48,13 +45,13 @@ def write_to_github end def apply_label - logger.info("Updating PR #{branch.pr_number} with label #{LABEL.inspect}.") + logger.info("Updating PR #{branch.fq_pr_number} with label #{LABEL.inspect}.") GithubService.add_labels_to_an_issue(fq_repo_name, branch.pr_number, [LABEL]) end def remove_label - logger.info("Updating PR #{branch.pr_number} my removing label #{LABEL.inspect}.") + logger.info("Updating PR #{branch.fq_pr_number} removing label #{LABEL.inspect}.") GithubService.remove_label(fq_repo_name, branch.pr_number, LABEL) rescue Octokit::NotFound # This label is not currently applied, skip diff --git a/spec/factories/branch.rb b/spec/factories/branch.rb index a94b93f4..1ea31fae 100644 --- a/spec/factories/branch.rb +++ b/spec/factories/branch.rb @@ -5,7 +5,6 @@ sequence(:name) { |n| "branch_#{n}" } commit_uri { "https://example.com/#{repo.name}/commit/$commit" } last_commit { SecureRandom.hex(40) } - merge_target "master" repo end @@ -13,6 +12,7 @@ factory :pr_branch, :parent => :branch do sequence(:name) { |n| "prs/#{n}/head" } sequence(:pr_title) { |n| "PR title #{n}" } + merge_target "master" pull_request true end diff --git a/spec/models/branch_spec.rb b/spec/models/branch_spec.rb index 25f76bde..24ec64f8 100644 --- a/spec/models/branch_spec.rb +++ b/spec/models/branch_spec.rb @@ -110,6 +110,19 @@ end end + context "#fq_pr_number" do + it "on pr branch" do + branch.name = "pr/133" + branch.pull_request = true + + expect(branch.fq_pr_number).to eq "test-user/test-repo#133" + end + + it "on regular branch" do + expect(branch.fq_pr_number).to be_nil + end + end + describe "#pr_title_tags" do it "with a nil pr_title" do branch.pr_title = nil diff --git a/spec/models/repo_spec.rb b/spec/models/repo_spec.rb index 7958c6c8..8c686786 100644 --- a/spec/models/repo_spec.rb +++ b/spec/models/repo_spec.rb @@ -91,20 +91,38 @@ expect(repo.branch_names).to match_array([branch1.name, branch2.name]) end + it "#regular_branches" do + repo.save! + _pr_branch = create(:branch, :repo => repo, :pull_request => true) + regular_branch1 = create(:branch, :repo => repo, :pull_request => false) + regular_branch2 = create(:branch, :repo => repo, :pull_request => false) + + expect(repo.regular_branches).to match_array([regular_branch1, regular_branch2]) + end + + it "#regular_branch_names" do + repo.save! + _pr_branch = create(:branch, :repo => repo, :pull_request => true) + regular_branch1 = create(:branch, :repo => repo, :pull_request => false) + regular_branch2 = create(:branch, :repo => repo, :pull_request => false) + + expect(repo.regular_branch_names).to match_array([regular_branch1.name, regular_branch2.name]) + end + it "#pr_branches" do repo.save! - pr_branch1 = create(:branch, :repo => repo, :pull_request => true) - pr_branch2 = create(:branch, :repo => repo, :pull_request => true) - _non_pr_branch = create(:branch, :repo => repo, :pull_request => false) + pr_branch1 = create(:branch, :repo => repo, :pull_request => true) + pr_branch2 = create(:branch, :repo => repo, :pull_request => true) + _regular_branch = create(:branch, :repo => repo, :pull_request => false) expect(repo.pr_branches).to match_array([pr_branch1, pr_branch2]) end it "#pr_branch_names" do repo.save! - pr_branch1 = create(:branch, :repo => repo, :pull_request => true) - pr_branch2 = create(:branch, :repo => repo, :pull_request => true) - _non_pr_branch = create(:branch, :repo => repo, :pull_request => false) + pr_branch1 = create(:branch, :repo => repo, :pull_request => true) + pr_branch2 = create(:branch, :repo => repo, :pull_request => true) + _regular_branch = create(:branch, :repo => repo, :pull_request => false) expect(repo.pr_branch_names).to match_array([pr_branch1.name, pr_branch2.name]) end diff --git a/spec/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker_spec.rb b/spec/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker_spec.rb new file mode 100644 index 00000000..d23944e5 --- /dev/null +++ b/spec/workers/commit_monitor_handlers/commit_range/branch_mergeability_checker_spec.rb @@ -0,0 +1,13 @@ +describe CommitMonitorHandlers::CommitRange::BranchMergeabilityChecker do + let!(:branch) { create(:branch) } + let!(:pr_branch) { create(:pr_branch, :repo => branch.repo, :merge_target => branch.name) } + let!(:pr_branch2) { create(:pr_branch, :repo => branch.repo) } + + before { stub_sidekiq_logger } + + it "queues up PrMergeabilityChecker for PRs targeting this branch" do + expect(PrMergeabilityChecker).to receive(:perform_async).once.with(pr_branch.id) + + described_class.new.perform(branch.id, ["abcde123"]) + end +end diff --git a/spec/workers/commit_monitor_handlers/branch/pr_mergeability_checker_spec.rb b/spec/workers/pr_mergeability_checker_spec.rb similarity index 96% rename from spec/workers/commit_monitor_handlers/branch/pr_mergeability_checker_spec.rb rename to spec/workers/pr_mergeability_checker_spec.rb index 77190018..fc80b0ca 100644 --- a/spec/workers/commit_monitor_handlers/branch/pr_mergeability_checker_spec.rb +++ b/spec/workers/pr_mergeability_checker_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe CommitMonitorHandlers::Branch::PrMergeabilityChecker do +describe PrMergeabilityChecker do before(:each) do stub_sidekiq_logger end