Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
HHobeck merged 6 commits into from
Jan 28, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4966764
Add new property with name strategy (type VersionStrategies) to the c…
HHobeck Jan 25, 2024
fc9d7f7
Implement code review suggestions.
HHobeck Jan 25, 2024
b721d45
Rename ConfigNext to ConfigNextVersion and ensure that the array of v…
HHobeck Jan 26, 2024
eb80383
Integrate code review suggestions.
HHobeck Jan 26, 2024
c5512f2
Rename ConfigNextVersionVersionStrategy to ConfiguredNextVersionVersi…
HHobeck Jan 26, 2024
0bd90f5
Make code clean up and integrate code review suggestions.
HHobeck Jan 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Integrate code review suggestions.
  • Loading branch information
@HHobeck
HHobeck committed Jan 28, 2024
commit eb80383c0e730178d2283ec98cc00f1c27a79150
46 changes: 22 additions & 24 deletions docs/input/docs/reference/configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,20 +61,22 @@ strategies:
- VersionInBranchName
branches:
develop:
mode: ContinuousDelivery
tracks-release-branches: true
is-release-branch: false
is-main-branch: false
pre-release-weight: 0
label: alpha
increment: Minor
prevent-increment-of-merged-branch-version: false
track-merge-target: true
regex: ^dev(elop)?(ment)?$
source-branches: []
is-source-branch-for: []
tracks-release-branches: true
is-release-branch: false
is-main-branch: false
pre-release-weight: 0
main:
mode: ContinuousDeployment
tracks-release-branches: false
is-release-branch: false
is-main-branch: true
pre-release-weight: 55000
label: ''
increment: Patch
prevent-increment-of-merged-branch-version: true
Expand All @@ -84,11 +86,12 @@ branches:
- develop
- release
is-source-branch-for: []
tracks-release-branches: false
is-release-branch: false
is-main-branch: true
pre-release-weight: 55000
release:
tracks-release-branches: false
is-release-branch: true
is-main-branch: false
pre-release-weight: 30000
mode: ManualDeployment
label: beta
increment: None
prevent-increment-of-merged-branch-version: true
Expand All @@ -100,11 +103,8 @@ branches:
- support
- release
is-source-branch-for: []
tracks-release-branches: false
is-release-branch: true
is-main-branch: false
pre-release-weight: 30000
feature:
pre-release-weight: 30000
mode: ManualDeployment
label: '{BranchName}'
increment: Inherit
Expand All @@ -117,8 +117,8 @@ branches:
- support
- hotfix
is-source-branch-for: []
pre-release-weight: 30000
pull-request:
pre-release-weight: 30000
mode: ContinuousDelivery
label: PullRequest
increment: Inherit
Expand All @@ -132,8 +132,9 @@ branches:
- support
- hotfix
is-source-branch-for: []
pre-release-weight: 30000
hotfix:
is-release-branch: true
pre-release-weight: 30000
mode: ManualDeployment
label: beta
increment: Inherit
Expand All @@ -144,9 +145,11 @@ branches:
- support
- hotfix
is-source-branch-for: []
is-release-branch: true
pre-release-weight: 30000
support:
tracks-release-branches: false
is-release-branch: false
is-main-branch: true
pre-release-weight: 55000
label: ''
increment: Patch
prevent-increment-of-merged-branch-version: true
Expand All @@ -155,10 +158,6 @@ branches:
source-branches:
- main
is-source-branch-for: []
tracks-release-branches: false
is-release-branch: false
is-main-branch: true
pre-release-weight: 55000
unknown:
mode: ManualDeployment
label: '{BranchName}'
Expand All @@ -175,7 +174,7 @@ branches:
is-source-branch-for: []
ignore:
sha: []
mode: ManualDeployment
mode: ContinuousDelivery
label: '{BranchName}'
increment: Inherit
prevent-increment-of-merged-branch-version: false
Expand All @@ -188,7 +187,6 @@ is-source-branch-for: []
tracks-release-branches: false
is-release-branch: false
is-main-branch: false

```

The details of the available options are as follows:
Expand Down
6 changes: 2 additions & 4 deletions ...s/Configuration/ConfigurationProviderTests.CanWriteOutEffectiveConfiguration.approved.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ strategies:
- VersionInBranchName
branches:
develop:
mode: ContinuousDelivery
label: alpha
increment: Minor
prevent-increment-of-merged-branch-version: false
Expand All @@ -32,7 +31,6 @@ branches:
is-main-branch: false
pre-release-weight: 0
main:
mode: ContinuousDelivery
label: ''
increment: Patch
prevent-increment-of-merged-branch-version: true
Expand All @@ -47,6 +45,7 @@ branches:
is-main-branch: true
pre-release-weight: 55000
release:
mode: ManualDeployment
label: beta
increment: None
prevent-increment-of-merged-branch-version: true
Expand Down Expand Up @@ -105,7 +104,6 @@ branches:
is-release-branch: true
pre-release-weight: 30000
support:
mode: ContinuousDelivery
label: ''
increment: Patch
prevent-increment-of-merged-branch-version: true
Expand Down Expand Up @@ -134,7 +132,7 @@ branches:
is-source-branch-for: []
ignore:
sha: []
mode: ManualDeployment
mode: ContinuousDelivery
label: '{BranchName}'
increment: Inherit
prevent-increment-of-merged-branch-version: false
Expand Down
25 changes: 9 additions & 16 deletions src/GitVersion.Configuration/ConfigurationBuilderBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ internal abstract class ConfigurationBuilderBase<TConfigurationBuilder> : IConfi
private string? commitDateFormat;
private bool updateBuildNumber;
private SemanticVersionFormat semanticVersionFormat;
private VersionStrategies[] versionStrategies;
private VersionStrategies versionStrategy;
private Dictionary<string, string> mergeMessageFormats = new();
private readonly List<IReadOnlyDictionary<object, object?>> overrides = new();
private readonly Dictionary<string, BranchConfigurationBuilder> branchConfigurationBuilders = new();
Expand Down Expand Up @@ -200,20 +200,9 @@ public virtual TConfigurationBuilder WithSemanticVersionFormat(SemanticVersionFo
return (TConfigurationBuilder)this;
}

public virtual TConfigurationBuilder WithVersionStrategies(params VersionStrategies[] values)
=> WithVersionStrategies((IEnumerable<VersionStrategies>)values);

public virtual TConfigurationBuilder WithVersionStrategies(IEnumerable<VersionStrategies> values)
public virtual TConfigurationBuilder WithVersionStrategy(VersionStrategies value)
{
HashSet<VersionStrategies> versionStrategies = new();
foreach (var versionStrategy in values)
{
versionStrategies.AddRange(
Enum.GetValues<VersionStrategies>().Where(
element => element != VersionStrategies.None && versionStrategy.HasFlag(element))
);
}
this.versionStrategies = versionStrategies.ToArray();
this.versionStrategy = value;
return (TConfigurationBuilder)this;
}

Expand Down Expand Up @@ -339,7 +328,7 @@ public virtual TConfigurationBuilder WithConfiguration(IGitVersionConfiguration
WithCommitDateFormat(value.CommitDateFormat);
WithUpdateBuildNumber(value.UpdateBuildNumber);
WithSemanticVersionFormat(value.SemanticVersionFormat);
WithVersionStrategies(value.VersionStrategy);
WithVersionStrategy(value.VersionStrategy);
WithMergeMessageFormats(value.MergeMessageFormats);
foreach (var (name, branchConfiguration) in value.Branches)
{
Expand Down Expand Up @@ -377,6 +366,10 @@ public virtual IGitVersionConfiguration Build()
branches.Add(name, (BranchConfiguration)branchConfigurationBuilder.Build());
}

var versionStrategies = Enum.GetValues<VersionStrategies>()
.Where(element => element != VersionStrategies.None && this.versionStrategy.HasFlag(element))
.ToArray();

IGitVersionConfiguration configuration = new GitVersionConfiguration
{
AssemblyVersioningScheme = this.assemblyVersioningScheme,
Expand All @@ -396,7 +389,7 @@ public virtual IGitVersionConfiguration Build()
CommitDateFormat = this.commitDateFormat,
UpdateBuildNumber = this.updateBuildNumber,
SemanticVersionFormat = this.semanticVersionFormat,
VersionStrategies = this.versionStrategies,
VersionStrategies = versionStrategies,
Branches = branches,
MergeMessageFormats = this.mergeMessageFormats,
DeploymentMode = this.versioningMode,
Expand Down
38 changes: 12 additions & 26 deletions src/GitVersion.Configuration/SupportedWorkflows/TrunkBased/v1.yml
View file
Open in desktop
Copy link
Member

@asbjornu asbjornu Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@HHobeck HHobeck Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@asbjornu asbjornu Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@HHobeck HHobeck Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@asbjornu asbjornu Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@HHobeck HHobeck Jan 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@asbjornu asbjornu Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@HHobeck HHobeck Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Expand Up @@ -10,57 +10,43 @@ commit-date-format: yyyy-MM-dd
merge-message-formats: {}
update-build-number: true
semantic-version-format: Strict
strategies: [TrunkBased]
strategies: [TrunkBased, ConfigNextVersion]
branches:
main:
mode: ContinuousDeployment
label: ''
increment: Patch
prevent-increment-of-merged-branch-version: true
track-merge-target: false
regex: ^master$|^main$
source-branches:
- release
source-branches: []
tracks-release-branches: false
is-release-branch: false
is-main-branch: true
pre-release-weight: 55000
release:
label: beta
increment: None
prevent-increment-of-merged-branch-version: true
track-merge-target: false
regex: ^releases?[/-]
feature:
increment: Minor
regex: ^features?[/-](?<BranchName>.+)
source-branches:
- main
- release
tracks-release-branches: false
is-release-branch: true
is-main-branch: false
pre-release-weight: 30000
feature:
mode: ContinuousDelivery
label: '{BranchName}'
increment: Inherit
regex: ^features?[/-](?<BranchName>.+)
hotfix:
increment: Patch
regex: ^hotfix(es)?[/-](?<BranchName>.+)
source-branches:
- main
- release
- feature
pre-release-weight: 30000
pull-request:
mode: ContinuousDelivery
mode: ManualDeployment
label: PullRequest
increment: Inherit
label-number-pattern: '[/-](?<number>\d+)'
regex: ^(pull|pull\-requests|pr)[/-]
source-branches:
- main
- release
- feature
pre-release-weight: 30000
unknown:
mode: ContinuousDelivery
label: '{BranchName}'
mode: ManualDeployment
increment: Inherit
regex: (?<BranchName>.*)
source-branches:
Expand All @@ -70,7 +56,7 @@ branches:
- pull-request
ignore:
sha: []
mode: ContinuousDelivery
mode: ManualDeployment
label: '{BranchName}'
increment: Inherit
prevent-increment-of-merged-branch-version: false
Expand Down
2 changes: 1 addition & 1 deletion src/GitVersion.Core.Tests/IntegrationTests/ComparingTheBehaviorOfDifferentVersioningModes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ private static GitHubFlowConfigurationBuilder GetConfigurationBuilder() => GitHu
);

private static readonly IGitVersionConfiguration trunkBased = GetConfigurationBuilder()
.WithVersionStrategies(VersionStrategies.TrunkBased)
.WithVersionStrategy(VersionStrategies.TrunkBased)
.WithBranch("main", _ => _.WithIsMainBranch(true).WithDeploymentMode(DeploymentMode.ContinuousDeployment))
.WithBranch("feature", _ => _.WithIsMainBranch(false).WithDeploymentMode(DeploymentMode.ContinuousDelivery))
.Build();
Expand Down
4 changes: 2 additions & 2 deletions src/GitVersion.Core.Tests/IntegrationTests/TrunkBasedDevelopmentScenarios.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace GitVersion.Core.Tests.IntegrationTests;
public class TrunkBasedDevelopmentScenarios : TestBase
{
private static GitFlowConfigurationBuilder GetConfigurationBuilder() => GitFlowConfigurationBuilder.New
.WithVersionStrategies(VersionStrategies.TrunkBased)
.WithVersionStrategy(VersionStrategies.TrunkBased)
.WithBranch("main", builder => builder
.WithIsMainBranch(true).WithIncrement(IncrementStrategy.Patch)
.WithDeploymentMode(DeploymentMode.ContinuousDeployment)
Expand Down Expand Up @@ -511,7 +511,7 @@ public void MergingFeatureBranchThatIncrementsMinorNumberIncrementsMinorVersionO
public void VerifyIncrementConfigIsHonoured()
{
var minorIncrementConfig = GitFlowConfigurationBuilder.New
.WithVersionStrategies(VersionStrategies.TrunkBased)
.WithVersionStrategy(VersionStrategies.TrunkBased)
.WithBranch("main", builder => builder
.WithDeploymentMode(DeploymentMode.ContinuousDeployment)
.WithIncrement(IncrementStrategy.None)
Expand Down
2 changes: 1 addition & 1 deletion src/GitVersion.Core.Tests/IntegrationTests/VersionBumpingScenarios.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public void CanUseConventionalCommitsToBumpVersion(string commitMessage, string
.WithMinorVersionBumpMessage("^(feat)(\\([\\w\\s-]*\\))?:")
// For future debugging of this regex: https://regex101.com/r/oFpqxA/2
.WithPatchVersionBumpMessage("^(build|chore|ci|docs|fix|perf|refactor|revert|style|test)(\\([\\w\\s-]*\\))?:")
.WithVersionStrategies(VersionStrategies.TrunkBased)
.WithVersionStrategy(VersionStrategies.TrunkBased)
.WithBranch("main", builder => builder.WithDeploymentMode(DeploymentMode.ContinuousDeployment))
.Build();

Expand Down