Correcting AppVeyor Support for Pull Requests #262
Conversation
- As per discussion with @nulltoken, implemented a fix for AppVeyor where if the commit sha is not present of any detected local branches, check out a new branch using the sha as the pointer
- As per discussion with @nulltoken, implemented a fix for AppVeyor where if the commit sha is not present of any detected local branches, check out a new branch using the sha as the pointer
|
I have just checked, and this will also resolve Issue #254 |
|
Ok, this isn't going to work as is. Need to re-think: Failed Build here: Am I safe to just "pick" either branch to checkout? Since they are both at the same head sha, it shouldn't really matter right? |
|
Also, this failed build would suggest that we are missing at least one Unit Test to cover this circumstance. Sorry but I wouldn't know where to begin in creating a unit test to mockup this situation. Any ideas? |
- As per suggestion here: GitTools#260 (comment)
We could solve this by favoring local branches over remote tracking ones (see inline comment) |
| // In order to decide whether a fake branch is required or not, first check to see if any local branches have the same commit SHA of the head SHA. | ||
| // If they do, go ahead and checkout that branch | ||
| // If no, go ahead and check out a new branch, using the known commit SHA as the pointer | ||
| var localBranchesWhereCommitShaIsHead = repo.Branches.Where(b => b.Tip.Sha == headSha).ToList(); |
There was a problem hiding this comment.
.Where(b => !b.IsRemote && b.Tip.Sha == headSha).ToList();
There was a problem hiding this comment.
Ooo clever! I like it! Will get this included! :-)
|
I am away from computer now, but I will try and look at this tomorrow morning. After that I am away on holiday for a week, so might be a delay if I don't get to it. Thanks! |
- Updated with suggestion from @nulltoken - GitTools#262 (comment)
|
@nulltoken I have just implemented your suggestion and run a test on both a Pull Request (from remote repo) and then merging that Pull Request in, both succeeded successfully: https://ci.appveyor.com/project/GaryEwanPark/appveyortest/build/0.2.0-PullRequest.7+2%20(Build%2033) I "think" we are good to go here. Do you agree? |
|
👍 for me. However I'd also like @distantcam to take a peek at this as he originally added the AppVeyor support. |
|
Sounds good to me. Sent from my Windows Phone From: nulltokenmailto:notifications@github.com 👍 for me. However I'd also like @distantcam to take a peek at this as he originally added the AppVeyor support. Reply to this email directly or view it on GitHub: |
|
@distantcam are you ok with this work? Are we good to pull it in? Thanks! /cc @nulltoken |
|
Looks good to me. The fact that APPVEYOR_REPO_BRANCH had the wrong branch name smells like a bug to me but that's an AppVeyor problem. Ping @FeodorFitsner |
|
The idea was setting |
|
@FeodorFitsner Maybe just update the doco. Currently it just says "APPVEYOR_REPO_BRANCH - build branch" which I would take to mean the PR branch, not the branch the PR is merging into. |
Correcting AppVeyor Support for Pull Requests
|
This is great thanks! :-) Any thoughts on when this might be released? /cc @SimonCropp Sent from my Windows Phone From: nulltokenmailto:notifications@github.com Merged #262. Reply to this email directly or view it on GitHub: |
|
Will deploy on the weekend |
|
Thank you! Much appreciated! :-) Sent from my Windows Phone From: Simon Croppmailto:notifications@github.com Will deploy on the weekend Reply to this email directly or view it on GitHub: |
In a recent Pull Request (GitTools#262), the decision was taken to, in the absence of setParameter being available in AppVeyor, to create environment variables that could then be accessible in the remainder of Build Script. I would like to suggest that this also makes sense for TeamCity. In my scenario, I want to use: ```/output buildserver``` but use the generated variables later in my psake build script. In order to do this, I have to call GitVersion twice. Using this approach should mean that I can access the created Environment Variables later in my psake script when I need them. Does that make sense?
|
@FeodorFitsner do you have a second to look at this: I have just tried to run the modified version of GitVersion which now should output the GitVersion Variables to an environment variable, and when I try to read them back in my psake script, I can't see them. Here is my psake script: https://github.com/gep13/ChocolateyGUI/blob/feature/Issue-172/BuildScripts/default.ps1#L239 Unless I am doing something wrong in PowerShell in terms of reading the value back, you can see that it isn't there. Is using |
|
I think the problem could be in the way you read them. Look at this: http://stackoverflow.com/questions/312314/environment-variables-in-powershell-with-a-dot-in-the-name I'd better replace dot |
|
@FeodorFitsner ok, let me try that, and I will report back... |
|
@FeodorFitsner doesn't look like the alternative syntax works either... This is how I am calling it: https://github.com/gep13/ChocolateyGUI/blob/feature/Issue-172/BuildScripts/default.ps1#L239 I am going to do one final test with _ to see if that helps, before signing off for the night. |
|
I think it should possibly be implemented as part of the App Veyor specific part of GitVersion. It shouldn't be generic since most services (e.g. TeamCity) will not work with this approach since the machine environment variables aren't isolated. |
So what would you suggest I do when I am running on TeamCity, using the same style of psake build script? I have tried the same thing in terms of reading the System Parameters that are set when using |
|
I think setting machine level env variables is a bad idea. And the issue with Maybe the best option is to use it as a library, load the exe as a .net library and invoke methods (you are using powershell after all). We have not thought much about the internal API, but that was the idea of the core library is that you could use it. Or use the |
But that is what I am having to do just now anyway, when calling GitVersion twice. I don't see an issue with have to do this.
I haven't tried this approach yet, but I can certainly look into it. |
|
... or we could add another method into build agent's REST API for collecting key-values and then injecting them into build session as environment variables before running the next command, so the approach looks similar to other build servers. |
Well, that would quite simply be amazing, at least from my point of view :-) |
One call is just returning json, if we did combined it would be @FeodorFitsner agreed, it is handy not just for our case. Collecting from stdout of the executed process would be even better :) |
@JakeGinnivan Exactly. So all the Build Server variables would be set, and the build number was set, and all the branches would be in the right place, meaning that additional build steps could make use of the variables, and then, immediately after executing the call to GitVersion, I could use the JSON in the remainder of my psake script. In my mind, the best of both worlds. My main aim here is to have psake, or whatever build script you want to use, be the "owner" of everything. I don't necessarily want to be tied to something in TeamCity, or AppVeyor, being set, that I can use in a subsequent build step. I want to make a single call to the build script, do the necessary work, and have a finalised output. I don't think this is a foolish approach. I think I should be able to immediately pick up the build script and start working either when running locally, or moving to a different CI engine. Right now, the fact that I can't access the Build Server variables is hampering to me. While I could use the approach to have GitVersion be the entry point, and use Will have to ponder this some more... |
|
@gep13 how would the buildscript differentiate the output for the build server and the json? It will just be one string with json and build server output in it. Doesn't seem like a good option to make people parse that. |
|
Ah I see what you are getting at. Hmm, on that I am not sure :-( |
|
We've just added a new build worker API method By calling this API method in your script (PowerShell or command-line) or utility (HTTP) you collect environment variables which will be set in parent process as soon as the current process ends to make them available to the rest of build commands. Regarding |
|
Wow, you guys are good! 👍 So let me see if I have this right... Do you see this method being called from within GitVersion, using the Thanks again for helping out with this! |
|
Yes, that's for calling from However, I think we could make it available immediately in the same PowerShell script. PS script is running in the same process as build worker and if we modify its environment variables they must be available immediately in any child process created. Let me try that. |
|
@FeodorFitsner that would be great if they could be :-) I will submit a pull request for changing the Can you provide an example of how to read the variable in a PowerShell script, assuming that it works. Thanks! |
- As per discussion here: GitTools#262 - Changed to use the AppVeyor REST API SetVariable method, instead of setting an Environment Variable
|
OK, that worked! I think we finally got what you're looking for. So, this is your build script, think PSake's default.ps1: https://github.com/FeodorFitsner/simple-console/blob/master/build.ps1 SimpleConsole.exe is kind of GitVersion.exe where you set build environment variables using AppVeyor API: https://github.com/FeodorFitsner/simple-console/blob/master/SimpleConsole/Program.cs Build results: https://ci.appveyor.com/project/appvyr/simple-console/build/9#L74 ...and of course, these variables will be available in any child process called anywhere after SimpleConsole.exe. Everything in production. |
|
Thanks for getting this done! I have just done a test, using what I think is the exact same approach, and unfortunately, it doesn't work :-( You can see the missing version number here: I have modified the GitVersion source code, to do this: and I have pushed the "hooky" version of 1.2.9 to my MyGet Feed, and you can see that this is what has been installed: And I am trying to read the environment variable here: gep13/ChocolateyGUI@18fe97a#diff-fb0e4ab501cf90c08df493e728f4ec66R267 Did I do something wrong? |
|
Check that GitVersion variables are using underscore. TeamCity converts the GitVersion.SemVer into GitVersion_SemVer from memory Sent from my Windows Phone From: Gary Ewan Parkmailto:notifications@github.com Thanks for getting this done! I have just done a test, using what I think is the exact same approach, and unfortunately, it doesn't work :-( You can see the missing version number here: I have modified the GitVersion source code, to do this: gep13/GitVersion@f0c1981gep13@f0c1981 and I have pushed the "hooky" version of 1.2.9 to my MyGet Feed, and you can see that this is what has been installed: And I am trying to read the environment variable here: gep13/ChocolateyGUI@18fe97a#diff-fb0e4ab501cf90c08df493e728f4ec66R267gep13/ChocolateyGUI@18fe97a#diff-fb0e4ab501cf90c08df493e728f4ec66R267 Did I do something wrong? — |
|
Yip, that was brought up the other day by @FeodorFitsner. I am doing this:
When creating the variables, so using _'s. |
|
@FeodorFitsner just thinking about this... I am actually first calling a build.bat file (from I am trying to call GitVersion and read the variables back from the same default.ps1 file. |
|
Absolutely! Running through build.bat is the problem. In this case we have the following processes chain: From GitVersion using REST API you update environment variables of However, should you have so any call to program following GitVersion (child process) will inherit variables from its parent |
|
@FeodorFitsner ah, gotcha. Let me try calling my The reason that I was doing this was because when executing locally, I don't want to have to mess around with setting and unsetting security policies to run the psake script, so I do this:
In my Will report back in a bit... |
|
@FeodorFitsner Just wanted to follow up to say... IT TOTALLY WORKS! 👍 Thanks so much for taking the time to look into this, I really appreciate it. Working output here: |
|
I have added a new Pull Request here: To cover the work required to use the new REST API method when calling |
|
@SimonCropp Can this PR be assigned a milestone? It's companion PR is marked as 1.3.1 |
- Temporarily use the hooky installation source for GitVersion which comes from my MyGet feed - This has the result of Pull Request GitTools/GitVersion#262
- As a result of discussions in this PR GitTools/GitVersion#262 - Changed to make use of a new REST API method in AppVeyor to set environment variables which are then immediately available in the running process. - This means that we no longer have to call GitVersion twice