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

Change the logic inheriting BranchConfiguration from parent branch if the IncrementStrategy is set to Inherit #3190

Merged
merged 69 commits into from
Oct 11, 2022

Conversation

HHobeck
Copy link
Contributor

@HHobeck HHobeck commented Sep 9, 2022

Description

The initial point here is that we need to refactor the approach of determining the EffectiveConfiguration of a branch. If you ask me it should be dynamic dependent of the VersionStrategy implementation. Thus we need to remove the static logic from the GitVersionContextFactory class and replace it with a really simple logic like GetEffectiveConfiguration in GitVersionContext.cs.

In addition I have changed the following things:

  • If you have an empty repository you get an Version number 0.0.0.0 instead of an error (which was really bad). This version number functions as a fallback version. (not part of this PR)
  • Because the fallback number is 0.0.0 the next version number after a commit has been pushed to master is 0.0.1 (because it is a patch branch right!?). If you do a change on develop than the minor version will be incremented. In my opinion the default version 0.1.0 on main was very confusing. (done)
  • You can overwrite the next version via the configuration file. This is not the next version from code point of view. It is the preview released version with should be not incremented (BaseVersion.ShouldIncrement==false). So that means if you have a tagged version e.g. 1.0.0 and set the next version to 2.0.0 than this version should be applied. (done)
  • Reuse or maybe activate a lost feature with the usage of the property TrackMergeTarget. In my opinion the behavior should be always false for the gitflow workflow. The consequence is that in fact the class MergeMessageVersionStrategy.cs is not used for this workflow. Maybe it is good for other scenarios!? (see discussion in [Bug] track-merge-changes produces unexpected result when combining hotfix and support branches #3052 please). Anyway you can set the TrackMergeTarget to true if you want a different behavior. (not part of this PR).
  • Sometimes I see that after a merge to master or to develop the fourth part of the version number (CommitsSinceVersionSource or I call it revision number) is not calculated correct. For instance if you have three changes in a release lets say 1.0.0 and merge it to master (tag with 1.0.0) an merge back to develop and make one change in develop why would you expect the number 1.1.0+5 (3 from release one from develop and one from merge)? I would say in the next release you have just two commits which differs from the previous release. This behavior is also related to TrackMergeTarget=false which can be configured very quickly. (not part of this PR)
  • The generation in that case when you for instance branch the develop branch to e.g. a release/1.0.0 branch. In that scenario the version should be different dependent on what your current branch is. The commit is the same but not the version. From develop point of view it is 1.1.0-alpha.0 and from release point of view it is 1.0.0-beta.0. Zero means no changes since last version which is at that moment true for both. (done)
  • Last but not least when a version has been tagged and you rerun it why would you expect that the CommitsSinceVersionSource number is zero? The fourth part of the version number (not the fourth number at semantic version it is always zero) but the CommitsSinceVersionSource (revision) should be like the name says containing the number of commits within this release. There are absolutely no arguments why the BaseVersionCalculator cannot be executed in that case to determine e.g. the BaseVersion.BaseVersionSource of an previous release. (not part of this PR)

Resolved Issues

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

[Bug] SemVer of a feature branch started from a release branch gets decremented ( Resolves #3151 )

[Bug] Closing pull request from hotfix to support failed to inherit Increment branch configuration ( Resolves #3020 )

[Bug] Wrong semver calculation when making a PR from a hotfix branch to main branch ( Resolves #3187 )

[Bug] Version of commit in develop merged to master changes if master is tagged ( Resolves #3105 )

[Bug] Closing pull request from hotfix to support failed to inherit Increment branch configuration ( Resolves #3020 )

[Bug] Merging develop to release branch makes alpha version jump back ( Resolves #3154 )

Related Issues

[Bug] track-merge-changes produces unexpected result when combining hotfix and support branches #3052 (not part of this PR)

[Issue] GitVersion produces different BuildMetaData once commit is tagged #1397 (not part of this PR)

[Issue] track-merge-target in branch config not working #1789 (not part of this PR)

[Q&A] Prevent decrementation of versions on the develop branch #3177

[Q&A] Increment a feature from the release #3145

Motivation and Context

Here is my view on it and an example how it might be better. All unit tests are adapted right now. The tests I have touched are looking good... That means some unit tests were false positive (or I have a fundamental different view on generating semantic versions). Hope this finds a way to a productive version of GitVersion because I put a lot of effort to this and I want to give something back to this project. GitVersion is great and it should be refactored more in direction of clean code. Thank you very much for given this a chance.

How Has This Been Tested?

I put a lot of time to understand the logic of GitVersion and how it is implemented. For this I have created some additional tests.

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.

… Here is my view on it and an example how it might be better. Not all unit tests are adapted right now from me. But this tests I have touched it looks quite good... That means the unit tests might be not correct implemented and false positive (or I have a fundamental different view on generating semantic versions). Hope this finds a way to a productive version of GitVersion because I put a lot of effort to this because I want to give something back. GitVersion is great and should be refactored more in direction of clean code. Thank you very much for given this a chance.
…l like I would expected it. But at one point I'm not certain. That's related how the version should be incremented when having a different VersioningMode ContinuousDelivery, ContinuousDeployment or Mainline. I think I have misinterpreted the parameter TrackMergeTarget and should be using instead the ContinuousDeployment value instead.
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.

I'm a bit surprised by the number of failing tests. It clearly indicates that we at least need to postpone this change to v6. But before you rebase from support/5.x to main, let's discuss the current implementation and its repercussions. I don't quite understand what you are changing and why, so it's hard to reason about why so many of the tests need to change their expected behavior.

@HHobeck
Copy link
Contributor Author

HHobeck commented Sep 10, 2022

I'm a bit surprised by the number of failing tests. It clearly indicates that we at least need to postpone this change to v6. But before you rebase from support/5.x to main, let's discuss the current implementation and its repercussions. I don't quite understand what you are changing and why, so it's hard to reason about why so many of the tests need to change their expected behavior.

First of all thank you very much for taking the time. This is actually a working copy on the view I have on GitVersion and how I think it should be behave. Of course we can agree on it to make no breaking changes (or at least not so many) and set the parameters in the ConfigurationBuilder (in version 5.x) and make the breaking configuration change later in version 6.x. Sorry that I haven't done it immediately. Anyway I would like to bring the focus on the business use cases and the behavior in general. Maybe I'm wrong and didn't understand the github or gitflow workflow. Please give me your opinion about how the default behavior should be in version 6.x.

The main reason why so many tests fails are:

1. I have introduced a new feature or at least re-implemented the TrackMergeTarget because in my opinion it was missing/lost (please see the discussion 3052). I would love to hear your opinion about that. (not part of this PR)
2. I have set the TrackMergeTarget in ConfigurationBuilder for the develop branch to false. Thus the MergeMessageVersionStrategy.cs is not returning any BaseVersions. If you set it to true than you have the previous behavior. I think in version 5.x I need to set it true on the support, master and develop branch. After that a lot of tests are not failing anymore. (not part of this PR)
3. I have changed the fallback version from 0.1.0 to 0.0.0.
4. In the previous version a repository with just one commit returns the FullSemVer 0.1.0+0. This is not intentionally. First of all it is a patch branch so why it's not return a 0.0.1 or at least the 0.1.1 (see point three). Second the CommitsSinceVersionSource has the value zero but I have one commit in this release. Maybe I have a different understanding or view on this value. But for me it indicates the number of commit since the last released version.
5. If you call GitVersion on an empty repository you get the version 0.0.0+0 back. (I change it back in version 5.x no problem). Or maybe I remove the hole feature that would also fine with me. The reason why I not feeling comfortable that GitVersion fails with an exit code in this situation was: I think GitVersion should not break any build processes. (not part of this PR)

@HHobeck
Copy link
Contributor Author

HHobeck commented Sep 10, 2022

Please do have a look. In this version the previous unit tests should be not breaking anymore.

…mmitsSinceVersionSourceShouldNotGoDownWhenMergingHotfixToDevelop test
…instances. In advance add a overload to specify the initial branch name for the empty repository. Last but not least the logic in BaseVersionCalculator has been adapted: If the maximal version has no pre-release tag defined than we want to determine just the latest previous base source which are not comming from pre-release tag.
@asbjornu
Copy link
Member

Can you please run dotnet format so the test becomes successful? I can also see the tests failing with the following error:

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

Which I've noticed before and don't quite understand why is happening. @arturcic, do you have any ideas?

…sion strategy implementation is highly recursive and works with inheritance from one branch configuration to another. This makes it highly configurable and we can support every workflow on the world and all workflows which are created in the future. Please notice the BranchConfigurationCalculator is not really used anymore.
@HHobeck
Copy link
Contributor Author

HHobeck commented Sep 15, 2022

Can you please run dotnet format so the test becomes successful? I can also see the tests failing with the following error:

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

Which I've noticed before and don't quite understand why is happening. @arturcic, do you have any ideas?

I got following error:

PM> dotnet format
dotnet : Warnings were encountered while loading the workspace. Set the verbosity option to the 'diagnostic' level to log warnings.
At line:1 char:1
+ dotnet format
+ ~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (Warnings were e...o log warnings.:String) [], RemoteException
    + FullyQualifiedErrorId : NativeCommandError

@asbjornu
Copy link
Member

Try the following, @HHobeck:

dotnet format ./src/ --exclude **/AddFormats/

The test GitVersion.Core.Tests.DynamicRepositoryTests.FindsVersionInDynamicRepo fails twice:

Failed FindsVersionInDynamicRepo("GV_main","https://github.com/GitTools/GitVersion","main","efddf2f92c539a9c27f1904d952dcab8fb955f0e","5.8.2+56") [4 s]
Error Message:
   String lengths are both 8. Strings differ at index 2.
Expected: "5.8.2+56"
But was:  "5.10.4+0"
Failed FindsVersionInDynamicRepo("GV_main","https://github.com/GitTools/GitVersion","main","2dc142a4a4df77db61a00d9fb7510b18b3c2c85a","5.8.2+47") [934 ms]
Error Message:
   String lengths are both 8. Strings differ at index 2.
Expected: "5.8.2+47"
But was:  "5.10.4+0"

Is that intentional with this change?

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.

Great stuff. 👏🏼 Very close now! 😃 👍🏼

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 11, 2022

The FullConfiguration is a property in GitVersionContext of type Config and contains the overall configuration. Should be renamed to maybe Configuration.

Yes please. 👍🏼

The refactoring step to rename the Config to Configuration class is quite huge. I would propose to do this in a separate PullRequest. But in this PR I have changed the FullConfiguration property and named it to Configuration.

@asbjornu
Copy link
Member

The refactoring step to rename the Config to Configuration class is quite huge. I would propose to do this in a separate PullRequest.

Yes, agreed.

But in this PR I have changed the FullConfiguration property and named it to Configuration.

Perfect! 👌🏼

BREAKING_CHANGES.md Outdated Show resolved Hide resolved
Co-authored-by: Asbjørn Ulsberg <[email protected]>
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.

Magnificent! 👏🏼

@asbjornu asbjornu added this to the 6.x milestone Oct 11, 2022
@asbjornu asbjornu merged commit 92388ad into GitTools:main Oct 11, 2022
@mergify
Copy link
Contributor

mergify bot commented Oct 11, 2022

Thank you @HHobeck for your contribution!

@arturcic
Copy link
Member

@asbjornu please include all the fixed bugs/ issues and this PR to the V6 milestone so that we get them included in the release notes

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 12, 2022

@asbjornu On top of the current main branch I would do the renaming of BranchConfig -> BranchConfiguration and Config -> Configuration and create a PR. Is it okay with you?

@HHobeck
Copy link
Contributor Author

HHobeck commented Oct 12, 2022

Please review the following PR: #3226

@asbjornu
Copy link
Member

@asbjornu On top of the current main branch I would do the renaming of BranchConfig -> BranchConfiguration and Config -> Configuration and create a PR. Is it okay with you?

From To Ok? Explanation
BranchConfig BranchConfiguration 👍🏼
Config Configuration 👎🏼 Because then we need to rename the namespace from Configuration to something else.
Config GitVersionConfiguration 👍🏼 Because it reflects GitVersion.yml well and doesn't collide with the namespace.

@arturcic
Copy link
Member

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

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment