Skip to content

Feature/pathfilter #2862

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

Closed
wants to merge 3 commits into from
Closed

Conversation

marc-mueller
Copy link

@marc-mueller marc-mueller commented Sep 29, 2021

Description

Filter on tags on prefix or regex, to make it possible to use multiple git versions in one repository.

Related Issue

Resolves #2436
https://github.com/sebastienwarin/GitVersion/commits/feature/pathfilters

Motivation and Context

Use multiple git versions in one repository

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Marc Müller added 2 commits September 29, 2021 10:36
@marc-mueller
Copy link
Author

Questions to @asbjornu:

  • The path filters are currently applied as commit filter in GetIncrementForCommits in IncrementStrategyFinder but in the debug session, this method is not executed. Did I understand something wrong?
  • Could you please advice me how to get the reference to IGitRepository in the PathFilter? Is the current approach acceptable or am I on a completely wrong track?
  • The configuration is instantiated many times but only once with the content from the configuration yaml file. So only one object has the correct filters in it. Do you build the default config multiple times in the code?

@smasihemami
Copy link

Is there a hope that this pull request gets merged in some time soon?

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.

Is there a hope that this pull request gets merged in some time soon?

Sorry for leaving this unreviewed for so long.

Questions to @asbjornu:

  • The path filters are currently applied as commit filter in GetIncrementForCommits in IncrementStrategyFinder but in the debug session, this method is not executed. Did I understand something wrong?

Perhaps you didn't have the right mode configured in the debug session? A bit hard to say without more information.

  • Could you please advice me how to get the reference to IGitRepository in the PathFilter? Is the current approach acceptable or am I on a completely wrong track?

I think the current solution with using GitVersionContext is fine, but I'd like @arturcic's input here as well.

  • The configuration is instantiated many times but only once with the content from the configuration yaml file. So only one object has the correct filters in it. Do you build the default config multiple times in the code?

Not deliberately. I'm not sure why we would need several Config objects. I would think that should be an immutable singleton.

if (version == null) throw new ArgumentNullException(nameof(version));

reason = null;
if (version.Source.StartsWith("Fallback") || version.Source.StartsWith("Git tag") || version.Source.StartsWith("NextVersion")) return false;
Copy link
Member

Choose a reason for hiding this comment

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

These string matches feel fragile. Is there not something more constant and typesafe we can base this comparison on?


reason = null;

var match = new System.Text.RegularExpressions.Regex($"^({context.Configuration.GitTagPrefix}).*$", System.Text.RegularExpressions.RegexOptions.Compiled);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a using statement to make the code terser.

Suggested change
var match = new System.Text.RegularExpressions.Regex($"^({context.Configuration.GitTagPrefix}).*$", System.Text.RegularExpressions.RegexOptions.Compiled);
var match = new Regex($"^({context.Configuration.GitTagPrefix}).*$", System.Text.RegularExpressions.RegexOptions.Compiled);

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 2, 2022
@asbjornu
Copy link
Member

asbjornu commented Mar 2, 2022

@marc-mueller, do you think you could spend a few minutes going over my feedback and rebasing this onto the HEAD of main? 🙏🏼

@stale stale bot removed the stale label Mar 2, 2022
@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. After 30 days from now, it will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 21, 2022
@gentledepp
Copy link

:-(

@stale stale bot removed the stale label Sep 22, 2022
@asbjornu
Copy link
Member

@gentledepp you are free to fork this PR and submit a new one.

@arturcic
Copy link
Member

@asbjornu I'm going to close this draft as the difference is already too huge for us to maintain. If the author wants to continue the work I would say he needs to re-create the Draft or PR from the current main

@arturcic arturcic closed this Feb 19, 2024
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.

filter on tags on prefix or regex, to make it possible to use multiple git versions in one repository
5 participants