Replace the version mode Mainline in 6.x (Part IV) #3883
…onStrategy
- Loading branch information
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Why is the folder called
Contributor
Author
There was a problem hiding this comment. Hmm correct we have a clash with the term TrunkBased here... What is your suggestion? In the folder We could define e.g. on the workflow template Edit:
Member
There was a problem hiding this comment. Yeah, the overlap between
Contributor
Author
There was a problem hiding this comment. 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:
Member
There was a problem hiding this comment. Good point wrt. allowing configurability on individual strategies. So we probably want
Contributor
Author
There was a problem hiding this comment.
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:
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.
Member
There was a problem hiding this comment. 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 I think that's going to lead to fewer problems (i.e. fewer filed issues on GitHub) than allowing developers to mix If we are to allow
Contributor
Author
There was a problem hiding this comment. 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?
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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ internal static class ConfigurationConstants | |
| public const AssemblyFileVersioningScheme DefaultAssemblyFileVersioningScheme = AssemblyFileVersioningScheme.MajorMinorPatch; | ||
| public const SemanticVersionFormat DefaultSemanticVersionFormat = SemanticVersionFormat.Strict; | ||
| public static readonly VersionStrategies[] DefaultVersionStrategies = [ | ||
| VersionStrategies.ConfiguredNextVersion, | ||
| VersionStrategies.MergeMessage, | ||
| VersionStrategies.TaggedCommit, | ||
| VersionStrategies.TrackReleaseBranches, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -724,7 +724,7 @@ GitVersion.VersionCalculation.VersionCalculationModule | |
| GitVersion.VersionCalculation.VersionCalculationModule.RegisterTypes(Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> void | ||
| GitVersion.VersionCalculation.VersionCalculationModule.VersionCalculationModule() -> void | ||
| GitVersion.VersionCalculation.VersionStrategies | ||
| GitVersion.VersionCalculation.VersionStrategies.ConfiguredNextVersion = 1 -> GitVersion.VersionCalculation.VersionStrategies | ||
| GitVersion.VersionCalculation.VersionStrategies.MergeMessage = 2 -> GitVersion.VersionCalculation.VersionStrategies | ||
| GitVersion.VersionCalculation.VersionStrategies.None = 0 -> GitVersion.VersionCalculation.VersionStrategies | ||
| GitVersion.VersionCalculation.VersionStrategies.TaggedCommit = 4 -> GitVersion.VersionCalculation.VersionStrategies | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,11 +8,11 @@ namespace GitVersion.VersionCalculation; | |
| /// BaseVersionSource is null. | ||
| /// Does not increment. | ||
| /// </summary> | ||
| internal class ConfiguredNextVersionVersionStrategy(Lazy<GitVersionContext> versionContext) : VersionStrategyBase(versionContext) | ||
| { | ||
| public override IEnumerable<BaseVersion> GetBaseVersions(EffectiveBranchConfiguration configuration) | ||
| { | ||
| if (!Context.Configuration.VersionStrategy.HasFlag(VersionStrategies.ConfiguredNextVersion)) | ||
| yield break; | ||
|
|
||
| var contextConfiguration = Context.Configuration; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,10 +4,10 @@ namespace GitVersion.VersionCalculation; | |
| public enum VersionStrategies | ||
| { | ||
| None = 0, | ||
| ConfiguredNextVersion = 1, | ||
| MergeMessage = 2, | ||
| TaggedCommit = 4, | ||
| TrackReleaseBranches = 8, | ||
| VersionInBranchName = 16, | ||
| TrunkBased = 32 | ||
| } | ||
There was a problem hiding this 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 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.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to any renaming as long we are renaming the underlying classes who implements the strategy as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
TrackReleaseBranchesreally a versioning strategy, though? Isn't it more like a branch configuration?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like the configurable strategy approach. But like
next-version,tracks-release-branchesis a configuration property. It's strange to havetracks-release-branchesconfigured for a branch, but ignored becauseTrackReleaseBranchesisn't configured.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think of the possibility of
tracks-release-branchesbeing used by more than one strategy. That's not the case fornext-version, is it?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I was mistaken. I have mixed
tracks-release-brancheswithtrack-merge-messagein my mind. You are righttracks-release-brancheswill be used inTrackReleaseBranchesVersionStrategyandnext-versioninConfiguredNextVersionVersionStrategyonly. But that can change. Who knows!?