Use GITHUB_REF only for CI builds on branches

[ThomasPiskol]

Improve support for GitHub Actions and fix #2838

Description

In case GitVersion is running during GitHub Actions CI build, the environment variable GITHUB_REF is used by GitVersion to get the current branch.

But GitHub Actions only provide branch name here in case of a "normal" CI build. If the GitHub Action was triggered because of a tag event or pull request event, the GITHUB_REF environment variable does not contain an existing branch name as described by GitHub:

The branch or tag ref that triggered the workflow run. For branches this is the format refs/heads/<branch_name>, for tags it is refs/tags/<tag_name>, and for pull requests it is refs/pull/<pr_number>/merge. This variable is only set if a branch or tag is available for the event type. For example, refs/heads/feature-branch-1.

Related Issue

Fixes #2838.
Fixes #2852.
Fixes #2301.
Fixes #2928.
Fixes #2869.
Related to #2074.

Motivation and Context

Bug fix and align with GitHub Actions concept.

How Has This Been Tested?

Three GitHub Actions scenarios must be tested:

1. "normal" GitHub Actions CI build
2. Tag Event
3. Pull Request Event

https://github.com/ThomasPiskol/gitversion-repro/actions

There's no change to the "normal" the Pull Request events, they create the same version string as before.
So this change should fix the issue without causing some issue for the other workflows.

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[asbjornu]

Thank you so much for this, @ThomasPiskol! However, looking a bit closer at this, I see that other build server tests have similar assertions to those of GitHub Actions (see review comments).

And then we have tests for Drone and Jenkins which assert something completely different (without /refs/heads/):

GitVersion/src/GitVersion.Core.Tests/BuildAgents/DroneTests.cs

Lines 102 to 118 in 666bd37

	[Test]
	public void GetCurrentBranchShouldUseDroneBranchInCaseOfPullRequestAndEmptyDroneSourceBranchAndCiCommitRefSpec()
	{
	// Arrange
	const string droneBranch = "droneBranch";
	this.environment.SetEnvironmentVariable("DRONE_PULL_REQUEST", "1");
	this.environment.SetEnvironmentVariable("DRONE_SOURCE_BRANCH", "");
	this.environment.SetEnvironmentVariable("CI_COMMIT_REFSPEC", "");
	this.environment.SetEnvironmentVariable("DRONE_BRANCH", droneBranch);
	// Act
	var result = this.buildServer.GetCurrentBranch(false);
	// Assert
	result.ShouldBe(droneBranch);
	}

GitVersion/src/GitVersion.Core.Tests/BuildAgents/JenkinsTests.cs

Line 59 in 666bd37

this.buildServer.GetCurrentBranch(true).ShouldBe($"origin/{MainBranch}");

So I'm not really sure why this is working for some build servers and not others. I'm also not sure what the correct expectation for GetCurrentBranch() is supposed to be. But I think we need to harmonize the behavior here across all build server implementations and not just change the behavior of GitHub Actions.

Thoughts?

PS: I took the liberty to edit your PR description to add more related issues.

[ThomasPiskol]

Thanks for changing the description and linking the other issues 🙏

I agree, the main question is what is the expected behaviour of the ICurrentBuildAgent.GetCurrentBranch() method.

For me it seems that some additional "magic" happens in GitPrepare.EnsureLocalBranchExistsForCurrentBranch(IRemote? remote, string? currentBranch). The result of ICurrentBuildAgent.GetCurrentBranch() is passed via GitPrepare.NormalizeGitDirectory() to this method.

In the log file which I have created to reproducing #2838 I can see the following line:

2022-03-05 18:46:30		  INFO [03/05/22 18:46:30:88] Creating local branch tags/1.2.1 pointing at e597374

And it seems that's the origin of this log message:

GitVersion/src/GitVersion.Core/Core/GitPreparer.cs

Lines 383 to 385 in 666bd37

	this.log.Info(isBranch
	? $"Creating local branch {referenceName}"
	: $"Creating local branch {referenceName} pointing at {repoTipId}");

So it seems that this line is doing some "magic" with the branch name. Before entering this method the branch name has the value refs/tags/1.2.1, but it's changed to refs/heads/tags/1.2.1:

GitVersion/src/GitVersion.Core/Core/GitPreparer.cs

Lines 360 to 364 in 666bd37

	var localCanonicalName = !isRef
	? "refs/heads/" + currentBranch
	: isBranch
	? currentBranch
	: currentBranch.Replace("refs/", "refs/heads/");

This leads to the issue, that a branch with the name tags/1.2.1 is created and checked-out. And because of this, we have the strange SemVer as described in #2838.

So I'm not sure, where we should fix this. On one side it seems correct, to change the ICurrentBuildAgent.GetCurrentBranch() implementations so that they only return "real" branch names and not the special references for tags or pull requests.

On the other side it seems also feasible to change the behavior of EnsureLocalBranchExistsForCurrentBranch, but I guess the current code exists because of a reason and it seems to me quite risky to change this implementation because EnsureLocalBranchExistsForCurrentBranch is also used outside of the build-server context.

So it seems less risky to change all the implementations of ICurrentBuildAgent.GetCurrentBranch() to have a consistent behaviour.

I'll update the PR in the coming days and until then, we can think about the impact of the changes 🙂

[asbjornu]

If you take a look at #2928 (comment), it seems like this problem actually exists in GitLab as well. Which gives me a bit confidence in that taking a build server agnostic approach to fixing this is the right thing to do.

So it seems less risky to change all the implementations of ICurrentBuildAgent.GetCurrentBranch() to have a consistent behaviour.

I agree. I'm not 100% sure what behaviour that should be, but I can't think of a reason why GetCurrentBranch() should return tag references.

[ThomasPiskol]

I've adapted the build server implementations for

• Azure Pipelines
• CodeBuild
• Buildkite
• GitHub
• GitLab (should also fix [Bug] Tag not used as version when running in GitLab Pipeline (Mainline) #2928)
• Space Automation

No changes required for the following implementations, because they don't have a custom implementation for GetCurrentBranch and GetCurrentBranch returns alwaysnull:

• ContinuaCI
• EnvRun
• MyGet
• Travis CI

There are some other implementations but I'm not familiar with them and the current implementations look ok from my point of view:

• AppVeyor
• Drone
• Jenkins
• TeamCity

[ThomasPiskol]

One question: the default branch is now support/5.x but this PR still targets main.

Is this ok or should I change the PR to target support/5.x?

[arturcic]

Please switch to 5.x, we will eventually merge that into main as well

[asbjornu]

LGTM!

[ThomasPiskol]

@asbjornu Can you restart the failed job?

It looks like an issue with the agent who was executing the job (timeout) and I don't have the permission to restart the job.

[mergify]

Thank you @ThomasPiskol for your contribution!

[arturcic]

@ThomasPiskol I had to revert this PR, if you have time please re-work this PR

[ThomasPiskol]

I've reproduced #3081 with an Azure DevOps pipeline. It's complicated, here's what I've observed so far.

Azure DevOps creates a branch for any PR according to the naming scheme: ref/pull/<PR ID>/merge.

Due the change in this PR we end up in this branch of the switch case:

GitVersion/src/GitVersion.Core/Core/GitPreparer.cs

Lines 235 to 238 in b17d33e

	case 0:
	this.log.Info($"No local branch pointing at the commit '{headSha}'. Fake branch needs to be created.");
	this.retryAction.Execute(() => this.repository.CreateBranchForPullRequestBranch(authentication));
	break;

and we hit the exception "LibGit2Sharp.LibGit2SharpException: this remote has never connected" in LibGit2Sharp

A first look shows that there many places in GitVersion which assume that any branch has a ref starting with ref/HEAD but Azure DevOps creates these special PR branches with ref/pull/... which cause some interesting side effects.

I'll focus at first to reproduce this behaviour with an automated test.

[arturcic]

Thanks for taking the time to investigate