diff --git a/Gemfile b/Gemfile index 8f5dbf2..65cba2e 100644 --- a/Gemfile +++ b/Gemfile @@ -5,4 +5,5 @@ gem "octokit" group :development do gem "pry-byebug" gem "rspec" + gem "hashie" end diff --git a/Gemfile.lock b/Gemfile.lock index fdfb294..3de6f47 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -8,6 +8,7 @@ GEM diff-lcs (1.4.4) faraday (1.0.1) multipart-post (>= 1.2, < 3) + hashie (5.0.0) method_source (1.0.0) multipart-post (2.1.1) octokit (4.18.0) @@ -41,6 +42,7 @@ PLATFORMS ruby DEPENDENCIES + hashie octokit pry-byebug rspec diff --git a/actions/bubble_merge/git_client.rb b/actions/bubble_merge/git_client.rb index cd7ff0f..d66c940 100644 --- a/actions/bubble_merge/git_client.rb +++ b/actions/bubble_merge/git_client.rb @@ -3,7 +3,7 @@ module Squiddy class GitClient - attr_reader :client, :event, :pr, :repo + attr_reader :client, :event, :pr CHECK_STATUSES = %w[queued in_progress completed].freeze CHECK_CONCLUSIONS = %w[success failure neutral cancelled skipped timed_out action_required stale].freeze @@ -16,25 +16,56 @@ def initialize @client = Octokit::Client.new(access_token: ENV["GITHUB_TOKEN"]) @event = git_event @pr = pull_request - @repo = repository end def bubble_merge if already_merged? client.add_comment(repo_name, pr_number, 'This PR is already merged.') else - merge_and_close_pr + wait_for_pr_to_match_branch do |merge_sha| + merge_and_close_pr(merge_sha) + end end end private + def wait_for_pr_to_match_branch + retry_sleeps = [1, 2, 3, 5, 8] + + loop do + expected = branch_head_sha + actual = pull_request_head_sha + + if actual == expected + puts "PR HEAD #{actual[0..10]} matches #{branch} HEAD #{expected[0..10]}; merging..." + yield expected + return + elsif s = retry_sleeps.shift + puts "PR HEAD #{actual[0..10]} did not match #{branch} HEAD #{expected[0..10]}; waiting for #{s}s..." + sleep s + next + else + raise "PR HEAD #{actual[0..10]} did not match #{branch} HEAD #{expected[0..10]}; exiting" + end + end + end + def already_merged? client.pull_merged?(repo_name, pr_number) end - def merge_and_close_pr - merge + def merge_and_close_pr(merge_sha) + puts "PR ##{pr_number}: merging #{branch}@#{merge_sha} into #{base_branch}" + client.merge_pull_request( + repo_name, + pr_number, + commit_message, + { + merge_method: 'merge', + sha: merge_sha + } + ) delete_branch rescue StandardError => e message = <<~MESSAGE @@ -46,59 +77,31 @@ def merge_and_close_pr client.add_comment(repo_name, pr_number, message) end - def merge - client.merge( - repo_name, - base_branch, - branch, - { - merge_method: 'rebase', - commit_message: commit_message, - sha: main_sha - } - ) - end - def delete_branch - return if repo.delete_branch_on_merge + return if repository.delete_branch_on_merge + puts "PR ##{pr_number}: deleting #{branch}" client.delete_branch(repo_name, branch) end - def main_sha - client.list_commits(repo_name).last.sha - end - def branch - pr[:head][:ref] + pr.head.ref end def base_branch - pr[:base][:ref] + pr.base.ref end def pr_number event.dig('issue', 'number') end - def pr_status - checks = head_commit_checks[:check_suites] - - if checks.any? { |check| check[:status] != 'completed' } - 'pending' - elsif checks.any? { |check| check[:conclusion] != 'success' } - 'failure' - else - 'success' - end - end - - def head_commit_checks - client.check_suites_for_ref(repo_name, head_commit_sha) + def branch_head_sha + client.branch(repo_name, branch).commit.sha end - def head_commit_sha - client.pull_request_commits(repo_name, pr_number).last.sha + def pull_request_head_sha + client.pull_request(repo_name, pr_number).head.sha end def comment_author diff --git a/lib/squiddy.rb b/lib/squiddy.rb deleted file mode 100644 index 6c7b62a..0000000 --- a/lib/squiddy.rb +++ /dev/null @@ -1,91 +0,0 @@ -require 'trello' - -require_relative 'squiddy/event' -require_relative 'squiddy/pull_request' -require_relative 'squiddy/trello' - -module Squiddy - # TrelloPullRequest automatically creates a checklist on a Trello card called - # "Pull Requests", and adds the pull request URL as an item. - # - # If the Pull Request is in a closed state, then it marks the item as - # complete - class TrelloPullRequest - def self.run(pull_request_number:) - fail "pull_request_number is nil" if pull_request_number.nil? - - event = Squiddy::Event.new - - return nil unless event.type == "pull_request" - pull_request = Squiddy::PullRequest.new(event.repository, pull_request_number) - - trello_regex = /https:\/\/trello\.com\/c\/\w+/ - - trello_card = pull_request.body_regex(trello_regex) - return nil unless trello_card - - begin - trello = Squiddy::Trello::Checklist.new(trello_card) - - item = pull_request.url - - if pull_request.open? - puts "#{item} has been opened or edited" - - unless trello.checklist_exist? - trello.create_checklist - puts "Checklist created" - end - - unless trello.item_exist?(item) - trello.add_item(item) - puts "Added #{item} to checklist" - end - end - - if pull_request.closed? - puts "#{item} has been merged or closed" - - if trello.item_exist?(item) - trello.mark_item_as_complete(item) - puts "#{item} marked as complete" - end - end - rescue Squiddy::Trello::Checklist::ChecklistNotFound => e - e.message - end - end - end - - # TrelloDependabot will create a card on a specific board and list for every - # pull request which has a specific label that is configured in dependabot, - # with the body and link to the PR - class TrelloDependabot - def self.run(board_id:,list_create_id:,list_done_id:,github_label:,pull_request_number:) - fail "pull_request_number is nil" if pull_request_number.nil? - - event = Squiddy::Event.new - return nil unless event.type == "pull_request" - - pull_request = Squiddy::PullRequest.new(event.repository, pull_request_number) - return nil unless pull_request.labels.include?(github_label) - - board = ::Trello::Board.find(board_id) - card = board.cards.find { |card| card.name == pull_request.title } - - if pull_request.open? - return nil if card - - dependabot_label = board.labels.find {|label| label.name == "Dependabot" } || ::Trello::Label.create(board_id: board_id, name: "Dependabot") - - ::Trello::Card.create(list_id: list_create_id, name: pull_request.title, desc: pull_request.url, card_labels: [dependabot_label.id], pos: "bottom") - end - - if pull_request.closed? - return nil unless card - - card.move_to_list(list_done_id) - end - end - end -end diff --git a/spec/actions/bubble_merge/git_client_spec.rb b/spec/actions/bubble_merge/git_client_spec.rb index bcbc89c..f31767c 100644 --- a/spec/actions/bubble_merge/git_client_spec.rb +++ b/spec/actions/bubble_merge/git_client_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' +require 'hashie/mash' require_relative '../../../actions/bubble_merge/git_client' RSpec.describe Squiddy::GitClient do @@ -20,33 +21,47 @@ } }.to_json } - let(:pr_data) { { base: { ref: 'test-base-branch' }, head: { ref: 'test-branch' } } } - let(:octokit_client) { instance_double('Octokit::Client', merge: nil, delete_branch: nil, add_comment: nil) } - let(:head_commit_status) do - { - total_count: '1', - check_suites: [ - { - id: '1', - status: 'completed', - conclusion: 'success' - } - ] - } - end + let(:pull_request) { + Hashie::Mash.new({ + base: { ref: 'test-base-branch' }, + head: { ref: 'test-branch', sha: '1234' } + }) + } + let(:branch) { + Hashie::Mash.new({ + commit: { sha: '1234' }, + }) + } + let(:pull_merged?) { false } + let(:octokit_client) { + instance_double( + 'Octokit::Client', + merge_pull_request: nil, + delete_branch: nil, + add_comment: nil, + pull_merged?: pull_merged? + ) + } let(:repository) { double("Repository", delete_branch_on_merge: false) } before do stub_const('ENV', 'GITHUB_EVENT' => event, 'GITHUB_TOKEN' => 'token') allow(Octokit::Client).to receive(:new).and_return(octokit_client) - allow(octokit_client).to receive(:pull_request).with('test-user/test-repo', 'test-pr-number').and_return(pr_data) - allow(octokit_client).to receive_message_chain(:list_commits, :last, :sha).and_return('1234') - allow(octokit_client).to receive(:pull_merged?).and_return(false) - allow(octokit_client).to receive_message_chain(:pull_request_commits, :last, :sha).and_return('5678') - allow(octokit_client).to receive(:check_suites_for_ref).and_return(head_commit_status) + allow(octokit_client).to receive(:pull_request).with('test-user/test-repo', 'test-pr-number').and_return(pull_request) + allow(octokit_client).to receive(:branch).with('test-user/test-repo', 'test-branch').and_return(branch) allow(octokit_client).to receive(:repository).with('test-user/test-repo').and_return(repository) end + around do |example| + begin + old_stdout = $stdout + $stdout = StringIO.new(String.new, 'w') + example.call + ensure + $stdout = old_stdout + end + end + context '#bubble_merge' do let(:commit_message) { <<~MESSAGE @@ -64,13 +79,12 @@ } it 'merges successfully' do - expect(octokit_client).to receive(:merge).with( + expect(octokit_client).to receive(:merge_pull_request).with( 'test-user/test-repo', - 'test-base-branch', - 'test-branch', + 'test-pr-number', + commit_message, { - commit_message: commit_message, - merge_method: 'rebase', + merge_method: 'merge', sha: '1234' } ) @@ -93,7 +107,7 @@ } before do - allow(octokit_client).to receive(:merge).and_raise(Octokit::Conflict.new) + allow(octokit_client).to receive(:merge_pull_request).and_raise(Octokit::Conflict.new) end it 'prints an error message' do @@ -113,7 +127,7 @@ end it 'does not attempt to merge again' do - expect(octokit_client).not_to receive(:merge) + expect(octokit_client).not_to receive(:merge_pull_request) subject.bubble_merge end @@ -123,6 +137,42 @@ end end + context 'when the PR branch and PR are out of sync' do + let(:pull_request_head) { double(:pull_request_head, ref: 'test-branch') } + let(:pull_request) { + Hashie::Mash.new({ + base: { ref: 'test-base-branch' }, + head: pull_request_head, + }) + } + let(:branch) { + Hashie::Mash.new({ + commit: { sha: '1234' }, + }) + } + + before do + allow(subject).to receive(:sleep) + end + + it 'waits 5 times and then fails' do + allow(pull_request_head).to receive(:sha).and_return('5678') + + expect { + subject.bubble_merge + }.to raise_error(/PR HEAD 5678 did not match test-branch HEAD 1234/) + expect(subject).to have_received(:sleep).exactly(5).times + end + + it 'recovers if the PR updates to match the branch' do + allow(pull_request_head).to receive(:sha).and_return('5678', '1234') + + subject.bubble_merge + + expect(subject).to have_received(:sleep).once + end + end + context 'when the repository auto deletes branches on merge' do let(:repository) { double(delete_branch_on_merge: true) }