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

Refactoring/remove the EffectiveConfiguration property in the GitVersionContext class #3215

Conversation

HHobeck
Copy link
Contributor

@HHobeck HHobeck commented Oct 1, 2022

Description

Refactoring/remove the EffectiveConfiguration property in the GitVersionContext class. This change is a preparation for fixing the issue #3101 in PR #3190 and targets the v5 version. No business logic changed.

Related Issue

[Bug] Version not generated correct when creating a feature branch from a release branch #3101

Motivation and Context

This PR is a preparation for PR #3190 and should be integrated in v5.

How Has This Been Tested?

All unit and integration tests have been executed.

Screenshots (if appropriate):

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.

…ionContext class. This change is a preparation for fixing the issue GitTools#3101 in PR GitTools#3190 and targets the v5 version. No business logic changed.
@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 2, 2022

Hi @asbjornu and @arturcic. Do you have an idea why the CI pipeline not passing?

CodeQL / Analyze (csharp) (pull_request):

  ========================================
  CodeFormat
  ========================================
  Code format...
  Unrecognized command or argument '-p:UseSharedCompilation=false'.
  Unrecognized command or argument '-p:UseSharedCompilation=false'.
  
  Description:
    Formats code to match editorconfig settings.
  
  Usage:
  format [<The project or solution file to operate on. If a file is not specified, the command will search the current directory for one.>] [command] [options]

CI / Unit Test code (windows-latest, net5.0) (pull_request):

Error: unknown test file type for 'artifacts/test-results/GitVersion.MsBuild.Tests.net5.0.results.xml'

@arturcic
Copy link
Member

arturcic commented Oct 3, 2022

I will check this one

@arturcic
Copy link
Member

arturcic commented Oct 3, 2022

@HHobeck seems like recently codeql changed a bit the way they run the build so I had to fix that. You will need to rebase on top of support/5.x to have this fixed

@asbjornu
Copy link
Member

asbjornu commented Oct 3, 2022

@arturcic, we also have a problem with the Test Summary step:

Error: unknown test file type for 'artifacts/test-results/GitVersion.App.Tests.net48.results.xml'

I think the issue may be that that the test-summary action does not support NUnit test reports. Perhaps we should just remove the Test Summary step until test-summary/action#12 is resolved?

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

Thank you so much for your hard work on this @HHobeck! It is much appreciated. 🙏🏼 Please have a look at my review comments. As soon as they are resolved and the tests are fixed, I think this should be merged. Then we'll do a merge of support/5.x into main so you can rebase #3190 on top of the great work you've done here.

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

👏🏼 Now we just need to figure out what to do with the failing Test Summary task. Thoughts @arturcic?

@arturcic
Copy link
Member

arturcic commented Oct 3, 2022

👏🏼 Now we just need to figure out what to do with the failing Test Summary task. Thoughts @arturcic?

The interesting thing is on main the Test summary works just fine, sometimes the macos is failing but on rerun it's ok

@asbjornu
Copy link
Member

asbjornu commented Oct 3, 2022

The interesting thing is on main the Test summary works just fine, sometimes the macos is failing but on rerun it's ok

Is it really fine, or does it work when artifacts/test-results/*.results.xml returns no files, while it fails when it does?

@arturcic
Copy link
Member

arturcic commented Oct 3, 2022

The interesting thing is on main the Test summary works just fine, sometimes the macos is failing but on rerun it's ok

Is it really fine, or does it work when artifacts/test-results/*.results.xml returns no files, while it fails when it does?

if you check this run for example https://github.com/GitTools/GitVersion/actions/runs/3173292935 you will see 5 test summaries for every platform excluding macos + netcore3.1

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 3, 2022

The interesting thing is on main the Test summary works just fine, sometimes the macos is failing but on rerun it's ok

Is it really fine, or does it work when artifacts/test-results/*.results.xml returns no files, while it fails when it does?

if you check this run for example https://github.com/GitTools/GitVersion/actions/runs/3173292935 you will see 5 test summaries for every platform excluding macos + netcore3.1

What does this mean? I can see the following error:

Run test-summary/action@v1
Error: unknown test file type for 'artifacts/test-results/GitVersion.MsBuild.Tests.net5.0.results.xml'

On the main branch this error seems to be not existing.

@arturcic
Copy link
Member

arturcic commented Oct 4, 2022

@HHobeck I think on support branch we can disable the test-summary action, so please rebase again on support branch, I disabled it there

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 4, 2022

@HHobeck I think on support branch we can disable the test-summary action, so please rebase again on support branch, I disabled it there

No problem I can do this. Please let me know if you have changed it.

image

@arturcic
Copy link
Member

arturcic commented Oct 4, 2022

@HHobeck you can rebase it now

@arturcic
Copy link
Member

arturcic commented Oct 4, 2022

@HHobeck you don't need to push new commits to re-trigger the failed unit test step. I can retrigger it

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 4, 2022

Thank you very much @arturcic and @asbjornu for your input and the good team work. After the code base has been merged to the support/5.x branch we need to merge it to the master branch right?

@arturcic
Copy link
Member

arturcic commented Oct 4, 2022

Yes, that is the plan

@arturcic arturcic merged commit 3e5d1e7 into GitTools:support/5.x Oct 4, 2022
@arturcic arturcic added this to the 5.x milestone Oct 4, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 4, 2022

Thank you @HHobeck for your contribution!

@arturcic arturcic modified the milestones: 5.x, 5.11.0 Nov 7, 2022
@arturcic
Copy link
Member

arturcic commented Nov 7, 2022

🎉 This issue has been resolved in version 5.11.0 🎉
The release is available on:

Your GitReleaseManager bot 📦🚀

@HHobeck HHobeck deleted the feature/3101_remove-git-version-context-configuration branch March 13, 2023 06:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants