Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

github PR ends up "closed" instead of "merged" #149

Closed
dragonsinth opened this issue Mar 2, 2016 · 8 comments
Closed

github PR ends up "closed" instead of "merged" #149

dragonsinth opened this issue Mar 2, 2016 · 8 comments

Comments

@dragonsinth
Copy link

The way 'deliver' currently works, the original PR ends up marked Closed instead of Merged.

There is a flow that can fix this.

  1. Perform the merge-squash against master.
  2. Force-push the original branch to update the PR.
  3. Attempt to push master (if it fails, loop back to step 1)
  4. Delete the original branch.

I'm not sure if a small sleep is required before step 4; at some time in the past github had a race condition where the PR might get marked closed or merged if you delete the upstream immediately after pushing master, but they might have fixed this now.

@nhance
Copy link
Member

nhance commented Mar 2, 2016

I am normally against the idea of force pushing, but the idea of hiding force pushing inside of a tool like this actually makes a ton of sense. This seems like a good idea to me

@codenamev
Copy link
Collaborator

This is the second time this has come up (see #111). I think it would make sense to squash commits in the feature branch itself as well. As long as we're pulling in an update before force-pushing I don't see any real drawback.

@pboling
Copy link
Contributor

pboling commented Apr 7, 2016

Alternate solution is interactive rebase, which is very similar to a squash merge.

@codenamev
Copy link
Collaborator

Personally I like an interactive rebase better, but I'm concerned it might be too advanced of a tool? Maybe we'll make it an option and default to merge --squash? @nhance thoughts here?

@nhance
Copy link
Member

nhance commented Apr 7, 2016

Agreed, interactive rebase is beyond the scope of reflow. It's a great idea and I love using it, but I think it places too much knowledge demand on those who might be new to reflow and not used to using it.

@dragonsinth
Copy link
Author

It doesn't really matter how you end up getting there; if you want to end up "merged" you have to force-push the original branch after you setup the new master; you also have to change the final commit message from "Closes #4" to something like "Merges #4" that doesn't auto-trigger github. Here's what I've patched for myself locally:

--- git_reflow-0.7.2/lib/git_reflow.rb.was
+++ git_reflow-0.7.2/lib/git_reflow.rb
@@ -145,6 +145,8 @@

             if deploy_and_cleanup
               run_command_with_label "git push origin #{base_branch}"
+              run_command_with_label "git push -f origin #{base_branch}:#{feature_branch}"
+              sleep 5
               run_command_with_label "git push origin :#{feature_branch}"
               run_command_with_label "git branch -D #{feature_branch}"
               puts "Nice job buddy."
--- git_reflow-0.7.2/lib/git_reflow/git_helpers.rb.was
+++ git_reflow-0.7.2/lib/git_reflow/git_helpers.rb
@@ -48,7 +48,7 @@
       message = "#{options[:message]}"

       if "#{options[:pull_request_number]}".length > 0
-        message << "\nCloses ##{options[:pull_request_number]}\n"
+        message << "\nMerges ##{options[:pull_request_number]}\n"
       end

       if lgtm_authors = Array(options[:lgtm_authors]) and lgtm_authors.any?

simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 18, 2016
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 18, 2016
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 18, 2016
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 18, 2016
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 27, 2016
- Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 28, 2016
- Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 28, 2016
- Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue Apr 29, 2016
- Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
- Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
- Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 2, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 3, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 4, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 5, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 5, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 5, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
@codenamev
Copy link
Collaborator

simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 5, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 6, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 6, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 6, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 6, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 6, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 7, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 7, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 7, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 10, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 10, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 11, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 11, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 11, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 11, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
simonzhu24 added a commit to simonzhu24/gitreflow that referenced this issue May 11, 2016
…ed" - Calling Github API to Squash Merge the PR as opposed doing it manually
codenamev pushed a commit that referenced this issue May 11, 2016
…ng it manually

* Updates README with changes to deliver process for Github
* Adds MergeError to raise during merge for better error handling during deliver
* Adds DEFAULT_EDITOR to establish a global editor of choice
* Cleans up options hash accessor syntax to use symbols instead of strings
* Moves merging process into new PullRequest#merge! method
* Now outputs feature branch status before merging
* Removes option to deliver to a specific base branch
* Removes GitReflow::GitHelpers.merge_feature_branch in favor of new PullRequest#merge! method
* Adds opening of commit message to deliver process for Github (pre-merge)
* Adds PullRequest#commit_message_for_merge
* Adds PullRequest#cleanup_feature_branch?
* Adds PullRequest#deliver?
* Adds PullRequest#cleanup_failure_message?

Merges #170 
LGTM given by: @codenamev
@codenamev
Copy link
Collaborator

Fixed in v0.8.0 with 3b44510.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants