Skip to content

Use the first matched branch configuration #1070

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

Merged
merged 3 commits into from
Nov 27, 2016

Conversation

cvpcs
Copy link
Contributor

@cvpcs cvpcs commented Oct 21, 2016

This change causes GitVersion to allow multiple branch configurations to match the current branch, and instead of throwing an exception it will use the first matching configuration that was found.

Fixes #881

@@ -37,9 +37,6 @@ public class BranchConfigurationCalculator

return keyValuePair;
}

const string format = "Multiple branch configurations match the current branch branchName of '{0}'. Matching configurations: '{1}'";
throw new Exception(string.Format(format, currentBranch.FriendlyName, string.Join(", ", matchingBranches.Select(b => b.Key))));
Copy link
Member

Choose a reason for hiding this comment

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

It would probably be nice to know that there were multiple matches, even though it now doesn't fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a WriteInfo call that will output essentially the old exception message, only now it will specify that it is taking the first configuration and also explicitly state which configuration that is to avoid confusion.

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.

Would it be possible to test this change of behavior somehow?

@cvpcs
Copy link
Contributor Author

cvpcs commented Oct 28, 2016

I added a simple test to the GitVersionContextTests that creates two release configurations with non-mutually exclusive regex values and test which gets picked among two branches, one of which matches both.

@cvpcs
Copy link
Contributor Author

cvpcs commented Oct 28, 2016

I'm not sure what's causing the error on the Travis CI build. It claims its something wrong with DocumentationSamples.GitHubFlowMajorRelease but there doesn't appear to be much output from the log on that test run. Also the error is Win32Exception: Success?

@asbjornu
Copy link
Member

asbjornu commented Nov 1, 2016

@cvpcs: I think I have identified the problem in GitTools/GitTools.Core#38. As soon as that's merged, released and upgraded in GitVersion, I hope we can get rid of this and the problem @mkoertgen is having in #720.

if (matchingBranches.Length > 1)
{
Logger.WriteInfo(string.Format(
"Multiple branch configurations match the current branch branchName of '{0}'. Using the first matching configuration, '{1}'. Matching configurations include: '{2}'",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should probably be a warning..

@asbjornu
Copy link
Member

@cvpcs: Can you please have a look at the feedback you've gotten on this PR and rebase it on master so it hopefully turns green? 😄

This change causes GitVersion to allow multiple branch configurations to
match the current branch, and instead of throwing an exception it will use
the first matching configuration that was found.
This adds a test for validating that the first matched branch
configuration will be chosen when multiple are present that match the
current branch.
@cvpcs cvpcs force-pushed the feature/MatchFirstBranchConfig branch from 090ad80 to 832356c Compare November 26, 2016 19:07
@JakeGinnivan JakeGinnivan merged commit 691b295 into GitTools:master Nov 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants