Skip to content
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

Ignore version numbers in branch names that are not release branches. #1541

Merged
merged 13 commits into from
Feb 15, 2019
Merged

Ignore version numbers in branch names that are not release branches. #1541

merged 13 commits into from
Feb 15, 2019

Conversation

bert2
Copy link
Contributor

@bert2 bert2 commented Dec 2, 2018

This fixes #1536.

Introduces a new config switch in the ignore section called non-release-branches. Setting it to true will ignore versions coming from branch names that are not release branches.

Branches that are not release branches will not be considered for version calculation anymore.

Works with versions coming from branch names in merge commits and from the currently checked out branch.

Also works for remote-tracking branches as long as their remote is origin.

Additional notes:

  • There is some duplication in the strategies MergeMessageBaseVersionStrategy and VersionInBranchNameBaseVersionStrategy that I didn't know where to extract it to.
    • The Config class seemed like a good place.
  • The check if a branch is a release branch fails when a remote branch is merged due to the origin/ prefix in the merge commit message: "Merge remote-tracking branch 'origin/release/0.8.0' into develop/master". Should such a merge be considered a merge of a release branch? Please let me know and I'll ammend the PR.
    • Fixed by removing "refs/remotes/<remote-name>/" from the branch name if present. For instance "refs/remotes/origin/release/2.0.0" will be stripped to it's friendly name without the remote, i.e. "release/2.0.0".
  • I'm not sure how some branches should be handled. Originally the branch Particular/release-1.0.0 would always yield the version 1.0.0. With this PR the above branch name will only be considered for version calculation when Particular also is a remote reference of the repository. Is that assumption correct?
    • Only merges of remote-tracking release branches coming from origin can yield versions. All other remotes are ignored.

@bert2
Copy link
Contributor Author

bert2 commented Dec 2, 2018

One of the tests fails on Travis CI, but I don't understand what this is supposed to mean:

Failed CacheKeySameAfterReNormalizing
Error Message:
LibGit2Sharp.LibGit2SharpException : SSL error: syscall failure

@bert2
Copy link
Contributor Author

bert2 commented Dec 3, 2018

Something ain't right with Travis... He failed the first build with a random error in the linux job while the osx job went through.
And now he just failed the osx job with another random error, but the linux job passed?

@asbjornu
Copy link
Member

asbjornu commented Dec 8, 2018

@bert2: I ran the build again, so it's now successful. LGTM, thanks! 😃

@asbjornu
Copy link
Member

asbjornu commented Dec 8, 2018

@GitTools/developers, what's your thoughts on this feature?

@asbjornu
Copy link
Member

As #1422 is close to being merged and we discuss incrementing major with it, I think we should reverse the current behavior and ignore the version number in branches that aren't release branches by default. I'm not even sure we need a config for enabling the current (imho unintuitive) behavior. Thoughts, @bert2?

@bert2
Copy link
Contributor Author

bert2 commented Dec 12, 2018

I'd prefer making it the default behavior as well. I only implemented it as a configuration flag to avoid the possible breaking change.

It makes sense to remove the flag again in this case.

I need some advice regarding the effect of this change to merges of remote branches though (see the 2nd point in the additional notes above). I'm not sure if this is a valid scenario or not.

Also, I won't be able to amend the PR before next Monday (Dec 17th).

@asbjornu
Copy link
Member

@bert2: Yes, whether the branch is remote or local shouldn't matter in its consideration for release classification, I think. So remote or local are just the same; a release branch is a release branch no matter where it lives.

Just take this up when you have time for it and when you do, I suggest you rebase on master (I hope #1422 will be merged by then), remove the config and reverse the logic so it applies by default.

…nfig flag and make it the default behavior); include remote release branches
@bert2
Copy link
Contributor Author

bert2 commented Dec 20, 2018

Hey @asbjornu, I changed the PR as suggested and updated the description above. Three tests are still failing, because I'm not sure what the expected behavior should be. Please see this comment.

Copy link
Member

@asbjornu asbjornu left a comment

Choose a reason for hiding this comment

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

This looks great. Just complete the breaking change so the failing tests succeed. #1422 is merged, we just need to tag it so we increment major to 5 and then I'll merge this. It will then end up as 5.0.0-alpha.2

@bert2
Copy link
Contributor Author

bert2 commented Dec 21, 2018

Alright, the last changes are done, CI is green and description is updated. I think this PR is ready :)

@asbjornu
Copy link
Member

Excellent. We now just need to figure out #1550 and we should be good to go. Thanks for your patience and all the work you’ve put into this! 🙏

@richardgavel
Copy link

Any chance this will be able to be merged soon?

@asbjornu
Copy link
Member

Now that #1581 is merged, I think we're in a better place wrt. #1550. What do you say, @arturcic? Do we have a working product again? I'm sorry I don't have time to test this myself at the moment. 😞

@arturcic
Copy link
Member

More or less yes, we still use older version of Libgit2sharp, and I have integrated the code from GitTools.Core to use directly the Libgit2Sharp library. I'm planning to upgrade it and fix the packaging. For now the current version relies on the workaround to explicitly install the needed libgit2-library on linux

@arturcic
Copy link
Member

I guess we can integrate some PRs that improve the code, as the changes I'm planning are more related to build/packaging

@asbjornu
Copy link
Member

@arturcic, Ok, sounds good. The only thing missing from #1422 then is #1422 (comment). I guess any commit is as good as any for a 5.0.0-alpha.1 tag?

@arturcic
Copy link
Member

@arturcic, Ok, sounds good. The only thing missing from #1422 then is #1422 (comment). I guess any commit is as good as any for a 5.0.0-alpha.1 tag?

Agree

@asbjornu
Copy link
Member

Since #1422 marked the beginning of this whole debacle, I tagged cf2ccfa with 5.0.0-alpha.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why do version numbers in branch names change the calculated version?
4 participants