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

HHobeck · 2022-09-09T13:06:24Z

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.

asbjornu

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.

src/GitVersion.App.Tests/ExecCmdLineArgumentTest.cs

src/GitVersion.Core.Tests/Core/GitVersionExecutorTests.cs

src/GitVersion.Core.Tests/IntegrationTests/JustSomeTestScenarios.cs

src/GitVersion.Core/VersionCalculation/BaseVersionCalculators/VersionStrategyBase.cs

src/GitVersion.Core/VersionCalculation/BaseVersionCalculators/VersionStrategyModule.cs

src/GitVersion.Core/VersionCalculation/MainlineVersionCalculator.cs

src/GitVersion.Core.Tests/Core/DynamicRepositoryTests.cs

....Core.Tests/Configuration/ConfigProviderTests.CanWriteOutEffectiveConfiguration.approved.txt

HHobeck · 2022-09-10T06:09:29Z

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)

…et TrackMergeTarget to true for the main branch.

…et TrackMergeTarget to true for the support branch.

…et TrackMergeTarget to true for the develop branch.

HHobeck · 2022-09-10T09:59:37Z

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.

… develop branch GitTools#3177

asbjornu · 2022-09-12T11:23:32Z

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?

…rackMergeTarget functionality.

src/GitVersion.Core.Tests/IntegrationTests/PreventDecrementationOfVersionsOnTheDevelopBranch.cs

…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.

…merge to main

HHobeck · 2022-09-15T16:31:07Z

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

…Support

asbjornu · 2022-09-15T22:15:34Z

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?

…tag will be present only on fallback branch configuration settings.

…into feature/3101

asbjornu

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

BREAKING_CHANGES.md

...ersion.Core.Tests/VersionCalculation/Strategies/ConfigNextVersionBaseVersionStrategyTests.cs

HHobeck · 2022-10-11T12:08:58Z

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 · 2022-10-11T12:12:01Z

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! 👌🏼

…nsions class.

BREAKING_CHANGES.md

Co-authored-by: Asbjørn Ulsberg <asbjorn@ulsberg.no>

asbjornu

Magnificent! 👏🏼

mergify · 2022-10-11T19:39:14Z

Thank you @HHobeck for your contribution!

arturcic · 2022-10-11T20:56:46Z

@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 · 2022-10-12T08:00:18Z

@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 · 2022-10-12T08:49:08Z

Please review the following PR: #3226

asbjornu · 2022-10-12T10:30:45Z

@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 · 2022-12-22T15:28:10Z

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

Your GitReleaseManager bot 📦🚀

HHobeck added 5 commits September 9, 2022 14:29

Merge branch 'support/5.x' into feature/3101

3c1e64a

Ignore some failing tests

0431671

Fix some tests

12a6ecc

asbjornu requested changes Sep 9, 2022

View reviewed changes

HHobeck added 4 commits September 10, 2022 09:41

Set back the default behavior and change the configuration builder: s…

2f89159

…et TrackMergeTarget to true for the main branch.

Integrate code review feedbacks from Asbjørn Ulsberg.

01100f2

Set back the default behavior and change the configuration builder: s…

dcec44f

…et TrackMergeTarget to true for the support branch.

Set back the default behavior and change the configuration builder: s…

eac6f01

…et TrackMergeTarget to true for the develop branch.

Fix WhenPreventIncrementOfMergedBranchVersionIsSetToFalseForDevelopCo…

8f09a85

…mmitsSinceVersionSourceShouldNotGoDownWhenMergingHotfixToDevelop test

asbjornu mentioned this pull request Sep 10, 2022

track-merge-target in branch config not working #1789

Closed

HHobeck added 2 commits September 12, 2022 12:29

Create test for discussion: Prevent decrementation of versions on the…

3c74141

… develop branch GitTools#3177

asbjornu mentioned this pull request Sep 12, 2022

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

Closed

Modify PreventDecrementationOfVersionsOnTheDevelopBranch and remove T…

937f761

…rackMergeTarget functionality.

bitbonk reviewed Sep 13, 2022

View reviewed changes

HHobeck added 5 commits September 15, 2022 14:14

Remove BranchConfigurationCalculator

ccb9f1f

Run dotnet format

55c899d

Remove comments out of the test scenarios which I have added.

8f226d4

Change PreventDecrementationOfVersionsOnTheDevelopBranchScenario and …

4e29b80

…merge to main

HHobeck added 3 commits September 15, 2022 18:50

Change PreventDecrementationOfVersionsOnTheDevelopBranchScenario

9f12b5b

Move logic from VersionStrategyBase to VersionStrategyBaseWithInherit…

3110909

…Support

Change back some configuration changes like it was before.

33dc540

HHobeck added 5 commits October 11, 2022 08:27

Make the effective configuration tag not nullable. Default value for …

6bd49cd

…tag will be present only on fallback branch configuration settings.

Change breaking changes file

4a405c6

Change property FullConfiguration to Configuration in GitVersionContext.

410c92e

Merge branch 'feature/3101' of https://github.com/HHobeck/GitVersion …

f24f235

…into feature/3101

Fix typo in breaking changes

77095ac

asbjornu requested changes Oct 11, 2022

View reviewed changes

BREAKING_CHANGES.md Show resolved Hide resolved

...ersion.Core.Tests/VersionCalculation/Strategies/ConfigNextVersionBaseVersionStrategyTests.cs Show resolved Hide resolved

HHobeck added 3 commits October 11, 2022 14:50

Update breaking changes and document some examples

32b0398

Change documentation

3971272

Remove constructing logic of the configuration in GitToolsTestingExte…

0800edf

…nsions class.

asbjornu reviewed Oct 11, 2022

View reviewed changes

BREAKING_CHANGES.md Outdated Show resolved Hide resolved

Update BREAKING_CHANGES.md

1397f03

Co-authored-by: Asbjørn Ulsberg <asbjorn@ulsberg.no>

asbjornu approved these changes Oct 11, 2022

View reviewed changes

asbjornu added this to the 6.x milestone Oct 11, 2022

asbjornu added the breaking change label Oct 11, 2022

asbjornu merged commit 92388ad into GitTools:main Oct 11, 2022

HHobeck deleted the feature/3101 branch November 27, 2022 17:10

arturcic modified the milestones: 6.x, 6.0.0-alpha.1 Dec 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

HHobeck commented Sep 9, 2022 •

edited

asbjornu left a comment

HHobeck commented Sep 10, 2022 •

edited

HHobeck commented Sep 10, 2022

asbjornu commented Sep 12, 2022

HHobeck commented Sep 15, 2022

asbjornu commented Sep 15, 2022

asbjornu left a comment

HHobeck commented Oct 11, 2022

asbjornu commented Oct 11, 2022

asbjornu left a comment

mergify bot commented Oct 11, 2022

arturcic commented Oct 11, 2022

HHobeck commented Oct 12, 2022

HHobeck commented Oct 12, 2022

asbjornu commented Oct 12, 2022

arturcic commented Dec 22, 2022

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

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

Conversation

HHobeck commented Sep 9, 2022 • edited

Description

Resolved Issues

Related Issues

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

asbjornu left a comment

Choose a reason for hiding this comment

HHobeck commented Sep 10, 2022 • edited

HHobeck commented Sep 10, 2022

asbjornu commented Sep 12, 2022

HHobeck commented Sep 15, 2022

asbjornu commented Sep 15, 2022

asbjornu left a comment

Choose a reason for hiding this comment

HHobeck commented Oct 11, 2022

asbjornu commented Oct 11, 2022

asbjornu left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 11, 2022

arturcic commented Oct 11, 2022

HHobeck commented Oct 12, 2022

HHobeck commented Oct 12, 2022

asbjornu commented Oct 12, 2022

arturcic commented Dec 22, 2022

HHobeck commented Sep 9, 2022 •

edited

HHobeck commented Sep 10, 2022 •

edited