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

#2592: GitVersion.MsBuild does not set environment variables for GitHub Action #2611

Merged
merged 12 commits into from
Mar 22, 2021
Merged

Conversation

ThomasPiskol
Copy link
Contributor

This PR includes code changes and tests to fix #2592.

Description

The actual change in src/GitVersion.Core/BuildAgents/GitHubActions.cs is quite small.
Most of the code changes are related to the NUnit tests. I've renamed a few methods, e.g. ExecuteMsBuildTaskInBuildServer to ExecuteMsBuildTaskInAzurePipeline because they are actually testing the Azure Pipelines integration and not a generic build server integration.
Additionally I've added some tests (WriteVersionInfoTaskShouldUpdateBuildNumberInAzurePipeline) to reproduce #2552. Right now these tests are ignored.

Related Issue

#2592

Motivation and Context

Previously the GitHub Action integration was coupled to the setting update-build-number.
This behaviour is not consistent with the other build server integrations and also not intended from my point of view.
Adding the GitVersion environment variables does not change the build number and should be always done independently of update-build-number.

How Has This Been Tested?

  • added new automated tests (see below)
  • manually tested with a locally created NuGet package of GitVersion.MsBuild which was consumed by a test project.

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.

@arturcic arturcic self-requested a review March 2, 2021 08:40
@arturcic
Copy link
Member

arturcic commented Mar 2, 2021

I will have a look. Honestly I would prefer to keep the ExecuteMsBuildTaskInBuildServer generic, but I will check the change you did

@ThomasPiskol
Copy link
Contributor Author

@arturcic Shall I rename ExecuteMsBuildTaskInAzurePipeline back to ExecuteMsBuildTaskInBuildServer ?

@arturcic
Copy link
Member

Ok, I had a look now, I would agree we need to have different names for AzurePipelines and got GitHubActions, merging

@arturcic arturcic merged commit abd6f67 into GitTools:main Mar 22, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2021

Thank you @ThomasPiskol for your contribution!

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.

[Bug] GitVersion.MsBuild does not set environment variables
2 participants