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

Add feature ignore regex for commit messages #1507

Closed
wants to merge 14 commits into from
Closed

Add feature ignore regex for commit messages #1507

wants to merge 14 commits into from

Conversation

konne
Copy link

@konne konne commented Oct 18, 2018

We really like to use automatic dependency nuget tools like dependabot.
Since Version 4.0.0-beta 12 the pull request are used to calculate the
version for this packages. This makes no sense for example:

Merge pull request #yy from xxx/dependabot/nuget/src/development/NLog-4.5.9

Because this message contains the new NLog Version and has nothing todo with the
Version of the current package.

The new filter allows to define a general regular expression to for example ignore all
commit messages that contains /dependabot/nuget/

@konne
Copy link
Author

konne commented Oct 18, 2018

this fixes #1495
@david-driscoll please have a look. I think this solves this in a more generic and configurational way compared to your PR #1495

@konne
Copy link
Author

konne commented Oct 18, 2018

@arturcic this is my first PR in your repo, please give me a feedback if I miss something or on the architecture.
We have at the moment a lot of trouble and need to get fixed the merge message parsing that is change during the 4.0-beta versions.

@konne
Copy link
Author

konne commented Oct 18, 2018

@gep13 thanks, I fixed it

@gep13
Copy link
Member

gep13 commented Oct 19, 2018

@konne thanks for fixing that up. Would you be in a position to look at this PR: #1495 as I think it is trying to solve a similar problem, but in a different way.

@konne
Copy link
Author

konne commented Oct 19, 2018

@gep13 yes, but i see the big disadvantage of #1495 that you have to complete disable all merge message version updates, but we still want to use this cool feature for our PRs.
I like to have it just disabled for some merge messages and I can very easy define this with a regex.
So this is the reason I added this PR even if I have already seen the PR from @david-driscoll

@gep13
Copy link
Member

gep13 commented Oct 19, 2018

@konne gotcha. So, do you think that we need both approaches? To completely disable the feature, and also the ability to essentially filter which messages are being used?

@konne
Copy link
Author

konne commented Oct 19, 2018

@gep13 yes, but I think to make the configuration more clear the commit-message should be also move to the ignore.
like

ignore:
sha:
- xxxxxx
- yyyyyy
messages: true

@gep13
Copy link
Member

gep13 commented Oct 19, 2018

@konne can you please comment on that PR with your suggestion? Thanks

@konne
Copy link
Author

konne commented Oct 24, 2018

@gep13 I still waiting for a feedback in the other PR.
Please decide what to do so that I can make it ready and that it can be merged.
Should I add the "message" config to the ignore block, together with this PR

@konne
Copy link
Author

konne commented Oct 27, 2018

@gep13 I have added the content of PR #1495 into that PR.
Now we have two different approaches. First from PR #1495 is to disable complete the usage of the version numbers of merge messages, or to add an regex into the ignore.
Please can you give an feedback and if everything is fine,

Copy link
Contributor

@david-driscoll david-driscoll left a comment

Choose a reason for hiding this comment

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

LGTM (not that it means anything 😄)

@david-driscoll
Copy link
Contributor

@gep13 any chance you have a moment to look at this one? Thanks!

@konne
Copy link
Author

konne commented Dec 17, 2018

@gep13 any change that we get this merge this year? PLEASE

@asbjornu
Copy link
Member

Thanks for this, @konne! How does this relate to #1541? If you believe this is still required, I'd appreciate if the PR could be rebased on the HEAD of master and conflicts resolved. 🙏

@konne
Copy link
Author

konne commented Feb 15, 2019

@asbjornu #1541 should also solve a lot of issues that are comming from the changed version strategy in 4.0. This PR could also used to solve theese issues, but I think it enhance much more the ignore strategy from an simple sha strategy. In my eyes a possibility to just add an regex is nothing that should not be in gitversiontask.

@asbjornu
Copy link
Member

@konne: Great. I'm a bit unsure what your position is on this PR in relation to #1541. Do you still think it's needed? If it is, I'm more fan of the following syntax than what's a part of this PR:

ignore:
    messages: true

@konne
Copy link
Author

konne commented Feb 18, 2019

@asbjornu
This syntax complete disable the usage of versions in the commit message

ignore:
   messages: true

This is in my PR implemented in the way

use-merge-message-version: true

I can easily change it to your syntax.

This PR also contains the possibility to define an regex in the ignore section and this
is in my eyes for more powerusers they only want to ignore just some patterns and want to general use the commit messages versions.

ignore:
  regex: 'ignoreMatch'

@asbjornu
Copy link
Member

@konne, thanks for the summary. After a bit more thought, perhaps this syntax is going to be more consistent with the existing configuration syntax and is perhaps even more intuitive?

merge-message-version: Enabled

ignore:
    merge-message: '<message regex>'

It follows the established pattern of commit-message-incrementing: Enabled for enabling and disabling the feature itself and then follows the pattern of *-bump-message for the regex.

I assume that when you set merge-message-version to Disabled, the ignore.merge-message property will be ignored, right? A warning should perhaps even be written when ignore.merge-message is set if merge-message-version is Disabled.

Thoughts?

@konne
Copy link
Author

konne commented Feb 19, 2019

@asbjornu

I assume that when you set merge-message-version to Disabled, the ignore.merge-message property will be ignored, right?

Yes
and
Yes and a warning should be written.

If you accept this, I will rewrite the PR to fill this requirements.

@asbjornu
Copy link
Member

@konne: Awesome! Yes please. 😃 🙏

@konne
Copy link
Author

konne commented Feb 23, 2019

@asbjornu sorry I get crazy, I just made an fetch to you upstream master, I don't get this solution compiled with my VS2017 and so I tried to just change the PR, but also the only build doesn't compile and I can not debug the root cause.

@asbjornu
Copy link
Member

asbjornu commented Feb 27, 2019

That's ok, @konne. I've created #1609 to rebase and resolve the conflicts. However, I notice now that there's no tests that verify the behavior of the added feature. Could you please add at least one test to this PR and I'll pull it into #1609 after?

If you'd like, you can base your development on my rebased branch. You can do that as follows:

git remote add asbjornu [email protected]:asbjornu/GitVersion
git fetch asbjornu
git checkout -b feature/ignore-merge-messages asbjornu/feature/ignore-merge-messages

You should now have a local branch called feature/ignore-merge-messages you can commit to. You can then push this branch and create a new PR for it:

git push --set-upstream origin feature/ignore-merge-messages

@asbjornu
Copy link
Member

This PR is so out of date now that this has to be reworked from the bottom up, possibly in a completely new way due to #1487 and #1488. I'm therefore closing this PR, but please submit a new one if this is still something you'd like to see in GitVersion.

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.

4 participants