Get-BuildVariable: inconsistent PATH parameter usage

[peppekerstens]

Build environment
Azure Devops

Tested
PS5.1/PS6core
build stage

Issue
the commit message detection code ((https://github.com/RamblingCookieMonster/BuildHelpers/blob/303eebee0bf92f9e086f131d89cf478a6ff87275/BuildHelpers/Public/Get-BuildVariable.ps1#L154) ignores a provided path.

when not using the 'standard ' location, but by providing the 'path' parameter to either Get-BuildEnvironment or Get-BuildVariable, Azure Devops returns an error:

##[error]Invoke-Git : fatal: d:\a\1\s: 'd:\a\1\s' is outside repository
##[error]At C:\Users\VssAdministrator\Documents\WindowsPowerShell\Modules\BuildHelpers\2.0.9\Public\Get-BuildVariable.ps1:189

cause
Get-BuildVariable (and Get-BuildEnvironment) has a 'PATH" parameter to provide a path to the variables to detect. Yet, in the commit message part of the code, this is totally ignored by just using the detected environment values.

Although the $path variable is part of the invoke-git @IGParams part, in the Git command itself

"log --format=%B -n 1 $( (Get-Item -Path "ENV:$_").Value )"

it is not used, and thus ignored. This results in strange behavior when providing a path with these commands.

[FISHMANPET]

Well that probably explains why that code was using Git instead of just pulling the details from Environment variables.

So I assume in this case you're in a release pipeline, and you've specified an alternate checkout path so the SYSTEM_DEFAULTWORKINGDIRECTORY isn't actually where your code is. And then even though you specify a path it's ignored and it looks for your git repo in the working directory. So in that case Using the specified path rather than the working directory as the location for the Git command should work, right?

[FISHMANPET]

Ok, so this is my fault, because I passed in a directory when the command was expecting a revision, and that caused all sorts of unintended weirdness. Ironically I made that change because a previous change broke my pipeline because it was... passing a directory instead of a revision.

So I think the only change I need to make is replace SYSTEM_DEFAULTWORKINGDIRECTORY with BUILD_SOURCEVERSION which will be the commit hash and so the Invoke-GIt command will grab that hash from your specified directory instead of grabbing the working directory. Ironically I waffled on which seemingly identical variables to use there and I think if I'd used BUILD_SOURCESDIRECTORY it would have just accidentally worked.

[peppekerstens]

I'll be posting a PR soon with fixes :)

[FISHMANPET]

So I think that sort of works but it's kind of dancing around the problem maybe.

The path passed in will ultimately get passed to Invoke-Git and if you dig in there it will actually be the directory from which that git command will be executed.

The current version, when it finds SYSTEM_DEFAULTWORKINGDIRECTORY, will execute a git command in the PATH directory. Git is pretty flexible such that in a command you can specify a revision (branch name, commit ID, etc) or you can also give it a directory. In the first case it will will execute that git log command in the PATH directory looking for the the given revision, which means it gets that revision from the git repo in that location. In the second case the git command executes in the PATH location but by passing in a location instead of a revision, it just runs that command for HEAD in whatever that location is. So in the case here where we call it with a directory, you end up with whatever is in that directory, regardeless of where you executed the Git command from.

If you look at the other options in that switch parameter, every other one is a commit id, which is really funny that I introduced that because in my pull request I basically wrote this same explanation, so I knew not to pass in a directory when I should pass a commit hash, and I just... did it anyway.

I see you replaced SYSTEM_WORKINGDIRECTORY with AGENT_RELEASEDIRECTORY. So that should solve the issue where both variables are present since the message is only present in build and yaml and yours is only present in release, so they should never both be there. But it's still passing in a directory where there should be a hash. It goes against the pattern but it would probably work to replace the code that actually runs git so that if there's an AGENT_RELEASEDIRECTORY, check if there's a BUILD_SOURCEVERSION pass the value of that into the git command.

That would also negate the whole check on if path was passed in that you added around that block, which is really more of a workaround that may break other things rather than fixing the issue (which again, is passing in a directory name to git log when it should be commit hash).

If there's one thing I've learned in the time I've been using this module, especially in Azure Devops, it's that you have to be really careful with changes made, and for every change made you have to think about why it works in your environment and think really hard about what assumptions you've made about other people's environments.

When I first started using this module it just worked and I didn't think about it, and I came back later and it stopped working, and it's because someone changed the way it got data in Azure Devops for release pipelines, and it worked for them but it broke my environment. So I fixed that in a way I thought would work everywhere but apparently I was wrong and I broke it for you. So don't be like me and break it for the next person 😄

[peppekerstens]

My reasoning for these changes;

• 'BUILD_SOURCEVERSIONMESSAGE' was already in version 2.0.9.
• 'SYSTEM_WORKINGDIRECTORY' was already pointing to an path, 'AGENT_RELEASEDIRECTORY' is pointing to same path.
• I could not find any DevOps build-in var for release, pointing to hash
• minimize change compared to 2.0.9 for exactly the reason you mention; I want to change as little as possible to maximize/guarantee a working solution.

But you are correct; compared to other environment codes in Get-Variable, 'BUILD_SOURCEVERSION' seems more appropriate. Maybe next release? :)

I do not agree that the if path statement is a workaround. Any function should 'honor' its parameters, not ignore it. If i point toward a path, i want that path to be processed/checked, not what some environment var says. The added code is ensuring that that happens, no matter what is in the switch statement. I did not add that code to workaround the detection problem of Azure Devops.

[peppekerstens]

@FISHMANPET one last thing; in another issue you argument that this module is/can be used in different ways. (in your case it was a PS script, not a module if I recall correctly).

Same goes here; in our case, we download the Git repo's ourselves instead of via the DevOps agent.
So I expect the command Get-BuildEnvironment/Get-BuildVariable to obey/process my provided -Path value. What is the added value of this parameter if this is ignored?

[FISHMANPET]

The thing is, it is obeyed, as long we pass a commit hash and not a directory into git log. The root of the problem is that a directory was passed into git log instead of a commit hash, which essentially overrode the path parameter that was passed into Invoke-Git. So while your solution technically works, it does it by just avoiding the problem entirely. And if you don't specify a path but there isn't a git repo in SYSTEM_WORKINGDIRECTORY it will still fail so you've written a workaround that fixes your situation specifically but doesn't actually fix the original problem.

[FISHMANPET]

So, ok, spent 2 whole minutes thinking about it and... I think we're both right? If you're not using the builtin git steps then are variables like BUILD_SOURCEVERSION and BUILD_SOURCEVERSIONMESSAGE even populated? Or are they present but not populated?

I think maybe there's two cases for specifying a path to Set-BuildEnvironment or it's descendents. The one I'm thinking of is where you've told Azure Devops to checkout your code into an alternate directory and so the current directory is probably going to be your SYSTEM_WORKINGDIRECTORY but your code might be in SYSTEM_WORKINGDIRECTORY\MyGitPath. So Specifying a path in that case is a hint that things are over here actually. Your case it sounds like you're doing things entirely manually, and you don't want to rely on some of the automatic stuff that gets set, so you're passing the directory not as a hint of where things are, but as an override, telling BuildHelpers "I know you want to do it this way, but trust me you should do it this way instead." I don't know what BUILD_SOURCEVERSION would be in your case but if it's a commit different than the commit you've manually checked out, then it will work in the sense that it doesn't throw an error, but it will return bogus data because the hint wasn't enough. It's almost like there should be two path parameters, one a hint path, one a force path.

[peppekerstens]

We're getting somewhere :)

Yes, they are always populated. But in my particular case I want them ignored. Especially when I start a release; then i gather A LOT OF PS modules, all updating them to 'stable' in 1 go by using PowerShell function(s) from yet another helper PS module build upon/depending on this one.

In my point of view, the default and preferred method should indeed be to 'automagically' detect the latest commit message. But i cannot/could not figure out the added value of the path parameter unless it is obeyed in the detection. Current path assignment.....assumes a lot. My autism cannot cope with "maybe your provided path is honored , but only i can't find any environment var. In that case, you're out of luck" :))

Can we settle on an extra switch statement? Something like -NoDetect or -UseOnlyPath?

[FISHMANPET]

I'm thinking about this more (WHY HAVE I BECOME OBSESSED WITH THIS) and I'm wracking my brain trying to figure out how a path "hint" could be useful. Like, what is the scenario where I have a repo somewhere other than root, and my CI has an environment variable for the commit it's working on (which is presumably the same one that tool has checked out), and I want git to get the commit message of that specific commit ID rather than just getting the commit message for HEAD (which, again, should be the same as the commit ID in the variable). If I'm doing crazy things like changing which commit I'm checking out within the CI then that's probably outside the use case of a tool like this that's meant to standardize across CIs.

But there's still the bug you're not fixing, which is that that block of code passes a directory to Git when it should pass a commit hash. The path would be obeyed if there wasn't the bug of an extra path being passed in which overrides the parameters to Invoke-Git (if you look at the code for Invoke-Git it's using that passed path as the directory where it executes Git from, which is how it's supposed to work). But even with that logic bug fixed, the best outcome is that it looks for the build CI's commit ID in that specific directory, not what's currently checked out in the given directory.

So upon further reflection, I, a random person who started contributing to this project a month ago (😜), thinks it's reasonable that if a path is passed to Set-BuildEnvironment, that as many variables as possible are populated by running Git directly against that directory rather than merely using the directory as a hint for getting data based on environment variables. This also opens up the possibilities of doing crazy things when you've got multiple repos, like calling Set-BuildEnvironment with different VariableNamePrefixes to get variables for each individual repo if you wanted.

Looking at what's currently there, I can imagine BranchName, CommitMessage, CommitHash all operating like this, where if given a path they just run Git directly to get the information rather than relying on any environment variables. The rest of what's populated seems to be about the build run itself rather the Git repo so they can stay how they are (and currently none of those other variables get any data from Git)

[RamblingCookieMonster]

Hi! Haven't caught up on this thread, but I think $Path might need to be adjusted? Invoke-Git actually sets the processes working directory to this, so git log should be running from that path?

I think we need to remove SYSTEM_DEFAULTWORKINGDIRECTORY from commit message detection per #118 , but might be missing something