Skip to content

Fix for #232: GitHubVersion for pull requests are not working with dynamic repositories #245

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 15 commits into from
Closed

Fix for #232: GitHubVersion for pull requests are not working with dynamic repositories #245

wants to merge 15 commits into from

Conversation

GeertvanHorrik
Copy link
Contributor

No description provided.

@JakeGinnivan
Copy link
Contributor

Could you rebase to remove merge commits? :)

Sent from my Windows Phone

From: Geert van Horrikmailto:[email protected]
Sent: ý16/ý08/ý2014 17:16
To: Particular/GitVersionmailto:[email protected]
Subject: [GitVersion] Fix for #232: GitHubVersion for pull requests are not working with dynamic repositories (#245)

You can merge this Pull Request by running

git pull https://github.com/GeertvanHorrik/GitVersion master

Or view, comment on, or merge it at:

#245

Commit Summary

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/245.

@GeertvanHorrik
Copy link
Contributor Author

Not sure if I can do that after commits. Is it a real requirement?

@GeertvanHorrik
Copy link
Contributor Author

Don't accept it please. I found an issue. Will fix on monday.

@JakeGinnivan
Copy link
Contributor

If you rebase the merge commits just magically disappear :) git is clever

It keeps history cleaner, so rebase before merging is preferred.

Sent from my Windows Phone

From: Geert van Horrikmailto:[email protected]
Sent: ý16/ý08/ý2014 19:06
To: Particular/GitVersionmailto:[email protected]
Cc: Jake Ginnivanmailto:[email protected]
Subject: Re: [GitVersion] Fix for #232: GitHubVersion for pull requests are not working with dynamic repositories (#245)

Not sure if I can do that after commits. Is it a real requirement?

Sent from my phone, so message is short

From: Jake Ginnivanmailto:[email protected]
Sent: ý16-ý8-ý2014 17:56
To: Particular/GitVersionmailto:[email protected]
Cc: Geert van Horrikmailto:[email protected]
Subject: Re: [GitVersion] Fix for #232: GitHubVersion for pull requests are not working with dynamic repositories (#245)

Could you rebase to remove merge commits? :)

Sent from my Windows Phone

From: Geert van Horrikmailto:[email protected]
Sent: ?16/?08/?2014 17:16
To: Particular/GitVersionmailto:[email protected]
Subject: [GitVersion] Fix for #232: GitHubVersion for pull requests are not working with dynamic repositories (#245)

You can merge this Pull Request by running

git pull https://github.com/GeertvanHorrik/GitVersion master

Or view, comment on, or merge it at:

#245

Commit Summary

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/245.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/245#issuecomment-52396900.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/245#issuecomment-52399768.

@GeertvanHorrik
Copy link
Contributor Author

Pushed a fix and rebased. Hopefully haven't broken anything with the rebase

@JakeGinnivan
Copy link
Contributor

Looks like the rebase didn't work and there are a few failing tests:

I also noticed you are commiting to your forks master, generally I recommend never commit to master then you will be in a clean state to take another branch.

// These commands assume you have origin as your fork, and upstream as the Particular/GitVersion repo

// Take a feature branch
git checkout -b PullRequestsAndDynamicRepositories
git commit -am "Doing stuff"
...

// After you have done the work and you want to bring your branch up to date
git fetch upstream
git rebase upstream/master
git push origin PullRequestsAndDynamicRepositories -f

This will keep your history nice and clean and master will be free to branch at any time.

If you have any issues I can rebase, but that won't fix the failing tests.

@GeertvanHorrik
Copy link
Contributor Author

Thanks for the info. Let me take a look. It was something with uppercase/lowercase, I thought it was a "known" issue.

JakeGinnivan
JakeGinnivan reviewed Aug 19, 2014
View reviewed changes
GitVersionCore/Extensions/ExtensionMethods.git.cs
if (branchName.IsPullRequest())
{
branchName = branchName.Replace("pull-requests", "pull");
branchName = branchName.Replace("pull", "pull");
Copy link
Contributor

Choose a reason for hiding this comment

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

This line isn't needed

@GeertvanHorrik
Copy link
Contributor Author

Pull request updated.

@JakeGinnivan
Copy link
Contributor

There is also a code analysis error. GitPreparer has foreach loops which can be converted into Linq. R# warnings/suggestions are turned up to errors in the build

@GeertvanHorrik
Copy link
Contributor Author

Done.

@GeertvanHorrik
Copy link
Contributor Author

What is required for now to get this accepted? All unit tests succeed on my machine.

@GeertvanHorrik
Copy link
Contributor Author

What's the status of this PR?

@SimonCropp
Copy link
Contributor

build server says it is still failing

Test(s) failed. Shouldly.ChuckedAWobbly : 
                fixture.AssertFullSemver("2.0.0-alpha.1+5");
        should be
    "2.0.0-alpha.1+5"
        but was
    "1.0.4+5"
   at Shouldly.ShouldlyCoreExtensions.AssertAwesomely[T](T actual, Func`2 specifiedConstraint, Object originalActual, Object originalExpected) in c:\TeamCity\buildAgent\work\10efaabfa8adbd4e\src\Shouldly\ShouldlyCoreExtensions.cs:line 18
   at Shouldly.ShouldBeTestExtensions.ShouldBe[T](T actual, T expected) in c:\TeamCity\buildAgent\work\10efaabfa8adbd4e\src\Shouldly\ShouldBeTestExtensions.cs:line 16
   at OtherBranchTests.CanTakeVersionFromReleaseBranch() in c:\BuildAgent\work\5e0d79a4e3943b17\GitVersionCore.Tests\GitHubFlow\OtherBranchTests.cs:line 18
------- Stderr: -------
Created git repository at 'C:\BuildAgent\work\5e0d79a4e3943b17\GitVersionCore.Tests\bin\Release\TestRepositories\d7d34827-5ad7-48a9-b619-35a305eca233'

@GeertvanHorrik
Copy link
Contributor Author

Working on it now.

@GeertvanHorrik
Copy link
Contributor Author

Found the issue. A pull upstream master went terribly wrong. It should be fixed now.

@GeertvanHorrik
Copy link
Contributor Author

This is frustrating, now it thinks I changed 87 files. Is there a way for you to rebase so this won't screw up any history? Otherwise I will create a new PR

@nulltoken
Copy link
Contributor

Is there a way for you to rebase so this won't screw up any history? Otherwise I will create a new PR

As we don't have push rights in your own fork, we can't rebase it for you.

If you end up creating a new PR, I would advise you to not send it from your master, but rather to create a specific topic branch to hold your proposed changes.

@GeertvanHorrik
Copy link
Contributor Author

ok, will create new pr then.

@GeertvanHorrik GeertvanHorrik mentioned this pull request Oct 5, 2014
Closed
@GeertvanHorrik
Copy link
Contributor Author

New PR created, this is corrupt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@GeertvanHorrik @JakeGinnivan @SimonCropp @nulltoken