-
Notifications
You must be signed in to change notification settings - Fork 652
Added support for remote repositories (with and without authentication) #101
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
Conversation
Looks good, @nulltoken can you do a review as well and pull in if ok? |
|
||
public string Url { get; private set; } | ||
|
||
public string BranchName { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this property leveraged by the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, you are right. Let me check...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, good catch!
… remote repository
@@ -43,12 +43,12 @@ public static string GetSuffix(this Branch branch, BranchType branchType) | |||
|
|||
public static bool IsDevelop(this Branch branch) | |||
{ | |||
return branch.Name == "develop"; | |||
return branch.Name.EndsWith("develop"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed it was in my previous pull request as well. It will always work on remote/develop as well. GitVersion is downloading the right branch anyway now, but this will always work (where == won't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that this code won't protect us from branches that are named "mydevelop" or "origin/wrong/develop"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, since it is working with local branches now anyway, I will revert this file.
// Normalize (download branches) before using the branch | ||
GitHelper.NormalizeGitDirectory(gitDirectory); | ||
|
||
using (var repository = new Repository(gitDirectory)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just check the branch out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have an easier way, yes. But I couldn't find it (clone doesn't support default branch to check out). I don't want to download all the files though (since this might be a lot of data).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the repository is already fully downloaded, I think that something like this should work (Warning: untested code)
var branchName = "..."
var b = repo.FindBranch(branchName);
repo.Checkout(b, CheckoutModifiers.Force);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code works, but is much slower. Just let me know what you prefer, I don't mind what is being used in the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code works, but is much slower
Hmmm. You're right. We don't actually need to check the files out.
How about
var branchName = "..."
var b = repo.FindBranch(branchName);
Debug.Assert(b != null);
repo.Refs.UpdateTarget("HEAD", b.CanonicalName);
Logger.WriteInfo(string.Format("Moving HEAD to branch '{0}'", b.CanonicalName));
That should work 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed (with other options commented out). This works great and is slightly simpler than what we had before.
Logger.WriteInfo(string.Format("Switching to branch '{0}'", BranchName)); | ||
|
||
var branch = repository.FindBranch(BranchName); | ||
if ((branch != null) && !branch.IsCurrentRepositoryHead) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line (and the one above) can be safely dropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
So the pull request is now being accepted? |
@@ -34,6 +34,7 @@ public static void NormalizeGitDirectory(string gitDirectory) | |||
|
|||
static FetchOptions BuildFetchOptions() | |||
{ | |||
// TODO: Respect username/password of arguments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
@andreasohlund What's your take about this? How should we behave when both arguments and environment variables are valued? Which one should we consider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say commandline first, then fallback to env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Commandline should override general settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say commandline first, then fallback to env?
I agree. Commandline should override general settings.
@GeertvanHorrik Makes sense. Could you please update the PR to cope with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working on it. I will pass the arguments around everywhere then. Normally I would use an IoC container to inject the arguments automatically, but since this app is so simple, I will just pass it into the ctors manually.
@andreasohlund Should we create an issue to make this feature also available to the Task? |
Please do |
…password are first read from the environment variables, but can be overridden by command line arguments
…er is found so the build server can also pick up the version without using it as msbuild task
@@ -36,12 +40,18 @@ static void Main() | |||
|
|||
var versionAndBranch = VersionCache.GetVersion(gitDirectory); | |||
|
|||
foreach (var buildServer in applicableBuildServers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be opt in.
This change will break shouldly, because I parse the json output directly to use in my build scripts. I added a comment to #106 to discuss this particular issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are saying you use TeamCity but don't want to use the TeamCity integration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah,
The build scripts use the outputted version, patch assembly info and write the build number to teamcity themselves. This way I can run a complete build locally without teamcity
…type (json or buildserver) (-) Removed logging to console
I just merged with the latest upstream master. I think this pull request is ready and can be accepted. |
Added support for remote repositories (with and without authentication)
Just wondering if this feature makes it possible to run team city builds without having to set "checkout on agent" ? Or am I getting the wrong end of the stick? By default, Team city is set to Automatically check out on the Server, and historically unless you change it to "agent" side checkouts, then there is no git directory. Does this solve this issue? |
Yes, this feature solves that issue. |
Wowser that's brilliant. If I can get this working, I will happily create the above documentation. At the moment, there appears to be a bug as per the issue below. |
I have tried to configure this within Team City, and here are my issues:
It looks like, (from the stack trace in the above gist) that when the constructor for Any ideas why repositoryDirectory would be NULL? Is it misuse on my part, or is this a legitimate defect? I'd love to get this working. |
@dazinator GitVersion relies on LibGit2Sharp which doesn't support SSH yet. Feel free to subscribe to libgit2/libgit2sharp#852 in order to gets notified about progress.
Regarding the log above, it looks like only passing in the username causes an NullReferenceException which is... Hmmm... unfortunate. The error may originate from here. Either we should change this way of blindly building the callback or we should ensure that we do not accept partial credentials as parameters (eg. only the username is this case). @SimonCropp @JakeGinnivan Thoughts? |
Oh gees, was I only passing username. I thought I was passing username and password.. Dear lord. I will try it again. this time I'll try and pass in the actual arguments that I need to! :) |
@nulltoken - Sorry I am actually passing both username and password and I get the same exception. Here is a new log:
|
I will try and debug this locally for a bit see if I can work out what's going on. |
@dazinator Sorry, I was unclear. The exception above definitely looks like another issue. The log I was referring to (and which is pasted in my comment) is extracted from the initial mail I received when your first issued your comment (which you may have edited meanwhile). |
@nulltoken ah yes - I did edit things - probably my bad - no worries. |
Also, I went to create a regression test to replicate this problem, but it hit me that I have absolutely no idea how to do such a thing. I guess I am just not familiar enough with GitVersion at this point. So at the moment, to replicate this problem I simply put some command line arguments in the visual studio project configuration, and then ran the GitVersion.exe project. |
When no local .git folder is available, it is now possible to execute the logic on remote branches:
gitversion -url [repositoryurl]
to use authentication:
gitversion -url [repositoryurl] -u [username] -p [password]