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.
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
CommitsSinceVersionSource should not decrease upon merging a release branch into develop #2506
Conversation
CommitsSinceVersionSource going down when a release branch is merged to develop and PreventIncrementOfMergedBranchVersion is set to true.
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.
Happy new year, @ruhullahshah! 🎉 I think the test looks fine and that it should be successful. It would be great if the PR could also contain a clarification of the docs on PreventIncrementOfMergedBranchVersion
.
@@ -284,5 +284,50 @@ public void PreviousPreReleaseTagShouldBeRespectedWhenCountingCommits() | |||
|
|||
fixture.AssertFullSemver("1.0.0-alpha.5"); | |||
} | |||
|
|||
[Test] | |||
public void WhenPreventIncrementOfMergedBranchVersionIsSetToTrueCommitsSinceVersionSourceShouldNotGoDownWhenMergingReleaseToDevelop() |
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.
public void WhenPreventIncrementOfMergedBranchVersionIsSetToTrueCommitsSinceVersionSourceShouldNotGoDownWhenMergingReleaseToDevelop() | |
public void WhenPreventIncrementOfMergedBranchVersionIsSetToTrue_AndMergingReleaseToDevelop_CommitsSinceVersionSourceShouldNotDecrement() |
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.
@asbjornu Thanks for the feedback. I would love to enhance the documentation, however, to be honest, I am not so thorough with the use cases that PreventIncrementOfMergedBranchVersion
targets. Having said that, I am sure constructing the fix for this PR would unearth some of the intended use cases, but please feel free to add your thoughts that you think should be added to the relevant docs.
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.
@asbjornu Updated the docs
release branch are the same. These changes might not make it to the final PR, but are necessary for the discussion.
Analysis
The commit graph corresponds to the repository state after merging the release branch to develop (Line 319 of 1. Why does the
|
Thanks for digging into this, @ruhullahshah! 🙏🏼
I don't think that looks correct, no. The commits from the release branch should definitely be in there, imho.
Which consequences does that have? I assume that will break other tests? |
fcfffed
to
e0bff7a
Compare
@asbjornu You are welcome. Here are my further findings:
Conclusion:I do not think any code changes are necessary, the behavior should be documented (done already) and a unit test documenting this behavior should be added (done already) |
@asbjornu Sorry, missed to answer this:
The consequences of setting |
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.
Perfect! Great work, @ruhullahshah!
@asbjornu Thanks. I think in a Gitflow-based setting, we might also need to check the implications of merging a hotfix/major.minor.patch to I will investigate this and we could merge these changes afterwards. |
Indeed, a test for that would be nice.
Since the build is failing due to what seems to be AzDO outage, please feel free to add to this PR. |
…nSource when a hotfix branch is merged to develop
@asbjornu I have validated that setting |
Thank you so much for your contributions, @ruhullahshah! ❤️ 🙏🏼 |
You are welcome @asbjornu |
CommitsSinceVersionSource
decreases when a release branch is merged to develop andPreventIncrementOfMergedBranchVersion
is set totrue
.For now I have added a minimalistic testcase, exhibiting the problem defined in issue #2505.