Skip to content
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

Removing 'path' parameter from 'git log' command #118

Merged
merged 2 commits into from
Sep 19, 2019

Conversation

raandree
Copy link
Contributor

@raandree raandree commented Sep 16, 2019

This fixes #109 and #110.

As far as I have understood the git documentation, the path parameter for 'git log' is more like a filter to get the last commit for a specific directory or file. So here the path is it used twice and the two values may contradict each other . The current path must be set to somewhere in the git repo anyway and then - if things are as expected - the path (SYSTEM_DEFAULTWORKINGDIRECTORY) shows to the same repo again. In my case, I am having more than one artifact to deal with and the path in SYSTEM_DEFAULTWORKINGDIRECTORY does not point to the repo. The repo is in a separate folder in SYSTEM_DEFAULTWORKINGDIRECTORY.

And if the commit message could not be retrieved, we get in in line 224 without the path.

I don't see any value in defining the path as it is done for the 'git log' calls. Please correct me if I am wrong.

@@ -160,21 +160,21 @@ function Get-BuildVariable {
'CI_COMMIT_SHA' {
if($WeCanGit)
{
Invoke-Git @IGParams -Arguments "log --format=%B -n 1 $( (Get-Item -Path "ENV:$_").Value )"
Invoke-Git @IGParams -Arguments "log --format=%B -n 1"
Copy link
Owner

@RamblingCookieMonster RamblingCookieMonster Sep 17, 2019

Choose a reason for hiding this comment

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

So! In this section (getting commit message), the deleted bit is the commit sha. If you leave this off, AFAICT it will get the current commit. Potential corner case: Someone has been moving around in their build (checking out a different commit), but still want to refer to the commit message of the build's commit.

Do these lines ever error out for you? Maybe I'm grabbing a wrong environment variable? Intention in this section is to grab commit to do this: https://stackoverflow.com/a/3357357

@@ -186,21 +186,21 @@ function Get-BuildVariable {
'SYSTEM_DEFAULTWORKINGDIRECTORY' { #Azure Pipelines, this will be triggered in the case of a classic release pipeline
if($WeCanGit)
{
(Invoke-Git @IGParams -Arguments "log --format=%B -n 1 $( (Get-Item -Path "ENV:$_").Value )").split([Environment]::NewLine,[System.StringSplitOptions]::RemoveEmptyEntries) -join " "
(Invoke-Git @IGParams -Arguments "log --format=%B -n 1").split([Environment]::NewLine,[System.StringSplitOptions]::RemoveEmptyEntries) -join " "

Choose a reason for hiding this comment

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

Yeah this one is weird. It should be a commit sha, not a path, not sure what happened.

@RamblingCookieMonster
Copy link
Owner

Okay, added comments! Long story short, intention is to use an environment variable that indicates the current build's commit sha, and to get the commit message associated with that commit via this method.

As mentioned, you could imagine a hypothetical, perhaps ill-advised scenario where someone move around in their repo (to a different commit), and still wants to refer to the current commit message. Removing the git commit sha from the command in that case will AFAIK get the commit message for what they have checked out, rather than for the commit he build is running against

Let me double check, but yeah, we should totally scrap SYSTEM_DEFAULTWORKINGDIRECTORY. Is there a commit sha environment variable we can point to instead of that?

@raandree
Copy link
Contributor Author

Ok, got the point of that. But what happens using the path is actually this:

Like you do in your Init task, my build process sets the location to $ProjectRoot. Then getting the log with the path as an argument fails like that.

PS C:\BuildWorkerSetupFiles\_work\r2\a\CommonTasks CI\SourcesDirectory> git branch
* (HEAD detached at 417779a)
PS C:\BuildWorkerSetupFiles\_work\r2\a\CommonTasks CI\SourcesDirectory> git log -n 1 C:\BuildWorkerSetupFiles\_work\r2\a

fatal: C:\BuildWorkerSetupFiles\_work\r2\a: 'C:\BuildWorkerSetupFiles\_work\r2\a' is outside repository

If we use the Git Commit Id, things are much better:

PS C:\BuildWorkerSetupFiles\_work\r2\a\CommonTasks CI\SourcesDirectory> git log -n 1 417779a652535f229430e6289a229b85fba
f6b5f
commit 417779a652535f229430e6289a229b85fbaf6b5f (HEAD, origin/dev)
Author: Install <[email protected]>
Date:   Tue Sep 17 09:50:20 2019 +0530

    Test Commit 2

For Azure Pipelines, according to Release variables and debugging there are two variables storing the Git Commit Id:

  • Build.SourceVersion
  • Release.Artifacts.{Primary artifact alias}.SourceVersion

In AppVoyer the variable is the varialble 'APPVEYOR_REPO_COMMIT' but one can get the commit message from 'APPVEYOR_REPO_COMMIT_MESSAGE'.

I am not sure about all the other build systems that you support.

@RamblingCookieMonster
Copy link
Owner

Perfect! So! The only case where that would be a path in the code is for SYSTEM_DEFAULTWORKINGDIRECTORY.

If we leave $( (Get-Item -Path "ENV:$_").Value ) in each spot, and change SYSTEM_DEFAULTWORKINGDIRECTORY to Build_SourceVersion (it appears underscores are used in place of periods in environment variable names), then all of those cases should evaluate to variables that store a commit hash, rather than a path.

@raandree
Copy link
Contributor Author

OK, changed the code to the previous state and replaced the variable 'SYSTEM_DEFAULTWORKINGDIRECTORY' with 'BUILD_SOURCEVERSION'.

@RamblingCookieMonster
Copy link
Owner

Works for me, merging, thanks, and shooot forgot the conf started, sorry!

@RamblingCookieMonster RamblingCookieMonster merged commit e7c4fc6 into RamblingCookieMonster:master Sep 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get-BuildVariable: inconsistent commit message detection
2 participants