Replace the version mode Mainline in 6.x (Part IV)

[HHobeck]

Description

Create TrunkBasedVersionStrategy implementation and replace the version mode Mainline in 6.x.

Close #3877
Close #1839
Close #3308
Close #3570
Close #3656
Cloes #3644

Motivation and Context

For more detail please see the following ADR:

• Replace the version mode Mainline in 6.x #3601

Next steps are:

• Create TrunkBasedVersionStrategy implementation (done)
• Create integration tests for TrunkBasedVersionStrategy (I skip this because of the limit time of reviewer)
• Remove Mainline mode (done)
• Replace VersioningMode with DeploymentMode (done)
• Add new property with name strategies (type VersionStrategies[]) to the configuration root level <<-- Part of this PR
• Create documentation for TrunkBased workflow and add breaking changes notes <<-- Part of this PR

How Has This Been Tested?

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[arturcic]

@HHobeck please move the step with unit tests at the end

[asbjornu]

I'm a bit unsure about the name TrackReleaseBranches. We should be consistent with whether we use a verb (Track) in the enum values and whether we use singular (Commit) or plural (Branches) form.

[HHobeck]

I'm open to any renaming as long we are renaming the underlying classes who implements the strategy as well.

[asbjornu]

Is TrackReleaseBranches really a versioning strategy, though? Isn't it more like a branch configuration?

[HHobeck]

At the moment it is a version strategy yes. What is your suggestion!? To delete it? I thought this is the concept of our core domain to have provider based (strategy based) approach to determine the next version. And you can control whether or not it is called using the strategies property on the configuration root level.

[asbjornu]

Yes, I like the configurable strategy approach. But like next-version, tracks-release-branches is a configuration property. It's strange to have tracks-release-branches configured for a branch, but ignored because TrackReleaseBranches isn't configured.

[HHobeck]

Also here I would say like I already did previously: You need to think of the property like a characteristic of a branch or a characteristic of the repository which is generally isolated of the actual used strategies. If someone decide e.g. not to use the NextVersionVersionStrategy but define next-version then it would be fine to ignore it (Actually if not it would be a bug).

And your second example is even easy to argument. The track-release-braches property will be used in the TrackReleaseBranchesVersionStrategy and TrunkBaseVersionStrategy. If you decide to use TrunkBaseVersionStrategy you should not execute the TrackReleaseBranchesVersionStrategy because it gives you maybe a wrong behavior for the TrunkBased (Mainline) workflow.

Does it make sense?

[asbjornu]

I didn't think of the possibility of tracks-release-branches being used by more than one strategy. That's not the case for next-version, is it?

[HHobeck]

Sorry I was mistaken. I have mixed tracks-release-branches with track-merge-message in my mind. You are right tracks-release-branches will be used in TrackReleaseBranchesVersionStrategy and next-version in ConfiguredNextVersionVersionStrategy only. But that can change. Who knows!?

[asbjornu]

Why is the folder called SupportedWorkflows when it's associated with a configuration property called version-strategy? Either we should rename the YAML property to workflow or we should rename the folder to Strategies.

[HHobeck]

Hmm correct we have a clash with the term TrunkBased here... What is your suggestion? In the folder SupportedWorkflows we are talking from workflow where in the strategy we are talking from strategy which is something different.

We could define e.g. on the workflow template SupportedWorkflows/TrunkBased/v1.yml to use the strategy TrunkBased,NextVersion to support the next-version configuration as well if it is applicable. Thus there is no one to one relation from workflow to strategy. It's up to you how you would define it.

Edit:
TLDR: The folder called SupportedWorkflows is not associated with a configuration property called version-strategy.

[asbjornu]

Yeah, the overlap between strategy and workflow is a bit confusing. Would it be possible to remove strategy completely from configuration and only allow workflow to be configured? Perhaps not in v6, but in v7?

[HHobeck]

Actually I saw a lot of issues where the user wants to have it in control which strategies are used or not. The workflows are static and integrated in the assembly. If you delete the strategies then how would the user change the behavior?

Another point here is that we are not defining any hard coded business logic around the workflow. The workflow is just a template which will be used as a base configuration. If the user like he can take the configuration directly with the same effect.

Edit:
What do you think about to name the strategy instead of TrunkBasedStrategy MainlineStrategy?

[asbjornu]

Good point wrt. allowing configurability on individual strategies. So we probably want workflow and strategies to be mutually exclusive, no? If not, how would workflow: TrunkBased with strategies: [TaggedCommit] work?

[HHobeck]

Good point wrt. allowing configurability on individual strategies. So we probably want workflow and strategies to be mutually exclusive, no?

The VersionStrategies pattern is the main mechanism to control which strategy will be used to determine the next version. This can be of course changed by the user. Like every time if the user changes the configuration and customizes their own workflow he needs to ensure the correct behavior as well. Because it is hard work to build a workflow we as contributors are providing a supported workflow templates (at this moment GitHubFlow, GitFlow and TrunkBased and maybe more in future) to help the user get there build pipe up and running. And for this workflows we give a support.

Comming to your question: You only can answer this question if it is working or not if you know what workflow the user wants to implement. Because we don't know what the workflow might be we cannot do any assumptions about it. In best case it has just a performance impact in worst case the user gets a version number he would not expect.

Edit:

If not, how would workflow: TrunkBased with strategies: [TaggedCommit] work?

The scenario you are describing here would be a customization of the TrunkBased workflow. This setting would of course break everything. But the user is responsible to ensure the behavior by himself when customizing. Changing configuration is only for advanced user who understanding what they are doing. Again the used workflow template is only a base template which can be overwritten. For the version calculation GitVersion doesn't care if you use a base workspace and customize it afterwards or provide a full configuration from scratch without the usage of a base workflow. It is equivalent and the result is the same.

[asbjornu]

Right, I just want us to be on the same page here and offer the best possible developer experience when interacting with these new configuration properties. That's why I thought strategies and workflow could be made mutually exclusive; you either use one or the other. GitVersion would simply return an error if both properties are present in GitVersion.yml.

I think that's going to lead to fewer problems (i.e. fewer filed issues on GitHub) than allowing developers to mix workflow and strategies. From my years of experience maintaining API surfaces like this, it's wise to start out restrictive and increase flexibility over time. If we start out flexible, allowing strategies and workflow to be mixed, it's going to be much harder adding restrictions on their usage in the future.

If we are to allow strategies and workflow simultaneously, we need to figure out whether strategies are additive or reductive to the ones implicitly added by workflow, for instance. I say we just avoid having to answer that question by not allowing it.

[HHobeck]

I'm actually not a big fan of being a guardian for the user and protecting them for themselves. I would say to document it and make a attention sign that only advanced user who knows what they are doing should change this field.

Anyway if we do it like you mentioned how would the users of the following issues use this feature?

• Add configuration option to disable branch name version calculation #3877
• Configurable next-version strategies and mode #1839

My expectation would be if you want to skip the MergeMessageVersionStrategy you need to change the configuration as following:

workflow: GitFlow/v1
strategies: [ConfiguredNextVersion, TaggedCommit, TrackReleaseBranches, VersionInBranchName]

and if you want to skip VersionInBranchNameVersionStrategy you write:

workflow: GitFlow/v1
strategies: [ConfiguredNextVersion, MergeMessage, TaggedCommit, TrackReleaseBranches]

That means the strategies array is not additive.

[asbjornu]

Is TrackReleaseBranches really a versioning strategy, though? Isn't it more like a branch configuration?

[asbjornu]

Relevant to this PR: #1839.

[asbjornu]

I think we are close now. The YAML array vs. flags enum thing can probably be fixed in a separate PR where we:

1. Remove all [Json…] attributes from GitVersionConfiguration.
2. Add a new private class inside GitVersion.Configuration.ConfigurationSerializer called GitVersionConfigurationDto that is decorated with deserialization attributes and tailored towards simple de/serialization from/to YAML.
3. Change the implementation of GitVersion.Configuration.ConfigurationSerializer.Read() to deserialize to GitVersionConfigurationDto and then map from the DTO to the GitVersionConfiguration with manual handling of the array to Flags enum conversion.
4. Change the implementation of GitVersion.Configuration.ConfigurationSerializer.Write() to first map from the received IGitVersionConfiguration object to GitVersionConfigurationDto and then serialize the DTO to disk.

[HHobeck]

If you not have any additional remarks please merge this PR.

[asbjornu]

Great work, @HHobeck!

[asbjornu]

It seems like the class name here contains BaseVersion for no particular reason?

Suggested change

	public class ConfiguredNextVersionBaseVersionStrategyTests : TestBase
	public class ConfiguredNextVersionStrategyTests : TestBase

[HHobeck]

done

[HHobeck]

Actually what I like to discuss with you @asbjornu: With this PR the pump messages +semver: none is not usable anymore. What are we doing with the branch related property no-bump-message?

[mergify]

Thank you @HHobeck for your contribution!