Skip to content

Enable custom merge message formats via configuration #1488

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 14 commits into from
May 11, 2019
Merged

Enable custom merge message formats via configuration #1488

merged 14 commits into from
May 11, 2019

Conversation

Kieranties
Copy link
Contributor

NOTE: This PR has is dependent on the breaking changes introduced by #1487 and should not be approved unless the related breaking changes are approved.

This PR enables custom merge message formats to be added via configuration.

I've now used GitVersion on a few build systems but only ran into issues with merge messages when migrating to TFS. TFS merge messages vary depending on if the merge is a PR, has one or more commits, or is customised by the user completing the PR. In many cases, the merge message will be Merged PR 1234: . this sadly does not fit the baked-in messages that come with GitVersion.

Instead of simply adding another entry into the defaults, this PR enables user to add custom messages formats into their configuration.

@Kieranties
Copy link
Contributor Author

@asbjornu All synched up and green 🎉

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 good, although I have a tingling sense this steps on other pull requests we've recently merged or that are still open. I can't find them now, though, so I might be wrong. I also have a couple of questions I'd appreciate some feedback on.

@Kieranties
Copy link
Contributor Author

@asbjornu I've some time tomorrow to tidy things up as required, then will be AFK for a few days. Let me know what you want to do on the last two points above

@Kieranties
Copy link
Contributor Author

@asbjornu Did you have any further response on the code review? I'll be able to address things this evening. It seems the appveyor build failed for an unrelated reason.

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.

I think this mostly looks good. But I'm thinking, could perhaps merge-message-formats replace the entire TFS merge message support that was implemented in #1591?

@Kieranties
Copy link
Contributor Author

I think this mostly looks good. But I'm thinking, could perhaps merge-message-formats replace the entire TFS merge message support that was implemented in #1591?

Absolutely, but you'll have a regression for consumers of that change. They will need to update their configuration to cater for the TFS messages when they update to 5.0

@asbjornu
Copy link
Member

Absolutely, but you'll have a regression for consumers of that change. They will need to update their configuration to cater for the TFS messages when they update to 5.0

Couldn't the TFS merge messages be added as defaults in merge-message-formats?

@Kieranties
Copy link
Contributor Author

Couldn't the TFS merge messages be added as defaults in merge-message-formats?

I might be confused... Do you mean adding to the default configuration class itself? I'm not sure I see the benefit.

@asbjornu
Copy link
Member

asbjornu commented May 2, 2019

I might be confused... Do you mean adding to the default configuration class itself? I'm not sure I see the benefit.

Don't worry. If anything, I'm the confused one. 😄 I'm thinking that the merge-message-formats implementation can add the TFS merge messages as defaults so the TFS merge message support isn't lost, it's just migrated over to the merge-message-formats property.

@Kieranties
Copy link
Contributor Author

@asbjornu Anything else to do here?

@asbjornu
Copy link
Member

asbjornu commented May 9, 2019

@Kieranties, I don't feel we've got the same understanding of the relationship between merge-message-formats and the TFS merge messages. I still have a feeling we could implement the support for TFS merge messages with the merge-message-formats feature, I'm just unsure how.

Think of it this way: If this PR was merged before #1591, could #1591 have looked differently? If so, how? If we're able to describe "how", I would love to have that implemented in this PR.

@Kieranties
Copy link
Contributor Author

Kieranties commented May 9, 2019

@asbjornu if this had been completed before #1591 then the user would be able to add the following to their configuration:

merge-message-formats:
  TfsMergeMessageEnglishUS: ^Merge (?<SourceBranch>[^\s]*) to (?<TargetBranch>[^\s]*)
  TfsMergeMessageGermanDE: ^Zusammengeführter PR ""(?<PullRequestNumber>\d+)""\: (?<SourceBranch>.*) mit (?<TargetBranch>.*) mergen")

... and #1591 would never have been needed.

However, as #1591 has already been implemented, the above already exist as defaults in the MergeMessage class. So now there is no need for any user to ever specify them, unless they are removed from the list of defaults.

merge-message-formats allows users to specify any number of custom merge messages that will always be evaluated before the built in defaults. So they can add additional formats for existing git systems, or formats for new systems without having to wait for GitVersion to implement a default.

@asbjornu
Copy link
Member

Ok, thanks for the explanation, @Kieranties. It all makes sense now. Then we only have to decide whether we want to undo the changes introduced in #1591 just to keep the core of GitVersion as simple as possible. How do you feel about that, @joergmetzler?

@joergmetzler
Copy link
Contributor

I'm fine with that.

@asbjornu
Copy link
Member

asbjornu commented May 10, 2019

@joergmetzler: Awesome. Can you please effectively revert #1591 then, @Kieranties? I'll be quick to approve and merge after.

@Kieranties
Copy link
Contributor Author

Kieranties commented May 10, 2019 via email

@Kieranties
Copy link
Contributor Author

Ah! Just spotted it's a new thing in 5.0 as well. No regression then 👍

…ages"

This reverts commit a0a6cc4, reversing
changes made to 371a896.

# Conflicts:
#	src/GitVersionCore/MergeMessage.cs
@Kieranties
Copy link
Contributor Author

@asbjornu Good to go!

@asbjornu asbjornu merged commit 9084e6b into GitTools:master May 11, 2019
@asbjornu
Copy link
Member

Awesome, thank you so much for the great work and patience, @Kieranties!

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.

4 participants