GitVersion
GitVersion copied to clipboard
GitTools
The behavior of GitVersion is sometimes not like I would expected it.…
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 #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)
Related Issue
[Bug] Version not generated correct when creating a feature branch from a release branch #3101
[Bug] SemVer of a feature branch started from a release branch gets decremented #3151
[Bug] Closing pull request from hotfix to support failed to inherit Increment branch configuration #3020
[Bug] Wrong semver calculation when making a PR from a hotfix branch to main branch #3187
[Bug] Version of commit in develop merged to master changes if master is tagged #3105
~~[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)
[Bug] Closing pull request from hotfix to support failed to inherit Increment branch configuration #3020
~~[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
[Bug] Merging develop to release branch makes alpha version jump back #3154 ...
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:
- [x] My code follows the code style of this project.
- [x] My change requires a change to the documentation.
- [x] I have updated the documentation accordingly.
- [x] I have added tests to cover my changes.
- [x] All new and existing tests passed.
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)
Please do have a look. In this version the previous unit tests should be not breaking anymore.
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?
Can you please run
dotnet formatso 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
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?
The test
GitVersion.Core.Tests.DynamicRepositoryTests.FindsVersionInDynamicRepofails twice: ... Is that intentional with this change?
Actually no it was not my intention to break this feature. I think the problem is located in the TaggedCommitVersionStrategy class.
Fixed.
dotnet format ./src/ --exclude **/AddFormats/
Still it gives me this error:
PM> dotnet tool update --global dotnet-format
You can invoke the tool using the following command: dotnet-format
Tool 'dotnet-format' (version '5.1.250801') was successfully installed.
PM> dotnet-format ./src/ --exclude **/AddFormats/ --severity warn
Formatting code files in workspace 'C:\Workspaces\git_version_private\src\GitVersion.sln'.
dotnet-format : 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 ./src/ --exclude **/AddFormats/ --severity warn
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (Warnings were e...o log warnings.:String) [], RemoteException
+ FullyQualifiedErrorId : NativeCommandError
Format complete in 22675ms.
PM> dotnet-format --version
5.1.250801+4a851ea9707f87d381166c2fc2b2d4b58a10a222
Following things are pending and needs to be done in my opinion:
- Reviewing the conceptional change from @asbjornu and maybe others. (<<<--- PENDING)
- ~~I have commented some tests (unit tests) because they are relying deeply on the fact that the EffectedConfiguration will be created in the GitVersionContextFactory... This is not happened anymore because the effected configuration is within the ~~VersionStrategy implementation~~ EffectiveBranchConfigurationFinder class. The point here is we have not one effected configuration we have one or more effected configurations. That's why the Configuration property in the GitVersionContext class is marked as obsolete. We need to ensure that the test purposes are existing somewhere else or we need to re-implement this tests.~~ (DONE)
- ~~We need to refactor the caller of the property GitVersionContext.Configuration because it is obsolete (see point two)~~ (DONE)
- ~~Actually the performance needs to be checked. At this point the same logic to determine the effected configuration is called multiply times because the logic is located in the base VersionStrategyBaseWithInheritSupport class. We can think about it to move this logic to the class BaseVersionCalculator and change the interface of IVersionStrategy to be able to pass in the effective configuration.~~ (DONE)
- ~~We are calling the same IVersionStrategy implementation multiply times with different effective configurations. To have not the same result multiply times it might be a good idea to change the base version and semantic version to an immutable struct and make a distinct operation on the result list.~~ (NOT DONE; LIKE IT IS IT IS FINE WITH ME. I THINK THERE ARE POINTS WHICH ARE MORE IMPORTANT)
- ~~I have used a tuple (SemanticVersion IncrementedVersion, BaseVersion Version) for resulting both values. Better would be to give it a name and create a dedicated class for that.~~ (DONE)
- ~~NextVersionCalculator and BaseVersionCalculator needs to be fused to one class IMO~~ (DONE)
- ~~The documentation needs to be updated because I changed the default behavior from 0.1.0 to 0.0.1 version (on main) generation at the beginning. Please see the actual changes in the integration tests.~~ (DONE)
- If you ask me we should implement a logic to get the fallback and unknown branch configuration from the GitVersion.yml (at this moment it is hard coded). This gives the user the possibility to tailor GitVersion. (NICE TO HAVE and maybe a separate PR)
- ~~I would appreciate it to separate the properties which are only coming from Config and those who are coming from the branch configuration. That means we should not merge the Config with the BranchConfig to EffectiveConfiguration anymore (actually to be more precise it should be named as EffectiveBranchConfiguration). Default/Fallback values are not coming from Config anymore there are coming from configured fallback and unknown branch configuration (see point nine).~~ (LIKE IT IS IT IS FINE WITH ME. I THINK THERE ARE POINTS WHICH ARE MORE IMPORTANT)
- Try to refactor the code so we do not need this hack anymore FixTheBaseVersionSourceOfMergeMessageStrategyIfReleaseBranchWasMergedAndDeleted (NICE TO HAVE and maybe a separate PR)
- Clean up code / separate logic to dedicated functions and methods / test & refactoring
- We should think about to remove the MainlineVersionCalculator completely and let the business logic be part of the configuration. In my opinion the concept with inherited effective branch configuration is so power full that you can do everything per convention and configuration. Like the motto: This is a cooking machine... the ingredients are coming from the user. ;) (NICE TO HAVE and maybe a separate PR)
- It is totally unimportant that the current commit has been tagged or not IMO. We can do a double check actually if the result is the same or of course make the behavior configurable but each run should be deterministic. Even though the development process goes on and an previous tagged commit will be chosen the calculating result should always be the same. Otherwise something is wrong with the configuration or someone messed up the branching history. (NICE TO HAVE and maybe a separate PR)
- Mainline development mode should support pre-release tags on main. Please could someone explain me why we have so many special business logic related to mainline development? Is it maybe possible to handle it in a KIS way!? Sorry if I'm asking but is the mainline mode not just a simple workflow where the branch configuration of main will be set to IncrementStrategy.Minor instead of Patch? (NICE TO HAVE and maybe a separate PR)
- In the VariableProvider class we are not only getting the variables but also altering the semantic versions meta information. It was quite hard to get the effective configuration to that point for what reason!? What is about the separation of concern and single-responsibility principle? (NICE TO HAVE and maybe a separate PR)
So I'm done with the actual coding in the area of the core domain. I have complemented issues in the list which I have found and which needs in my opinion a serious re-thinking of the concept and/or refactoring. I marked it with NICE TO HAVE. Please take a look what is pending and not. And give me feedback feedback feedback. At least all tests which I have not uncommented are green on my PC and in my opinion I have not implemented any breaking changes (except the bug fixes). I'm so thankful that we have integration tests and it shows me again how important they are more than unit tests. ;) Cheers.
'1.' Reviewing the conceptional change from @asbjornu and maybe others. (<<<--- PENDING) '2.' ~~I have commented some tests (unit tests) because they are relying deeply on the fact that the EffectedConfiguration will be created in the GitVersionContextFactory... This is not happened anymore because the effected configuration is within the ~~VersionStrategy implementation~~ EffectiveBranchConfigurationFinder class. The point here is we have not one effected configuration we have one or more effected configurations. That's why the Configuration property in the GitVersionContext class is marked as obsolete. We need to ensure that the test purposes are existing somewhere else or we need to re-implement this tests.~~ (DONE) [...] '8.' ~~The documentation needs to be updated because I changed the default behavior from 0.1.0 to 0.0.1 version (on main) generation at the beginning. Please see the actual changes in the integration tests.~~(DONE)
Taking in consideration that many changes I suggest moving this to main branch, as the support is meant to fix with small changes
Taking in consideration that many changes I suggest moving this to main branch, as the support is meant to fix with small changes
Hi Artur. Thank you very much for joining. Yes I agree. I'm not certain which target branch I have to choose for that PR. Initial I took the source code version which was tagged with the number 5.10.3 and started the work. If you like I can create a PR to an other branch (master or develop??). I was surprised not to see any develop branch but maybe you are using a different workflow than git flow. May I ask you which target version you are preferring?
@HHobeck, retarget to main, it is currently being primed for v6 where I agree this will need to go.
I would like to highlight the importance of this point:
'15.' Mainline development mode should support pre-release tags on main. Please could someone explain me why we have so many special business logic related to mainline development? Is it maybe possible to handle it in a KIS way!? Sorry if I'm asking but is the mainline mode not just a simple workflow where the branch configuration of main will be set to IncrementStrategy.Minor instead of Patch? (NICE TO HAVE and maybe a separate PR)
~~Because of my changes the pre-release tag will be calculated and set in the NextVersionCalculator before the executing step of MainlineVersionCalculator::FindMainlineModeVersion happens. This is necessary because the pre-release tag is dependent on the effective branch configuration which is only available before. So far so good... but especially for the main line versioning mode this is problematic. Therefor I needed to disable the pre release tag persistence and cache it for that part:~~
public SemanticVersion FindMainlineModeVersion(BaseVersion baseVersion)
{
using (this.log.IndentLog("Using mainline development mode to calculate current version"))
{
var preReleaseTag = baseVersion.SemanticVersion.PreReleaseTag;
if (preReleaseTag != null)
{
// TODO: This needs to be refactored. It's a hack because mainline development doesn't support preReleaseTag.
preReleaseTag.DisableBecauseTheMainLineModeDoesntSupportPreReleaseTags();
//throw new NotSupportedException("Mainline development mode doesn't yet support pre-release tags on main");
}
[...] // business logic magic happens here
baseVersion.SemanticVersion.PreReleaseTag = preReleaseTag;
if (preReleaseTag != null)
{
// TODO: This needs to be refactored. It's a hack because mainline development doesn't support preReleaseTag.
preReleaseTag.EnableBecauseTheMainLineModeDoesntSupportPreReleaseTags();
mainlineVersion.PreReleaseTag = preReleaseTag;
}
return mainlineVersion;
}
}
~~@asbjornu I saw you are involved developing this part and would kindly ask you to maybe help me to solve this in a better way.~~
This is obsolete because I have moved back the logic to determine the pre-release tag to a location which will be exectued after FindMainlineModeVersion method.
@HHobeck, retarget to
main, it is currently being primed for v6 where I agree this will need to go.
May I ask you do you have a release plan? What is the goal of the next release and who is involved in it? Sorry for the dumb questions. Are they any features which are related maybe with my bug fixes?
HHobeck requested review from asbjornu and removed request for arturcic
That's not what I want. I want you both as a reviewer. Sorry Artur that I removed you.
Dear community.
I finished my work and all related TODOs from my side are done. This PR should solve the related bug fixes and maybe more. I have pointed out some conceptional improvements and proposals for some refactoring.
To summarize my experience I would say: GitVersion has allot of very nice features and is well elaborated. The integration tests are very good understandable and the code coverage is fantastic. Even though the CI pipelines are very good. Thank you for this work because without it I haven't been able to do the bug fix. Unfortunately I can see that the code violates in some areas against patterns like clean code programming, separation of concern, keep it simple or single-responsibility principle. This makes it very hard for improvements and bug fixing and the community should think about it to invest some effort on that. But the key point is that GitVersion has to much special business logic in code present. The motto this is a cooking machine the ingredients are coming from the user should considered even more. If you think about it to master the convention and configuration pattern in GitVersion then you have elegant and understandable (slim) code.
I have invested totally two weeks (80 hours) of my time for this bug fix (in my opinion I have fixed architecture gaps too). Actually I didn't foreseen this and are still unhappy about the support/help of the community which was in my opinion not enough. I know we are all busy with other stuff and this is not our prime work but if someone is going to invest 80 hours then feed back is highly appreciated. At the end of the day the only contact I have had was with @asbjornu. I want to thank him for his input.
Maybe the patron of the project should think about it to get some sponsors to do what a patron do to get the open source project still a life. I can think about it that big companies are using this tool on daily basis and are happy to have it. We all know what happened with JLOG.
Please go ahead and merge this PR to main. Thank you very much.
@HHobeck, retarget to
main, it is currently being primed for v6 where I agree this will need to go.May I ask you do you have a release plan? What is the goal of the next release and who is involved in it? Sorry for the dumb questions. Are they any features which are related maybe with my bug fixes?
The plans for v6.0 is to remove some obsolete code, and sunset .net 5.0, .net core 3.1 and add .net 7.0. There are also couple of other changes, you can check the 6.x milestone
HHobeck requested review from asbjornu and removed request for arturcic
That's not what I want. I want you both as a reviewer. Sorry Artur that I removed you.
I added me back, there are quite big changes in the code, maybe it's a good ideea to split it in several PRs that can be easier code reviewed and gradually merged. Too many files have changed and it's a bit hard to have the entire picture. I really want to check the way this PR goes, as to understand how this will fit in the v7.0 cli refactoring
I added me back, there are quite big changes in the code, maybe it's a good ideea to split it in several PRs that can be easier code reviewed and gradually merged. Too many files have changed and it's a bit hard to have the entire picture. I really want to check the way this PR goes, as to understand how this will fit in the v7.0 cli refactoring
Normally I would agree. But I focused only on this issue here https://github.com/GitTools/GitVersion/issues/3101 and also only changed what I have proposed in this ticket (except maybe the fallback version which is a minor change). Every things else even the fix of the other bugs are coming for free. I even didn't do anything for other ticket. That shows me that my work goes in the right direction. The point is that we have to change the usage of GitVersionContext.Configuration and this is really used every where. If you have an idea how to separate this please let me know. This PR is conceptional only replacing the mechanism to determine the effected branch configuration not more and not less.
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 #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)
Please notice that everything which was not related to the effected branch configuration I have skipped.
Following things are pending and needs to be done in my opinion:
- Reviewing the conceptional change from @asbjornu and maybe others. (<<<--- PENDING)
- ~~I have commented some tests (unit tests) because they are relying deeply on the fact that the EffectedConfiguration will be created in the GitVersionContextFactory... This is not happened anymore because the effected configuration is within the ~~VersionStrategy implementation~~ EffectiveBranchConfigurationFinder class. The point here is we have not one effected configuration we have one or more effected configurations. That's why the Configuration property in the GitVersionContext class is marked as obsolete. We need to ensure that the test purposes are existing somewhere else or we need to re-implement this tests.~~ (DONE)
- ~~We need to refactor the caller of the property GitVersionContext.Configuration because it is obsolete (see point two)~~ (DONE)
- ~~Actually the performance needs to be checked. At this point the same logic to determine the effected configuration is called multiply times because the logic is located in the base VersionStrategyBaseWithInheritSupport class. We can think about it to move this logic to the class BaseVersionCalculator and change the interface of IVersionStrategy to be able to pass in the effective configuration.~~ (DONE)
- ~~We are calling the same IVersionStrategy implementation multiply times with different effective configurations. To have not the same result multiply times it might be a good idea to change the base version and semantic version to an immutable struct and make a distinct operation on the result list.~~ (NOT DONE; LIKE IT IS IT IS FINE WITH ME. I THINK THERE ARE POINTS WHICH ARE MORE IMPORTANT)
- ~~I have used a tuple (SemanticVersion IncrementedVersion, BaseVersion Version) for resulting both values. Better would be to give it a name and create a dedicated class for that.~~ (DONE)
- ~~NextVersionCalculator and BaseVersionCalculator needs to be fused to one class IMO~~ (DONE)
- ~~The documentation needs to be updated because I changed the default behavior from 0.1.0 to 0.0.1 version (on main) generation at the beginning. Please see the actual changes in the integration tests.~~ (DONE)
- If you ask me we should implement a logic to get the fallback and unknown branch configuration from the GitVersion.yml (at this moment it is hard coded). This gives the user the possibility to tailor GitVersion. (NICE TO HAVE and maybe a separate PR)
- ~~I would appreciate it to separate the properties which are only coming from Config and those who are coming from the branch configuration. That means we should not merge the Config with the BranchConfig to EffectiveConfiguration anymore (actually to be more precise it should be named as EffectiveBranchConfiguration). Default/Fallback values are not coming from Config anymore there are coming from configured fallback and unknown branch configuration (see point nine).~~ (LIKE IT IS IT IS FINE WITH ME. I THINK THERE ARE POINTS WHICH ARE MORE IMPORTANT)
- Try to refactor the code so we do not need this hack anymore FixTheBaseVersionSourceOfMergeMessageStrategyIfReleaseBranchWasMergedAndDeleted (NICE TO HAVE and maybe a separate PR)
- Clean up code / separate logic to dedicated functions and methods / test & refactoring
- We should think about to remove the MainlineVersionCalculator completely and let the business logic be part of the configuration. In my opinion the concept with inherited effective branch configuration is so power full that you can do everything per convention and configuration. Like the motto: This is a cooking machine... the ingredients are coming from the user. ;) (NICE TO HAVE and maybe a separate PR)
- It is totally unimportant that the current commit has been tagged or not IMO. We can do a double check actually if the result is the same or of course make the behavior configurable but each run should be deterministic. Even though the development process goes on and an previous tagged commit will be chosen the calculating result should always be the same. Otherwise something is wrong with the configuration or someone messed up the branching history. (NICE TO HAVE and maybe a separate PR)
- Mainline development mode should support pre-release tags on main. Please could someone explain me why we have so many special business logic related to mainline development? Is it maybe possible to handle it in a KIS way!? Sorry if I'm asking but is the mainline mode not just a simple workflow where the branch configuration of main will be set to IncrementStrategy.Minor instead of Patch? (NICE TO HAVE and maybe a separate PR)
- In the VariableProvider class we are not only getting the variables but also altering the semantic versions meta information. It was quite hard to get the effective configuration to that point for what reason!? What is about the separation of concern and single-responsibility principle? (NICE TO HAVE and maybe a separate PR)
Please also see this. I have documented everything what I did. You cannot be serious asking me to separate this.
Actually I think because of the rebase from the support branch to my feature branch and then creating the PR to main is maybe the reason why this PR has more changes then expected. I would say that the changes from support to main will be merged immediately when the changes happened on support. I'm wrong?
Please also see this. I have documented everything what I did. You cannot be serious asking me to separate this.
In that case the review might take longer for us.
Let me outline some key points to give you a better understanding about my actual changes:
- Getting the base versions in the classes who implements the IVersionStrategy interface are highly dependent on the effected branch configuration. (like before)
- We have not one effected branch configuration we have one or more effected branch configurations. Think about it if you have more than one source branch (parents). (This is a key fact to solve some stupid edge cases)
- The current branch (child) inherits from source branch (parent branch) if the IncrementStrategy is set to Inherit.
- This is highly recursive and can be configured via the configuration builder (or even with the yaml file for custom workflows)
- Because of this mechanism we are totally generic and should in future place the hole special business logic in the configuration file. The core domain of GitVersion is not located in the MainVersionCalculator (with the massive if then else logic) or in the NextVersionCalculator (which also have massive logic determining the pre release tag which is out of scope). It's within the IVersionStrategy classes (keep this in mind) and the way like the configuration has been done.
- If point 5 is not sufficient in future for any workflows then we need not to place some special logic. We have to think about the use case and create a new IVersionStrategy implementation and make it maybe configurable.
- I have introduced the concept of fallback and unknown branch configuration. This is also quite important to understand the different. Please take a look into the EffectiveBranchConfigurationFinderTests class for more detail. The unknown branch configuration like the fallback branch configuration is at this moment hard coded and needs to be moved in the configuration like any other branches configurations.
- The fallback version strategy returns always 0.0.0 and is flagged with should increment true. This gives you the version 0.1.0 on develop branch (IncrementStrategy.Minor) and 0.0.1 on main branch (IncremetnStrategy.Patch).
In advance I would suggest to read #3177 and #3101 to see some user stories.
Let me outline some key points to give you a better understanding about my actual changes:
- Getting the base versions in the classes who implements the IVersionStrategy interface are highly dependent on the effected branch configuration. (like before)
- We have not one effected branch configuration we have one or more branch configurations. Think about it if you have more than one source branch (parents). (This is a key fact to solve some stupid edge cases)
- The current branch (child) inherits from source branch (parent branch) if the IncrementStrategy is set to Inherit.
- This is highly recursive and can be configured via the configuration build (or even with the yaml file for custom workflows)
- Because of this mechanism we are totally generic and should in future place the hole special business logic in the configuration file. The core domain of GitVersion is not located in the MainVersionCalculator (with the massive if then else logic) or in the NextVersionCalculator (which also have massive logic determining the pre release tag which is out of scope). It's within the IVersionStrategy classes (keep this in mind) and the way like the configuration has been done.
- If point 5 is not sufficient in future for any workflows then we need not to place some special logic. We have to think about the use case and create a new IVersionStrategy implementation and make it maybe configurable.
- I have introduced the concept of fallback and unknown branch configuration. This is also quite important to understand the different. Please take a look into the EffectiveBranchConfigurationFinderTests class for more detail. The unknown branch configuration like the fallback branch configuration is at this moment hard coded and needs to be moved in the configuration like any other branches configurations.
- The fallback version strategy returns always 0.0.0 and is flagged with should increment true. This gives you the version 0.1.0 on develop branch (IncrementStrategy.Minor) and 0.0.1 on main branch (IncremetnStrategy.Patch).
In advance I would suggest to read #3177 and #3101 to see some user stories.
Hope to find some time to review it soon
First, let me say that I am very thankful and appreciate all the hours you have poured into this, @HHobeck. 🙏🏼 It is impossible to maintain a project like GitVersion without help from contributors like yourself. ❤️
Reviewing the conceptional change from @asbjornu and maybe others. (<<<--- PENDING)
As described, I think the concept sounds like a great improvement compared to the chaos we have now. However, I have not found time to review every line of code that you have changed, so I can't confess under oath that the code does exactly what you describe and that I'm happy with every single change. I'm in the middle of a big crunch at work these days so please give time to review.
Especially when there are major changes to the tests, it is going to take a lot more time and scrutiny to review the changes. If you are able to break the PR up into parts that don't change or break any existing tests, I can promise a swift review and merge into both v5 and v6.
The point is that we have to change the usage of GitVersionContext.Configuration and this is really used every where. If you have an idea how to separate this please let me know.
Perhaps it would be possible to do the GitVersionContext.Configuration refactoring in a separate PR that does not touch any tests so we know that it is a non-breaking change? That way we can merge it into both v5 and v6. Then you can go on with the breaking changes in a separate PR?
First, let me say that I am very thankful and appreciate all the hours you have poured into this, @HHobeck. 🙏🏼 It is impossible to maintain a project like GitVersion without help from contributors like yourself. ❤️
Thank you very much it is a pleasure for me to give some innovation back to this project. My customer uses this tool and gave me time for this bug fix. I think he is happy to read this.
As described, I think the concept sounds like a great improvement compared to the chaos we have now. However, I have not found time to review every line of code that you have changed, so I can't confess under oath that the code does exactly what you describe and that I'm happy with every single change. I'm in the middle of a big crunch at work these days so please give time to review.
Okay no problem take the time you need and if you have questions or remarks then I'm going to give you this information you need or even integrate the review in this PR.
Especially when there are major changes to the tests, it is going to take a lot more time and scrutiny to review the changes. If you are able to break the PR up into parts that don't change or break any existing tests, I can promise a swift review and merge into both v5 and v6.
Actually this PR has no breaking changes (from the feature point of view) and doesn't break any tests (except those tests who are false positive ;)). The only thing which I have changed is a implementation detail. If you like you can take this and push it to v5 without any doubts (of course after you have reviewed and tested it). The code coverage and the integration tests are so powerful and are the proof that it's working. BTW I have created also massive new integration and unit tests to ensure the functionality.
Perhaps it would be possible to do the GitVersionContext.Configuration refactoring in a separate PR that does not touch any tests so we know that it is a non-breaking change? That way we can merge it into both v5 and v6. Then you can go on with the breaking changes in a separate PR?
If you just refactor the GitVersionContext.Configuration with the goal not to change code of any unit tests (please keep in mind that not the test aspect has been broken) then you are simple refactoring without any improvements. Of course it makes it easer to review but it brings no benefit for the end user. Especially it is a waste of time if you are already done with the actual work like in this PR in my opinion. The only argument to bring this to v5 will be to have an improvement or bugfix. I would highly suggest to think about it and maybe bring this to v5 because at the end this PR is just a bug fix and removes some conceptional deficits from GitVersion.
In addition I want to say I'm also understand it if you bring this to v6 because of the massive changes.
Thank you very much it is a pleasure for me to give some innovation back to this project. My customer uses this tool and gave me time for this bug fix. I think he is happy to read this.
Awesome! 😊 🙏🏼
Actually this PR has no breaking changes (from the feature point of view) and doesn't break any tests (except those tests who are false positive ;)).
It's these "false positives" I'm a bit wary about. We don't know what expectations current users of GitVersion has with regards to existing functionality. Even though the current behaviour can be considered a bug, it's a bug current consumers of GitVersion may depend on in their workflows. Altering that expectancy is a breaking change, regardless of whether it's a bugfix or not.
BTW I have created also massive new integration and unit tests to ensure the functionality.
Yes, I've noticed and don't have questions wrt. the code you've added and its test coverage. I feel confident about merging it in a vacuum. But GitVersion exists in an ecosystem of existing consumers with existing expectations and workflows. I don't want to cause any breakage for them.
If you just refactor the GitVersionContext.Configuration with the goal not to change code of any unit tests (please keep in mind that not the test aspect has been broken) then you are simple refactoring without any improvements.
Correct. And that's actually perfect. No change to the consumers means no breaking changes, which is good. What it does give us, though, is a much simpler review process because such a non-breaking refactoring can be reviewed and merged in isolation. When the refactoring is merged, all PRs building on that work is also going to be simpler to review since they contain less code, providing more focus on the altered functionality and added features, meaning they are also going to be reviewed and merged more quickly.
Especially it is a waste of time if you are already done with the actual work like in this PR in my opinion.
It does involve more work, but I can promise to not merge any other PRs before your refactoring PR is merged so you won't experience any conflicts other than your own when you rebase and condense this PR on top of the refactoring.
The only argument to bring this to v5 will be to have an improvement or bugfix.
It does make maintenance a lot easier to have v5 and v6 as close and identical as possible.
I would highly suggest to think about it and maybe bring this to v5 because at the end this PR is just a bug fix and removes some conceptional deficits from GitVersion.
I understand, but it's going to be a lot easier to assess that when the PR is made smaller due to the refactoring being done in a separate PR that is merged into both v5 and v6.
Actually this PR has no breaking changes (from the feature point of view) and doesn't break any tests (except those tests who are false positive ;)).
It's these "false positives" I'm a bit wary about. We don't know what expectations current users of GitVersion has with regards to existing functionality. Even though the current behaviour can be considered a bug, it's a bug current consumers of GitVersion may depend on in their workflows. Altering that expectancy is a breaking change, regardless of whether it's a bugfix or not.
BTW I have created also massive new integration and unit tests to ensure the functionality.
Yes, I've noticed and don't have questions wrt. the code you've added and its test coverage. I feel confident about merging it in a vacuum. But GitVersion exists in an ecosystem of existing consumers with existing expectations and workflows. I don't want to cause any breakage for them.
Of course that's why you and other persons in the community should review it. Maybe I have understood some scenarios wrong or introduced a bug. But if I view the so called false positive tests then most of them are just having a different pre-release number (the semantic versions are still the same). And if I quote your statement "GitVersion is used to determine the semantic version number only" then this is totally fine even for a minor version increment (v5). I mean we need of course a valid pre-release number. But for the usage the important points are that on the one hand the numbers are going still up (no flipping within a dedicated version of GitVersion) and on the other hand the generation of the version inclusive pre-release number followed some rules which can be explained and are plausible.
What are the rules in the current version of GitVersion in the following example:
[Test]
public void GivenARepositoryWithCommitsButNoTagsWithDetachedHeadVersionShouldBe01()
{
using var fixture = new EmptyRepositoryFixture();
// Given
fixture.Repository.MakeACommit(); // one
fixture.Repository.MakeACommit(); // two
fixture.Repository.MakeACommit(); // three
var commit = fixture.Repository.Head.Tip;
fixture.Repository.MakeACommit();
Commands.Checkout(fixture.Repository, commit);
// When
fixture.AssertFullSemver("0.0.1+3", onlyTrackedBranches: false); // current version gives 0.1.0+2
}
Another kind of false positive test is:
[Test]
public void BranchWithoutMergeBaseMainlineBranchIsFound()
{
var currentConfig = new Config { VersioningMode = VersioningMode.Mainline, AssemblyFileVersioningScheme = AssemblyFileVersioningScheme.MajorMinorPatchTag };
using var fixture = new EmptyRepositoryFixture();
fixture.Repository.MakeACommit();
fixture.AssertFullSemver("0.0.1", currentConfig);
Commands.Checkout(fixture.Repository, fixture.Repository.CreateBranch("master"));
fixture.Repository.Branches.Remove(fixture.Repository.Branches["main"]);
fixture.AssertFullSemver("0.0.1", currentConfig);
fixture.Repository.MakeCommits(2);
fixture.AssertFullSemver("0.0.3", currentConfig);
Commands.Checkout(fixture.Repository, fixture.Repository.CreateBranch("issue-branch"));
fixture.Repository.MakeACommit();
fixture.AssertFullSemver("0.0.4-issue-branch.1", currentConfig); // current version gives 0.1.3-issue-branch.1
}
Why is the minor number changed in the above example at the beginning? The configuration says IncrementStrategy.Patch. Even though the fallback number is 0.1.0 I would expect 0.1.1. How many ticket you have already answered trying to explain this crude behavior? ;)
At the end I would like to say that this questions you are asking are quite valid and of course they need to be asked and answered. But I would highly appreciate it to go ahead and leave the discussion on the meta level. Lets discuss concrete examples!
Or this example is a good one to highlight. No one can seriously expect the version 1.1.0 on the develop branch:
[Test]
public void InheritVersionFromReleaseBranch()
{
using var fixture = new EmptyRepositoryFixture();
fixture.MakeATaggedCommit("1.0.0");
fixture.BranchTo("develop");
fixture.MakeACommit();
fixture.BranchTo("release/2.0.0");
fixture.MakeACommit();
fixture.MakeACommit();
fixture.Checkout("develop");
fixture.AssertFullSemver("2.1.0-alpha.0"); // current version gives 1.1.0-alpha.1
fixture.MakeACommit();
fixture.AssertFullSemver("2.1.0-alpha.1");
fixture.MergeNoFF("release/2.0.0");
fixture.AssertFullSemver("2.1.0-alpha.4");
fixture.BranchTo("feature/MyFeature");
fixture.MakeACommit();
fixture.AssertFullSemver("2.1.0-MyFeature.1+5");
}