search

reenhanced / gitreflow

Reflow automatically creates pull requests, ensures the code review is approved, and squash merges finished branches to master with a great commit message template.
MIT License
1.49k stars 64 forks source link

Bug Fix for Merging "reenhanced:master" instead of "master" #171

Closed simonzhu24 closed 8 years ago

simonzhu24 commented 8 years ago

@codenamev Apologies for sending another diff, but I found a bug last minute when trying to deliver using branch_name reenhanced:master instead of master. It wasn't a blocking issue on deliver, but it was using the wrong base_branch_name (reenhanced:master instead of master) to update and cleanup.

I am also adding self back into the code as a Ruby insurance that the correct scope of the variable is called so we don't have any confusion in the future. Please accept this asap so the bug can be fixed.

codenamev commented 8 years ago

Can you update the tests please? I have another branch (vs-cleanup-new-gh-merge) I've been working on to clean up some other issues I've found as well before we release.

simonzhu24 commented 8 years ago

The tests are passing as I didn't add any additional methods or complex logic that would cause them to fail.

There was no added functionality so I don't believe new tests need to be added?

On Friday, May 13, 2016, Valentino notifications@github.com wrote:

Can you update the tests please? I have another branch ( vs-cleanup-new-gh-merge) I've been working on to clean up some other issues I've found as well before we release.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/reenhanced/gitreflow/pull/171#issuecomment-219013891

simonzhu24 commented 8 years ago

@codenamev do you mind patching in my changes and seeing if the issues that you saw still exist? I believe this patch will solve the clean up and deliver issues that were present.

codenamev commented 8 years ago

The tests are there for quality assurance that the methods do what they say. Since we have restructured so much here, it's possible that the old tests didn't cover the new way of doing things. We'll want to first add failing tests to catch the issue you found so that we can ensure any new changes going forward don't affect it as well.

Also, we have a running PR open doing some cleanup before we can release. It turns out that the squash feature in the API is only in preview and requires special headers to access. While we can simple add those headers in, the github_api gem does not currently support the extra parameters (it filters unwanted options out). There is a PR we have open to get that resolved and will need it released in the github_api gem before we can release this as well.

simonzhu24 commented 8 years ago

@codenamev I updated the tests.

codenamev commented 8 years ago

It looks like the bug in question is using a new syntax for the branch name owner:base_branch_name? Can you please update/add tests for that scenario?

simonzhu24 commented 8 years ago

@codenamev Perhaps I misunderstood you? The checkout and some cleanup commands that were ran after delivery were git checkout owner:base_branch_name which was incorrect as it should have been git checkout base_branch_name. Because some of those commands were left out of the test, I added them in and added some additional tests. Now the code works and the tests pass.

I just checked locally both with force delivering and normally, each with and without cleanup. All 4 test cases work locally and the test coverage reflects this as well.

Please let me know if we can merge this. This solves an immediate problem with cleanup / deploy.

simonzhu24 commented 8 years ago

The additional tests make sure that each command that was supposed to run executed on the correct branch_name. It should be master instead of codenamev:master since github does not recognize the latter syntax.

codenamev commented 8 years ago

lgtm, thank you :smile:

simonzhu24 commented 8 years ago

@codenamev np, lets merge this!