Skip to content

Commit

Permalink
[Issue reenhanced#149] Fix for PR ending as "Closed" instead of "Merged"
Browse files Browse the repository at this point in the history
- Calling Github API to Squash Merge the PR as opposed doing it manually
  • Loading branch information
simonzhu24 committed Apr 27, 2016
1 parent 5110d04 commit 36f50e4
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 21 deletions.
33 changes: 17 additions & 16 deletions lib/git_reflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,8 @@ def deliver(options = {})
feature_branch = current_branch
base_branch = options['base'] || 'master'

update_current_branch
fetch_destination(base_branch)
update_destination(feature_branch)

begin
existing_pull_request = git_server.find_open_pull_request( :from => current_branch, :to => base_branch )

if existing_pull_request.nil?
say "No pull request exists for #{remote_user}:#{current_branch}\nPlease submit your branch for review first with \`git reflow review\`", :deliver_halted
else
Expand All @@ -125,34 +120,40 @@ def deliver(options = {})
end

if existing_pull_request.good_to_merge?(force: options['skip_lgtm'])
puts "Merging pull request ##{existing_pull_request.number}: '#{existing_pull_request.title}', from '#{existing_pull_request.feature_branch_name}' into '#{existing_pull_request.base_branch_name}'"
puts "Merging pull request ##{existing_pull_request.number}: '#{existing_pull_request.title}', from '#{existing_pull_request.feature_branch_name}' into '#{GitReflow::Config.get("github.owner")}:#{base_branch}'"

update_destination(base_branch)
merge_feature_branch(feature_branch,
res = merge_feature_branch(feature_branch,
:destination_branch => base_branch,
:pull_request_number => existing_pull_request.number,
:lgtm_authors => existing_pull_request.approvals,
:message => commit_message)
committed = run_command_with_label 'git commit', with_system: true
:title => existing_pull_request.title,
:message => commit_message,
:head => existing_pull_request.head)

if committed
say "Merge complete!", :success
if res.code == "200"
# if committed
say "Pull Request successfully merged.", :success

# check if user always wants to push and cleanup, otherwise ask
always_deploy_and_cleanup = GitReflow::Config.get('reflow.always-deploy-and-cleanup') == "true"
deploy_and_cleanup = always_deploy_and_cleanup || (ask "Would you like to push this branch to your remote repo and cleanup your feature branch? ") =~ /^y/i

if deploy_and_cleanup
run_command_with_label "git push origin #{base_branch}"
run_command_with_label "git push origin :#{feature_branch}"

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

This line was to delete the remote feature branch. We still want to do this (not the line above it, just this line with git push origin :#{feature_branch}).

The : means delete the branch.

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 28, 2016

Author Owner

@pboling yup, thanks for catching this!

# Pulls merged changes from remote base_branch
run_command_with_label "git pull origin #{base_branch}"
run_command_with_label "git branch -D #{feature_branch}"
puts "Nice job buddy."
else
puts "Cleanup halted. Local changes were not pushed to remote repo.".colorize(:red)
puts "To reset and go back to your branch run \`git reset --hard origin/#{base_branch} && git checkout #{feature_branch}\`"
end

elsif res.code == "405"
say "Pull Request is not mergeable.", :deliver_halted
elsif res.code == "409"
say "Head branch was modified. Review and try the merge again.", :deliver_halted

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

Since review is a command of git reflow this may be confusing. Not sure if user is being told to use git reflow review or do something else.

We should probably suggest a solution. I think in both cases of 405 and 409 the first thing to try is git reflow refresh.

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 28, 2016

Author Owner

@pboling I wasn't sure which error messages to put here, so I just used the default error messages suggested by Github:

https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button

Let me think about trying git reflow refresh.

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

We should use the default error messages AND suggest a git reflow refresh I think.

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 29, 2016

Author Owner

Ok!

else
say "There were problems commiting your feature... please check the errors above and try again.", :error
say "There was another error. Please review the pull request and try again.", :deliver_halted
end
else
say existing_pull_request.rejection_message, :deliver_halted
Expand Down Expand Up @@ -185,7 +186,7 @@ def deploy(destination_server)
end

def git_server
@git_server ||= GitServer.connect provider: GitReflow::Config.get('reflow.git-server').strip, silent: true
@git_server ||= GitServer.connect provider: "#{GitReflow::Config.get('reflow.git-server')}".strip, silent: true
end

end
7 changes: 7 additions & 0 deletions lib/git_reflow/commands/setup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,12 @@
GitReflow::Config.add "constants.minimumApprovals", ask("Set the minimum number of approvals (leaving blank will require approval from all commenters): "), local: reflow_options[:project_only]
GitReflow::Config.add "constants.approvalRegex", GitReflow::GitServer::PullRequest::DEFAULT_APPROVAL_REGEX, local: reflow_options[:project_only]

say("Enter the follow information associated with your remote repository: (The information is used to run 'git reflow deliver'.")
GitReflow::Config.add "github.owner", ask("Enter the owner of the remote repository: "), local: reflow_options[:project_only]
GitReflow::Config.add "github.repo", ask("Enter the name of the remote repository: "), local: reflow_options[:project_only]

# Sets the user's github auth token
GitReflow::Config.set "github.oauth-token", ask("Set the your local github authorization token: (You cannot run 'git reflow deliver' without it!) "), local: reflow_options[:project_only]
say("Thanks! Your settings are saved to #{GitReflow::Config::CONFIG_FILE_PATH}.")
end
end
38 changes: 36 additions & 2 deletions lib/git_reflow/git_helpers.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
require 'git_reflow/config'
require 'git_reflow/sandbox'
require 'net/http'
require 'uri'
require 'json'

module GitReflow
module GitHelpers
Expand Down Expand Up @@ -60,10 +63,41 @@ def merge_feature_branch(feature_branch_name, options = {})
message << "\nLGTM given by: @#{lgtm_authors.join(', @')}\n"
end

run_command_with_label "git checkout #{options[:destination_branch]}"
run_command_with_label "git merge --squash #{feature_branch_name}"
payload = {
"commit_title" => options[:title],
"commit_message" => options[:message],
"sha" => options[:head].sha,
"squash" => true
}

res = post_github_api(
{
:payload => payload,
:pull_request_number => options[:pull_request_number]
}
)

append_to_squashed_commit_message(message) if message.length > 0
return res
end

def post_github_api(options = {})
url = URI.parse("#{GitReflow::Config.get('github.endpoint')}/repos/#{GitReflow::Config.get('github.owner')}/#{GitReflow::Config.get('github.repo')}/pulls/#{options[:pull_request_number]}/merge")

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

This string is sufficiently complex to be a separate method that just returns the string.


req = Net::HTTP::Put.new(url.to_s)
req.body = options[:payload].to_json

# Sets Request Headers
req["Accept"] = "application/vnd.github.polaris-preview"
req["Content-Type"] = "application/json"
req["Authorization"] = "token #{GitReflow::Config.get('github.oauth-token')}"

Net::HTTP.start(url.host, url.port,
:use_ssl => url.scheme == 'https') do |http|
res = http.request req # Net::HTTPResponse object
puts res.inspect

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

This was probably for debugging?

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 28, 2016

Author Owner

Yes, I thought it was useful to leave in (just to tell the users, who are most likely developers) what is going on!

return res
end
end

def append_to_squashed_commit_message(message = '')
Expand Down
7 changes: 6 additions & 1 deletion spec/git_reflow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@

before do
allow(GitReflow).to receive(:append_to_squashed_commit_message).and_return(true)
allow(GitReflow).to receive(:post_github_api).and_return({})

module Kernel
def system(cmd)
Expand Down Expand Up @@ -310,6 +311,7 @@ def system(cmd)
allow(github).to receive(:find_open_pull_request).and_return(existing_pull_request)
allow(GitReflow).to receive(:get_first_commit_message).and_return(first_commit_message)
allow(existing_pull_request).to receive(:reviewers).and_return(lgtm_comment_authors)
allow(GitReflow).to receive(:post_github_api).and_return({})
end

it "includes the first commit message for the new branch in the commit message of the merge" do
Expand All @@ -324,7 +326,9 @@ def system(cmd)
destination_branch: 'master',
pull_request_number: existing_pull_request.number,
lgtm_authors: ['nhance'],
message: existing_pull_request.body
title: existing_pull_request.title,

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

Follow same spacing, with aligned values.

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 28, 2016

Author Owner

Sure

message: existing_pull_request.body,
head: existing_pull_request.head
})

expect { subject }.to have_output "Merging pull request ##{existing_pull_request.number}: '#{existing_pull_request.title}', from '#{existing_pull_request.head.label}' into '#{existing_pull_request.base.label}'"
Expand Down Expand Up @@ -470,6 +474,7 @@ def system(cmd)
end

it "merges and squashes the feature branch into the master branch" do
allow(GitReflow).to receive(:post_github_api).and_return({})
expect(GitReflow).to receive(:merge_feature_branch)
subject
end
Expand Down
9 changes: 7 additions & 2 deletions spec/lgtm_git_reflow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@

before do
allow(GitReflow).to receive(:append_to_squashed_commit_message).and_return(true)
allow(GitReflow).to receive(:post_github_api).and_return({})

module Kernel
def system(cmd)
Expand Down Expand Up @@ -163,7 +164,9 @@ def system(cmd)
destination_branch: 'master',
pull_request_number: existing_pull_request.number,
lgtm_authors: ['nhance', 'Simon'],
message: existing_pull_request.body
title: existing_pull_request.title,

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

Also here.

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 28, 2016

Author Owner

Sure

message: existing_pull_request.body,
head: existing_pull_request.head
})

expect { subject }.to_not have_output "Merging pull request ##{existing_pull_request.number}: '#{existing_pull_request.title}', from '#{existing_pull_request.head.label}' into '#{existing_pull_request.base.label}'"
Expand Down Expand Up @@ -335,7 +338,9 @@ def system(cmd)
destination_branch: 'master',
pull_request_number: existing_pull_request.number,
lgtm_authors: ['nhance', 'Simon'],
message: existing_pull_request.body
title: existing_pull_request.title,

This comment has been minimized.

Copy link
@pboling

pboling Apr 28, 2016

And here.

This comment has been minimized.

Copy link
@simonzhu24

simonzhu24 Apr 28, 2016

Author Owner

Sure

message: existing_pull_request.body,
head: existing_pull_request.head
})

expect { subject }.to have_output "Merging pull request ##{existing_pull_request.number}: '#{existing_pull_request.title}', from '#{existing_pull_request.head.label}' into '#{existing_pull_request.base.label}'"
Expand Down

5 comments on commit 36f50e4

@simonzhu24
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things completed:

  1. Prompted Users for:
  2. Auth Token
  3. Owner Name
  4. Repo Name
  5. Remove Manual Merge Process
  6. Make HTTP Request to Github to squash merge PR
  7. Exception / Error Handling for HTTP Request

@codenamev
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! One thing I notice is that we've lost bit bucket support with this. The merge_feature_branch should move into a new PullRequest#merge! method.

From a higher level, it would be nice to see something like this:

begin
  existing_pull_request.merge!
rescue GitReflow::PullRequest::MergeFailure => e
  say e.message, :deliver_halted
end

Also, the github_api gem already provides a way to merge ;-)

@simonzhu24
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codenamev I didn't know about that gem before! Definitely, lets do that instead.

@simonzhu24
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note quite sure if we need something like this?

begin
  existing_pull_request.merge!
rescue GitReflow::PullRequest::MergeFailure => e
  say e.message, :deliver_halted
end

The standard if-else or case would suffice...

@pboling
Copy link

@pboling pboling commented on 36f50e4 Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simonzhu24 I think we need the rescue because on the failure to merge, if you go with the github_api gem, then the error will be raised, and an if-else can't handle that:
Error: GitReflow::PullRequest::MergeFailure

Please sign in to comment.