Improved support for dynamic repositories #269
Conversation
|
I'm unsure about something: what is a dynamic repository? Could you please add tests to cover those changes? |
|
Dynamic repositories are already part of the software. I will write unit tests, but too busy now. Will take at least a week. |
|
Dynamic repositories are repositories where the local .git is not available and the information has to be fetched from the git repository url. |
| { | ||
| Logger.WriteInfo(string.Format("Switching to branch '{0}'", arguments.TargetBranch)); |
There was a problem hiding this comment.
Added it back
Now I remember. You already explained me this once. Maybe that would deserver some docco/commnet in the code. What kind of CI build require this? |
|
|
Any server that doesn't provide the full repository (like Continua CI, that only provides the actual files needed to build). I might write some docs once. |
| // Github Message: refs/heads/pull/5/merge | ||
| // Stash Message: refs/heads/pull-requests/5/merge | ||
|
|
||
| var regex = new Regex(MergeMessageRegexPattern); | ||
| var match = regex.Match(mergeMessage); | ||
| // Note by @GeertvanHorrik: sorry, I suck at regex so did a quick hack, feel free to replace by regex |
There was a problem hiding this comment.
For the MergeMessageRegexPattern you should be able to use something like this
Sure. @nulltoken I don't see anything that would immediately clash, but I am not entirely clear on exactly what is being done here, or why the need to do this for ContinuaCI. @GeertvanHorrik when I added the Stash Pull Request verification, I added an additional TestCase here: Which was suggested by @JakeGinnivan, so I think you could at least add another TestCase here for the additional PR message that is now being tested for. I have also added a suggestion for the Regex parsing, rather than what you have implemented currently. |
Given that this is the case, and the presence of the .git directory is the trigger for anything else happening, then I don't think #262 should be affected, as the .git directory is present in AppVeyor. |
|
I'm starting to wonder if we shouldn't rather provide GitVersion with some information about the context regarding the hoster (Stash, GitHub, ..) and the CI platform (AppVeyor, Continua, TeamCity, ...) rather than letting it guessing... |
|
@nulltoken how would you do that? |
|
I think we should try and make it 'just work'. I dont want to have to pass more info. Also, passing context means it no longer is portable and you can't build in different contexts (like CI server and local). We do need more scenario based tests though to cover these things |
|
Isn't that out of scope for this issue? |
|
@GeertvanHorrik could you add a test or two around this functionality. There are quite a few nice changes in this so I have no problem with the PR itself, just want to our test coverage to move forward not backwards :) |
|
I understand, just need to find time. Was a bit overwhelmed with all other things I had to finish. It's still on my todo list, just need to find 30 minutes somewhere ;-) |
|
I want to get 2.0.0 released this weekend. Recon you could find 30 mins to throw some tests around this over next day or two? Would be good to have this in. I am happy to rebase if you just add the tests in :) |
|
I forgot, my mistake. So many PRs going on atm. I will do it now. |
|
I know the feeling, I just don't know much about this feature otherwise I would have just done it myself to get it in :) |
|
Disabling pepita (it is giving me build errors for now). If I forget to re-enable, forgive me. ps. dumping status info here, then I won't forget to put some stuff back later. |
|
Written unit tests, can you take a quick look if that is sufficient? I also wrote a test for local repositories to prove that works as well. I have implemented the unit tests as explicit, but you can change that if you like. |
|
It would be good to create a local remote repository so we don't have online requirements then it doesnt have to be explicit. But I can change that |
|
"online requirements". I think you need to get the latest code anyway, but yes, in theory you could abstract it away into a mock / local repository. |
|
It needs to be rebased and the test needs to be made to work offline (i.e create a fake repo then clone it). if someone gets to that before me, great. Otherwise I hope to do it in the next few days |
New PR for #245 which was buggy