Create new fallback and unknown section in GitVersionConfiguration an… #3235
Conversation
|
@arturcic I would like to ask you what do you think about to introduce the Newtonsoft.Json package and using JObject instead of GitVersionConfiguration in the following code base: public GitVersionConfiguration Provide(string? workingDirectory, GitVersionConfiguration? overrideConfiguration)
{
var configurationBuilder = new ConfigurationBuilder();
if (workingDirectory != null)
configurationBuilder = configurationBuilder.Add(this.configFileLocator.ReadConfig(workingDirectory));
if (overrideConfiguration != null)
configurationBuilder.Add(overrideConfiguration);
return configurationBuilder.Build();
}I would like to solve the following problem: #3226 (comment) |
|
@HHobeck we use the built in System.Text.Json instead, we removed Newtonsoft.Json some time ago, so please try with that one |
|
I have some trouble with the CI pipeline do you have an idea why this test sometimes failed? Failed FindsVersionInDynamicRepo("GV_main","https://github.com/GitTools/GitVersion","main","2dc142a4a4df77db61a00d9fb7510b18b3c2c85a","5.8.2+47") [2 m 14 s]
Error Message:
System.Exception : There was an unknown problem with the Git repository you provided
----> LibGit2Sharp.LibGit2SharpException : SecureTransport error: connection closed via error
Stack Trace:
at GitVersion.GitRepository.Clone(String sourceUrl, String workdirPath, AuthenticationInfo auth) in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.LibGit2Sharp/Git/GitRepository.cs:line 135
at GitVersion.GitPreparer.<>c__DisplayClass16_0.<CloneRepository>b__0() in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Core/GitPreparer.cs:line 142
at GitVersion.Helpers.RetryAction`1.<>c__DisplayClass1_0.<Execute>b__0() in /Users/runner/work/GitVersion/GitVersion/src/GitVersion.Core/Helpers/RetryAction.cs:line 16
at Polly.Policy`1.<>c__DisplayClass11_0.<Execute>b__0(Context _, CancellationToken _)
at Polly.Retry.RetryEngine.Implementation[TResult](Func`3 action, Context context, CancellationToken cancellationToken, ExceptionPredicates shouldRetryExceptionPredicates, ResultPredicates`1 shouldRetryResultPredicates, Action`4 onRetry, Int32 permittedRetryCount, IEnumerable`1 sleepDurationsEnumerable, Func`4 sleepDurationProvider) |
|
Also this seems to be a problem with dotnet 7 preview? [/repo/tests/integration/core/app.csproj::TargetFramework=net7.0] /usr/local/bin/sdk/7.0.100-rc.2.22477.23/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.DefaultItems.Shared.targets(99,7):
error MSB4028: The "CheckIfPackageReferenceShouldBeFrameworkReference" task's outputs could not be retrieved from the "ShouldAddFrameworkReference" parameter.
Object reference not set to an instance of an object. Thank you very much for helping |
| ### mode | ||
|
|
||
| Sets the `mode` of how GitVersion should create a new version. Read more at | ||
| [versioning modes][modes]. |
There was a problem hiding this comment.
This was surprising. Why is mode removed?
There was a problem hiding this comment.
First of all I want to highlight this discussion.
TLDR:
The versioning mode is configurable on the branch level and therefore I have identified it as a branch configuration property.
Long Story:
For this refactoring the first step was to identify the properties (conceptional) which are used to configure GitVersion in general (so called general or overall configuration) and those properties which are branch related (so called branch configuration).
The general configuration is at least accessible via GitVersionContext.Configuration property (result is of type GitVersionConfiguration).
The branch configuration is dependent in which branch you are and will be returned from EffectiveBranchConfigurationFinder. The logic is highly recursive and the configuration polymorph dependent on the IncrementStrategy. In advance we have a fallback configuration and a unknown configuration which has the same properties like the branch (class of type BranchConfiguration).
For a better understanding and a cleaner code I have reused this class as following:
public class GitVersionConfiguration
{
[YamlMember(Alias = "assembly-versioning-scheme")]
public AssemblyVersioningScheme? AssemblyVersioningScheme { get; set; }
[YamlMember(Alias = "assembly-file-versioning-scheme")]
public AssemblyFileVersioningScheme? AssemblyFileVersioningScheme { get; set; }
// ... and other general configuration properties
[YamlMember(Alias = "fallback-branch")]
public BranchConfiguration? FallbackBranch { get; set; }
[YamlMember(Alias = "unknown-branch")]
public BranchConfiguration? UnknownBranch { get; set; }
[YamlMember(Alias = "branches")]
public Dictionary<string, BranchConfiguration> Branches { get; set; }
}There was a problem hiding this comment.
If mode should be moved or removed, that should come as a separate PR. In this one, please focus on unkown and the fallback (root) configuration concepts. :)
| fallback-branch: | ||
| mode: ContinuousDelivery | ||
| tag: '{BranchName}' | ||
| increment: Inherit | ||
| prevent-increment-of-merged-branch-version: false | ||
| track-merge-target: false | ||
| commit-message-incrementing: Enabled | ||
| regex: '' | ||
| tracks-release-branches: false | ||
| is-release-branch: false | ||
| is-mainline: false |
There was a problem hiding this comment.
Why is the fallback moved from the root? Isn't it intuitive and obvious that the root configuration propagates to the branches?
There was a problem hiding this comment.
If I think about it this might be also a possibility:
public class GitVersionConfiguration : BranchConfiguration
{
[YamlMember(Alias = "assembly-versioning-scheme")]
public AssemblyVersioningScheme? AssemblyVersioningScheme { get; set; }
[YamlMember(Alias = "assembly-file-versioning-scheme")]
public AssemblyFileVersioningScheme? AssemblyFileVersioningScheme { get; set; }
// ... and other general configuration properties
[YamlMember(Alias = "unknown-branch")]
public BranchConfiguration? UnknownBranch { get; set; }
[YamlMember(Alias = "branches")]
public Dictionary<string, BranchConfiguration> Branches { get; set; }
}For the user it might be confusing what properties are branch related (fallback configuration) and what properties are used for the overall configuration.
There was a problem hiding this comment.
I think the root configuration and the branch configuration could probably be mostly the same, but branches being an override for each branch category of a given repository. The root configuration should be the fallback configuration that applies to all branches until the configuration is overridden.
At least for most properties. Perhaps not all. It's something we need to consider and discuss.
There was a problem hiding this comment.
Okay no problem I'm going to move the fallback configuration section to the root location.
I think this statement is not valid:
The root configuration should be the fallback configuration that applies to all branches until the configuration is overridden.
At least the statement should be:
The root configuration defines some fallback properties which applies on those property which are not specified on all branches but not marked with increment strategy
inherit.
Please notice that we are not overriding the configuration from base (fallback configuration) to top (one or more branch configurations). We are inheriting from top (one or more branch configurations) to base (fallback configuration).
There was a problem hiding this comment.
I also think we need to iterate on the name Inherit, since it does not clearly say that the value is inherited from the parent branch.
There was a problem hiding this comment.
I'm not sure if you are aware of that we have not just one or two properties which are branch related. We have at least nine. And in future we are going to introduce more properties e.g. track-merge-message:
mode: ContinuousDelivery
tag: '{BranchName}'
increment: Inherit
prevent-increment-of-merged-branch-version: false
track-merge-target: false
commit-message-incrementing: Enabled
tracks-release-branches: false
is-release-branch: false
is-mainline: falseI don't understand why you want to define the fallback configuration properties on the root configuration. If you doing this it's not clear for the user which properties are used as fallback and which properties are used for general purpose. Because of this fundamental difference I think we have not the same view on this what fallback properties are.
The concept of the branch configuration with polymorphism is so powerful in my opinion you don't need to introduce any additional mechanism (if you add another dimension it will be error prone).
The mode for pull-request will be ContinuousDelivery since that is the mode of the root configuration. I'm not sure about the Default value, as it can be interpreted in one of two ways:
The default value of the pull-request branch category.
The value that is provided as the default in the root configuration.
Why do think the pull-request will be ContinuousDelivery? It's ContinuousDeployment (I know that this example makes no sense from the GitFlow workflow perspective but that is the reason why we have defined the mode to ContinuousDelivery on the PR and not null).
There was a problem hiding this comment.
I'm not sure what we are discussing here. Are we discussing:
- That the fallback configuration section with all branch related properties (at least nine) needs to be placed on the root?
- That the fallback configuration is one mechanism and you want to introduce another mechanism to define branch properties on the root to have a overrule all functionality?
- Some new mode which give the user the possibility to decide where to take the property from (parent, root, actual, fallback)
There was a problem hiding this comment.
Maybe we need to go one step back and discuss the user scenario. The discussion until now is quite technical and we need to focus one what the users want. I can think of a scenario where we have two user groups. One user group which uses ContinuousDelivery and another user group which uses Mainline mode. What we both want is that the user understands the configuration and is able to easily switch the mode.
There was a problem hiding this comment.
I don't understand why you want to define the fallback configuration properties on the root configuration. If you doing this it's not clear for the user which properties are used as fallback and which properties are used for general purpose.
Can't we define that the root configuration has no "general purpose" properties and that all properties on the root configuration provides a fallback for the branch configurations?
That the fallback configuration section with all branch related properties (at least nine) needs to be placed on the root?
Yes (and thus "no" to your other two questions).
One user group which uses ContinuousDelivery and another user group which uses Mainline mode. What we both want is that the user understands the configuration and is able to easily switch the mode.
Yes. For both of these examples, unless the users have specific requirements for how the different branch configurations need to differ from the defaults, the following configuration should be sufficient:
mode: MainlineAnd:
mode: ContinuousDelivery| unknown-branch: | ||
| mode: ContinuousDelivery | ||
| tag: '{BranchName}' | ||
| increment: Inherit | ||
| regex: '' |
There was a problem hiding this comment.
Why isn't unknown-branch just an unknown property beneath the branches property?
There was a problem hiding this comment.
I'm not sure if I understood you correct. Do you mean the position or the name of this property?
There was a problem hiding this comment.
Why isn't
unknown-branchjust anunknownproperty beneath thebranchesproperty?
What happened if the user wants to configure a branch with the name unknown? ;)
There was a problem hiding this comment.
The property name beneath branches is not the name of the actual branch in Git. The name of the actual branch in Git is identified in the child regex property. The property names beneath branches are virtual and identify the branch category which GitVersion has pre-existing knowledge about in order to provide out of the box versioning for widely used workflows such as Git Flow.
There was a problem hiding this comment.
Okay maybe we should rename it to something else. Please notice that I haven't introduce the branch name placeholder (see 71d8fe5#diff-ad2fc424e99560699ab35cf2da3f7b20b939f1ad927a2d5dd8519828f5a09161).
Are you agree with the introduction of the unknown branch section?
There was a problem hiding this comment.
Yes, having an unknown branch category makes sense and is something that is being discussed in #2870 (comment) as well.
There was a problem hiding this comment.
Of course you can add a configuration to the array and match any branches with the regex .* pattern. But if you doing this the order of the branch configuration array is important (and the name of the configuration doesn't matter). Conceptionally you need to introduce a order by property. Otherwise it is hard or even impossible to override the branch configuration in GitVersion.yaml.
Exactly that is reason why I have introduced the unknown branch configuration section to specify the configuration for branches which are not known. Please notice that also here the inheritance works if it has been applied on the unknown branch for instance:
unknown --inherits-from--> master --inherits-from--> fallback configuration
The reason why I'm not sure if it is a good idea to move the unknown branch section in the branch configurations array is, that we have magic string and again special business logic which is not obviously to the user. Introducing a order by property would be better but for our use case over engineered.
There was a problem hiding this comment.
Exactly that is reason why I have introduced the unknown branch configuration section to specify the configuration for branches which are not known.
Yes, exactly. That's why I write:
Yes, having an
unknownbranch category makes sense.
:)
The reason why I'm not sure if it is a good idea to move the unknown branch section in the branch configurations array is, that we have magic string and again special business logic which is not obviously to the user.
Which "magic string" are you referring to?
Introducing a order by property would be better but for our use case over engineered.
I would not agree to add an order property. We already have the idea of an unknown branch in the codebase today, it's just not clearly defined or named. Once we give it a name, it becomes much clearer both in the codebase and in the documentation what it is, how it works, etc.
There was a problem hiding this comment.
It makes no sense to define a regular expression like .* without a order by property. You are not able to add new branches in the GitVersion.yaml file because the .* configuration matches first. Or would you like to introduce a magic string like unknown and say: Please if a unknown branch name existing in the array then move this configuration to the last position? That's not transparent to the user. Sorry I don't see a concept to be able to configure branches who are unknown.
There was a problem hiding this comment.
regex: .* makes sense for the unknown branch category, because it will by definition be evaluated last by GitVersion. Even if unknown is not present in the configuration, GitVersion should have a concept of an unkown branch which has regex: .* by default. The unknown branch configuration is only needed to be present in GitVersion.yml if the user needs it to deviate from the defaults (as with any other branch configuraiton).
| @@ -45,7 +45,8 @@ To change the mode to Continuous Delivery, change your | |||
| [configuration] to: | |||
|
|
|||
| ```yaml | |||
| mode: ContinuousDelivery | |||
| fallback-branch: | |||
| mode: ContinuousDelivery | |||
There was a problem hiding this comment.
It seems like mode is now moved into fallback-branch? If so, it seems to be missing from configuration.md. Also, I'm not sure I agree to moving it from the root.
There was a problem hiding this comment.
Yes that's true I need to document this. But before I do this I hold on and wait for the result of our discussion whether or not we are moving it. ;)
There was a problem hiding this comment.
Actually the big question is what is a VersioningMode and is it branch related or not. In my opinion it is at least for the value ContinuousDelivery and ContinuousDeployment.
Maybe we have mixed two concepts here:
- The concept of being mainline which is maybe a general (overall) configuration (should be placed maybe in the root)
- the concept of ContinuousDelivery and ContinuousDeployment which is for sure branch specific (should be placed in branch configuration)
In my opinion I would like to not having a separate main line mode and reuse the ContinuousDeployment for this scenario as well (maybe in future ;)). If you think about it: What is the different of ContinuousDeployment and Mainline when it has been defined on the main branch?
Defintion in AWS https://aws.amazon.com/devops/continuous-delivery/:
Continuous Delivery vs. Continuous Deployment
With continuous delivery, every code change is built, tested, and then pushed to a non-production testing or staging environment. There can be multiple, parallel test stages before a production deployment. The difference between continuous delivery and continuous deployment is the presence of a manual approval to update to production. With continuous deployment, production happens automatically without explicit approval.
There was a problem hiding this comment.
I agree that mode: Mainline seemingly doesn't provide much value. I'm going to wrap that around in a huge disclaimer, though, as I'm not using Mainline myself.
I'm not entirely sure why it was implemented in the first place. All of the features that were implemented alongside it (such as commit message increments) are features that should be usable regardless of which mode you use.
There was a problem hiding this comment.
If you like I can create a ticket for this to remove the mainline mode and replace it with the ContinuousDeployment mode. In my opinion this two options are equivalent.
There was a problem hiding this comment.
I at least think it's worth exploring. If we can support the same use cases we already do by removing mode: Mainline, it's going to make both the codebase and documentation simpler if we remove it.
|
|
||
| namespace GitVersion.Configuration; | ||
|
|
||
| internal class ConfigurationHelper |
There was a problem hiding this comment.
So does the ConfigurationHelper now replace the deleted ConfigurationBuilder class? I'm not sure I understand this change.
There was a problem hiding this comment.
No. The ConfigurationHelper doesn't replace the ConfigurationBuilder. I'm not sure if this class is named correct. If you have an idea how to implement this in a more elegant way please let me know. Anyway I would describe the motivation/function as following:
The ConfigurationHelper is just a helper class which gets a representation of the configuration (GitVersionConfiguration class, a generic dictionary or a yaml string). It encapsulates the serialization and deserialization steps and converts from one representation to another. In advance you can define a dictionary and override the configuration property independent of the initial representation: void Override(IDictionary<object, object?> dictionary)
There was a problem hiding this comment.
Ok, how about this:
We should only have a single class that represents our internal configuration, called GitVersionConfiguration. This class should reside in GitVersion.Core and should not know anything about serialization or deserialization. We can make this class abstract, since it makes little sense to construct it directly.
In the outer, infrastructural layers of the application, we can have superclasses of GitVersionConfiguration that knows about loading and saving to files, serialization, deserialization, etc. The concrete implementation of GitVersionConfiguration can then be registered at startup in the IOC container.
There was a problem hiding this comment.
Yep I agree for the business logic we only need the GitVersionConfiguration class. The logic how the configuration class is created can be placed outside of the core domain. I'm not sure if we should define this class as abstract. It's just a data transfer object or a business object which can be defined as immutable ;). If you like we can do this refactoring in a separate PR maybe together with the refactoring of the init wizard in #3229.
There was a problem hiding this comment.
A DTO does not belong in the core domain. A DTO is a way to transfer and transform data from the perimeter of the application and into one or more domain objects so the domain objects can ensure data validity and invariants. The reason I suggest it can be abstract (instead of an interface) is so we can define business logic inside the core domain and only implement the de/serialization in the infrastructural part of the application.
Thank you very much taking the time and investing the effort to review my changes. :) |
| unknown-branch: | ||
| mode: ContinuousDelivery | ||
| tag: '{BranchName}' | ||
| increment: Inherit | ||
| regex: '' |
There was a problem hiding this comment.
Yes, having an unknown branch category makes sense and is something that is being discussed in #2870 (comment) as well.
| @@ -54,16 +53,6 @@ public void OverwritesDefaultsWithProvidedConfig() | |||
| configuration.Branches["develop"].Tag.ShouldBe("dev"); | |||
| } | |||
|
|
|||
| [Test] | |||
| public void AllBranchesModeWhenUsingMainline() | |||
There was a problem hiding this comment.
Okay. I'm not on board with your template idea, but I agree that less magic should be enforced by default and that it's better to make the decisions in the configuration instead.
| [YamlMember(Alias = "unknown-branch")] | ||
| public BranchConfiguration? UnknownBranch { get; set; } |
There was a problem hiding this comment.
Please move this into Branches and call it Unknown.
| [YamlMember(Alias = "unknown-branch")] | |
| public BranchConfiguration? UnknownBranch { get; set; } |
There was a problem hiding this comment.
Just want to ask you @asbjornu and you @arturcic (and others?) to have a Microsoft teams meeting to discuss the structure of the configuration? If I read the #2870 issue (and others) it gives me the encouragement that the configuration (how it is defined today) is not intuitive and self-explained. With the unknown and fallback configuration it will be even more complex. We need to discuss in my opinion the following points:
- Separate the fallback branch configuration from the general configuration
- Separate the unknown branch configuration from the normal branch configuration
- Extending the configuration from the user perspective which are already pre defined
I'm not happy with the direction where we are going with the configuration if I'm honest. I see a lot of tickets coming to us in the future because of misusage and misunderstanding. What is about clean code and self explanation? Furthermore if you are not on board with the templating idea @asbjornu what is your suggestion? Could you please elaborate how to support different workflows in future?
If it is because of backwards compatibility we could support the old structure and a new structure of the configuration and define the old one as deprecated in future releases.
There was a problem hiding this comment.
I agree that the current state of affairs in v5 is sub-optimal. By making the configuration completely explicit in every way, I think the situation will be much improved. I'm not against the templating idea for backwards compatibility concerns, but because it seems to add needless complexity. The configuration should be simple. The reason it's not simple today is not due to the lack of a template system, but because it's so hard to deduce what the effective value of a given property in the branch configuration is supposed to be at any point. If we make that explicit, we can document it and reason about it much more clearly.
There was a problem hiding this comment.
As for a Teams meeting, I don't have much time at the moment. I'm able to find a few minutes here and there to invest in GitVersion, but that's it. It's difficult to squeeze in the time required for a full, focused meeting, unfortunately. Let's try to hash things out in text a bit more. I think we're making progress and appreciate the spitballing we have going here, @HHobeck. Thank you for pushing us forward! :)
|
Okay I'm done. Please go ahead and merge it to main. |
BREAKING_CHANGES.md
Outdated
| * be72411 56 minutes ago (develop) | ||
| * 14800ff 58 minutes ago (tag: 1.0.0, main) | ||
| ``` | ||
| * A new `unknown-branch` configuration section has been introduced to give the user the possibility to specify the branch configuration for a branch which is not known. A branch is not known if the regular expressions who are specified in the `branches` section are not matching. The `unknown-branch` section is optional. |
There was a problem hiding this comment.
Please call it unknown and have it underneath branches.
| * A new `unknown-branch` configuration section has been introduced to give the user the possibility to specify the branch configuration for a branch which is not known. A branch is not known if the regular expressions who are specified in the `branches` section are not matching. The `unknown-branch` section is optional. | |
| * A new `unknown` configuration section has been introduced to give the user the possibility to specify the branch configuration for a branch which is not known. A branch is not known if the regular expressions who are specified in the `branches` section are not matching. The `unknown` section is optional. |
BREAKING_CHANGES.md
Outdated
| * 14800ff 58 minutes ago (tag: 1.0.0, main) | ||
| ``` | ||
| * A new `unknown-branch` configuration section has been introduced to give the user the possibility to specify the branch configuration for a branch which is not known. A branch is not known if the regular expressions who are specified in the `branches` section are not matching. The `unknown-branch` section is optional. | ||
| * A new `fallback-branch` configuration section has been introduced to define base properties which will be inherit to the branch configurations. That means if no other branch configuration in the inheritance line defines the given property the fallback property applies. Notice that the inheritance tree can be controlled using the increment strategy property in the branch configuration section. |
There was a problem hiding this comment.
I disagree with this design. The properties defined at the root should act as the "fallback", we do not need a new property for that.
| @@ -371,48 +370,13 @@ public void OverrideconfigWithNoOptions() | |||
| arguments.OverrideConfig.ShouldBeNull(); | |||
| } | |||
|
|
|||
| [TestCaseSource(nameof(OverrideconfigWithInvalidOptionTestData))] | |||
There was a problem hiding this comment.
Why are you removing this test?
| @@ -452,27 +416,13 @@ private static IEnumerable<TestCaseData> OverrideConfigWithSingleOptionTestData( | |||
| AssemblyFileVersioningFormat = "{Major}.{Minor}.{Patch}.{env:CI_JOB_ID ?? 0}" | |||
| } | |||
| ); | |||
| yield return new TestCaseData( | |||
| "mode=ContinuousDelivery", | |||
There was a problem hiding this comment.
Why are you removing all of these test cases?
| ### template | ||
|
|
||
| Allows you to specify which workflow you would like to use. If you want to start building your own configuration from scratch please specify an empty string `template: ''`. This property is optional and defaults to `GitFlow/v1` if not set. Following templates are supported: | ||
| * GitFlow/v1 | ||
| * GitHubFlow/v1 | ||
|
|
There was a problem hiding this comment.
I've written already that I disagree with this design. Please don't implement major changes like these before we've discussed and agreed upon them. If we want a template system, it should first be discussed in an issue where we reach a conclusion on how it should look like before any code is written to implement it. This PR is large enough as it is. :)
| ### template | |
| Allows you to specify which workflow you would like to use. If you want to start building your own configuration from scratch please specify an empty string `template: ''`. This property is optional and defaults to `GitFlow/v1` if not set. Following templates are supported: | |
| * GitFlow/v1 | |
| * GitHubFlow/v1 |
| 11. `patch-version-bump-message` | ||
| 12. `tag-prefix` | ||
| 13. `tag-pre-release-weight` | ||
| 14. `update-build-number` |
There was a problem hiding this comment.
Why are the arguments changed?
| ### mode | ||
|
|
||
| Sets the `mode` of how GitVersion should create a new version. Read more at | ||
| [versioning modes][modes]. |
There was a problem hiding this comment.
If mode should be moved or removed, that should come as a separate PR. In this one, please focus on unkown and the fallback (root) configuration concepts. :)
| fallback-branch: | ||
| mode: ContinuousDelivery | ||
| tag: '{BranchName}' | ||
| increment: Inherit | ||
| prevent-increment-of-merged-branch-version: false | ||
| track-merge-target: false | ||
| commit-message-incrementing: Enabled | ||
| regex: '' | ||
| tracks-release-branches: false | ||
| is-release-branch: false | ||
| is-mainline: false |
There was a problem hiding this comment.
I don't understand why you want to define the fallback configuration properties on the root configuration. If you doing this it's not clear for the user which properties are used as fallback and which properties are used for general purpose.
Can't we define that the root configuration has no "general purpose" properties and that all properties on the root configuration provides a fallback for the branch configurations?
That the fallback configuration section with all branch related properties (at least nine) needs to be placed on the root?
Yes (and thus "no" to your other two questions).
One user group which uses ContinuousDelivery and another user group which uses Mainline mode. What we both want is that the user understands the configuration and is able to easily switch the mode.
Yes. For both of these examples, unless the users have specific requirements for how the different branch configurations need to differ from the defaults, the following configuration should be sufficient:
mode: MainlineAnd:
mode: ContinuousDelivery| unknown-branch: | ||
| mode: ContinuousDelivery | ||
| tag: '{BranchName}' | ||
| increment: Inherit | ||
| regex: '' |
There was a problem hiding this comment.
regex: .* makes sense for the unknown branch category, because it will by definition be evaluated last by GitVersion. Even if unknown is not present in the configuration, GitVersion should have a concept of an unkown branch which has regex: .* by default. The unknown branch configuration is only needed to be present in GitVersion.yml if the user needs it to deviate from the defaults (as with any other branch configuraiton).
This is exactly the point where this two concepts (you are already mentioned it with two source of truth) are clashing together (see #2388 (comment)). You cannot say on the one hand the mode property is a fallback branch property and on the other hand the mode property is overruling all other ones in the branch configurations section. It's conflicting and not working together. I want to keep it simple and remove any special business rules related to the way how the branch configuration will be determined. Again if you take this example: mode: ContinuousDelivery
branches:
main:
mode: ContinuousDeployment
increment: Patch
regex: ^master$|^main$
pre-release-weight: 55000
pull-request:
mode: null
increment: Inherit
regex: ^(pull|pull\-requests|pr)[/-]
source-branches:
- mainThe resulting mode is still not ContinuousDelivery for pullrequest branch it's ContinuousDeployment. Are you agree? So if we take this as a constraint we have four ways solving this in my opinion:
You mentioned a fifth option introducing a new value for the enum IncrementStrategy with the value Default. Could you elaborate it please? |
|
And I'm also not sure if this discussion with the overruling is hypothetically because you are already able to change the mode property on the root layer (fallback property with name mode). This has the effect on the following branches:
It has no effect on the following branches because they are defined explicit with the mode ContinuesDelivery or ContinuesDeployment (Is this not exactly the behavior which was hard coded?):
Because the default configuration pattern for GitFlow looks like the following: assembly-versioning-scheme: MajorMinorPatch
assembly-file-versioning-scheme: MajorMinorPatch
...
branches:
develop:
mode: ContinuousDeployment
...
main:
mode: null
...
source-branches:
- develop
- release
release:
mode: null
...
source-branches:
- develop
- main
- support
- release
feature:
mode: ContinuousDelivery
...
source-branches:
- develop
- main
- release
- feature
- support
- hotfix
pull-request:
mode: ContinuousDelivery
...
source-branches:
- develop
- main
- release
- feature
- support
- hotfix
hotfix:
mode: ContinuousDelivery
...
source-branches:
- release
- main
- support
- hotfix
support:
mode: null
...
source-branches:
- main
unknown:
mode: ContinuousDelivery
...
source-branches:
- main
- develop
- release
- feature
- pull-request
- hotfix
- support
ignore:
sha: []
mode: ContinuousDelivery
...If you are not agree with how the configuration mechanism is designed then we have a huge problem with the concept of inheritance and how it is implemented. Finally I would say skip the mechanism of overruling the branch configuration and enjoy the power of inheriting ;). |
|
@HHobeck, I'm a bit swamped these days with kids having the flu and towering amounts of work that needs to be finished until the holidays. I will get to this as soon as time allows. What may speed things up is if we split this PR in two, so the |
|
@asbjornu I think we need to stop adding new features to the v5 version and focus on version v6. The reason is v5 is targeting .net 5, .net 3.1 that are already unsupported (or almost) and I think it's better to focus the effort on v6. v5 will remain only for critical/security bug fixes. |
Okay no problem take the time you need.
Yes I agree we should focus on v6 and put all our effort to finalize the features and fix bugs which makes the next version of gitversion coherent. |
|
Folks, please take a look on my my post here and let me know if it has been fixed in this branch? |
HHobeck
force-pushed
the
feature/2388_root-configuration-should-propoagate-branches
branch
2 times, most recently
from
6e37e15
to
97f4ca5
Compare
HHobeck
force-pushed
the
feature/2388_root-configuration-should-propoagate-branches
branch
from
fe31991
to
59c3bd5
Compare
… properties in the root configuration. - Create a new configuration section with name unknown to ensure the unknown branch configuration. - The way how the configuration (file, parameter) will be merged with the build-in gitflow configuration has been changed.
HHobeck
force-pushed
the
feature/2388_root-configuration-should-propoagate-branches
branch
from
59c3bd5
to
062bfc4
Compare
|
Thank you @HHobeck for your contribution! |
HHobeck
deleted the
feature/2388_root-configuration-should-propoagate-branches
branch
|
🎉 This issue has been resolved in version 6.0.0-beta.1 🎉 Your GitReleaseManager bot 📦🚀 |
[Feature] Root configuration should propagate to branches #2388
Description
Following refactoring steps have been applied:
unknownto ensure the unknown branch configuration.Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Checklist: