Config based versioning improvements #346
Conversation
|
At this point there are a heap of failing tests around commit counting. I think we need to introduce the commit counting strategy which @yannisgu talked about in his PR, but didn't get to that. But most of the semantic versions are correct with this PR. Looks like we should be able to make his work |
…t kinda works but lots of small differences causing test breaks
…ature branches off develop or master to increment based off the develop or master increment strategy
…e the branch name to create the tag
…ill a tagged commit. The version should not change
…ng and different test scenarios
JakeGinnivan
force-pushed
the
ConfigBasedVersioningImprovements
branch
from
fb875d0 to
8ab088f
Compare
|
@SimonCropp @andreasohlund @yannisgu this has a green build now The new config based versioning is not turned on. It still needs a line uncommented to switch over to the new approach. There are a heap of failing unit tests once switched, but think that should be a different PR to fix. The failing tests are mainly around commit counting and where pre-release tags start counting from (some branches are 0, others start at 1). Would love a review on the approach. I know it is a massive PR so might be worth pulling code down and stepping through the new stuff |
| { | ||
| var config = new Config(); | ||
| config.Branches["develop"].Increment = IncrementStrategy.Major; | ||
| config.Branches["feature[/-]"].Increment = IncrementStrategy.Inherit; |
There was a problem hiding this comment.
This solves the issue with feature branches which are taken off master should bump patch but feature branches off develop should bump minor
There was a problem hiding this comment.
Isn't IncrementStrategy.Inherit default for feature branches?
There was a problem hiding this comment.
Yeah, I just thought I would make it explicit in the test. The test doesn't make sense without that knowledge
|
The overall architecture implemented in this PR is documented at #338 (comment) |
|
How cool is that? 😄 |
|
@yannisgu so what do you recon. Is this along the lines of what you were thinking? Next steps if this is ok is to merged. Then I will switch the code over so we have failing tests, we can then try and PR to get all the tests green. That is where the commit counting bits will need to come in I think, that was the main source of failing tests from memory |
| CommitterEx = new Signature("Joe", "[email protected]", DateTimeOffset.Now); | ||
| CommitterEx = new Signature("Joe", "[email protected]", when); | ||
| // Make sure each commit is a different time | ||
| when = when.AddSeconds(1); |
There was a problem hiding this comment.
No issues with this and running test multithreaded? Can think of any but just wanted to ensure static shared state was ok
There was a problem hiding this comment.
Not really, if two tests get the same time that is not an issue. It is within the same test we want it to bump
| @@ -18,6 +18,7 @@ public SemanticVersion FindVersion(GitVersionContext context) | |||
| throw new Exception("NextVersion.txt has been depreciated. See https://github.com/ParticularLabs/GitVersion/wiki/GitVersionConfig.yaml-Configuration-File for replacement"); | |||
| } | |||
|
|
|||
| //return new NewNextVersionCalculator().FindVersion(context); | |||
There was a problem hiding this comment.
What is the comment for?
There was a problem hiding this comment.
It turns all this new code on. When this is uncommented a whole heap of tests fail. I wanted to make sure I put the code in without affecting anything existing.
I will uncomment this on the other branch once this PR is ok
There was a problem hiding this comment.
Uncommented this. Tests are now failing.
|
Turned on the new code. This will cause the build to fail Have deleted the branch comparer. Other comments (like namespace) we can fix on the other branch if we can figure out how to make resharper/visual studio do that by default. Are we happy with overall approach of this PR? Is the architecture with the base version finder then apply tags/counting/metadata etc on top of the base version? |
| if (baseVersion.ShouldUpdateTag && !baseVersion.SemanticVersion.PreReleaseTag.HasTag() && !string.IsNullOrEmpty(context.Configuration.Tag)) | ||
| { | ||
| var tagToUse = context.Configuration.Tag; | ||
| if (tagToUse == "useBranchName") |
There was a problem hiding this comment.
I would prefer a boolean property "UseBranchNameAsTag" instead of magic strings.
There was a problem hiding this comment.
I thought about that, but it is a lot harder.
If we specify defaults, then you override do we clear out the tag option. If the user sets useBranchNameAsTag to true and also sets tag do we throw, or ignore the tag?
There was a problem hiding this comment.
I would throw when the users sets useBranchNameAsTag, though ignoring it would also be ok.
There was a problem hiding this comment.
Added as a task on the main PR
|
@JakeGinnivan yeah I like the architecture, I'm really looking forward now on how this plays with the new variable provider! |
|
Ok, will merge this. Then we can start trying to fix tests |
…ements Config based versioning improvements
Builds on #344